From edb89eaf271b605df7a8ba50e6bd30cc15025f2f Mon Sep 17 00:00:00 2001 From: dadajian Date: Thu, 21 May 2026 17:08:12 -0500 Subject: [PATCH 01/20] docs: add implementation plan for manifest-based visual comparison Co-Authored-By: Claude Opus 4.6 --- MANIFEST_PLAN.md | 123 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 MANIFEST_PLAN.md diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md new file mode 100644 index 00000000..9b1da096 --- /dev/null +++ b/MANIFEST_PLAN.md @@ -0,0 +1,123 @@ +# Manifest-Based Visual Comparison — Implementation Plan + +## Architecture Summary + +Three new workflow modes added to the existing action: + +| Mode | Trigger | Purpose | +| ------------------- | ------------------------------ | ------------------------------------------------------------- | +| `manifest-generate` | PR push | Run tests, hash screenshots, upload images + manifest to S3 | +| `manifest-compare` | PR (after generate) | 3-way hash comparison, diff generation, GitHub status/comment | +| `manifest-merge` | `pull_request` closed (merged) | Overlay changeset onto HEAD manifest, update base images | + +## S3 Structure (new) + +``` +bucket/ +├── manifests/{commit-sha}.json # Full manifest: { "pkg/path/screenshot.png": "md5hash", ... } +├── changesets/{pr-head-sha}.json # Changeset: { "changed.png": "newhash", "deleted.png": null } +├── base-images/[path]/base.png # (existing) Updated by manifest-merge +├── new-images/{sha}/[path]/new.png # (existing) Only changed images uploaded per commit +└── original-new-images/{sha}/[path]/new.png # (existing) Full-size originals if resize enabled +``` + +## File Schemas + +### Manifest (`manifests/{commit-sha}.json`) + +```json +{ + "components/Button/screenshot.png": "d41d8cd98f00b204e9800998ecf8427e", + "components/Modal/screenshot.png": "7d793037a076d2e1f3eb15d3a5e4389a", + "pages/Home/screenshot.png": "098f6bcd4621d373cade4e832627b4f6" +} +``` + +A flat object mapping each screenshot's relative path (prefixed with package path for monorepos) to its MD5 hash of the full-size image. + +### Changeset (`changesets/{pr-head-sha}.json`) + +```json +{ + "components/Button/screenshot.png": "a3c2f8d1b4e6a9c7d2f0e1b3a5c7d9f1", + "components/Removed/screenshot.png": null +} +``` + +A flat object containing only entries the PR changed. Non-null values are the PR's new hash. `null` indicates the screenshot was deleted by the PR. + +## Flow Details + +### `manifest-generate` mode + +1. Run `visual-test-command` (no base image download, no diff expected) +2. Walk screenshots directory, compute MD5 hash of each full-size image +3. Fetch the HEAD manifest from S3 to determine which hashes changed (if no manifest exists, treat as empty — all images upload) +4. Upload only changed images to `new-images/{commit-sha}/path/new.png` +5. If resize enabled: upload resized to `new-images/`, upload full-size original to `original-new-images/` +6. Upload manifest to `manifests/{commit-sha}.json` + +### `manifest-compare` mode + +1. Fetch PR's manifest from `manifests/{pr-head-sha}.json` +2. Resolve latest main HEAD via GitHub API (`GET /repos/{owner}/{repo}/branches/{base.ref}`) +3. Fetch HEAD's manifest from `manifests/{head-sha}.json` +4. Compare hashes: + - **All match** → set success status, done + - **At least one differs** → proceed to 3-way comparison +5. Resolve ancestor SHA via GitHub Compare API (`GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` → `merge_base_commit.sha`) +6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail with rebase instruction if missing) +7. For each differing screenshot, run 3-way comparison (treat missing entries as a distinct state): + - **Scenario 1 (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`, generate diff.png via pixelmatch, upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` + - Special case: new screenshot (not in HEAD or ancestor) → no base.png or diff.png, just new.png + - Special case: PR deletes screenshot (not in PR, HEAD = ancestor) → note deletion, no images to upload + - **Scenario 2 (PR = ancestor):** Main changed, PR is clean → pass (log informational message) + - Includes: screenshot added on main since branching (in HEAD only) + - **Scenario 3 (all different):** Conflict → collect conflicting paths +8. Determine outcome: + - All Scenario 2 → success status + - Any Scenario 1 (and no Scenario 3) → pending status + Comparadise comment + - Any Scenario 3 → failure status + comment listing conflicts with rebase instruction +9. If no Scenario 3 conflicts, write changeset to `changesets/{pr-head-sha}.json`: + - Changed entries (Scenario 1): `"path": "pr-hash"` + - Deleted entries (in ancestor but not in PR): `"path": null` + - Scenario 2 entries: not included (HEAD's values are correct) + - Skip entirely if outcome is failure (Scenario 3) + +### `manifest-merge` mode + +1. Get PR head SHA from `github.event.pull_request.head.sha` +2. Get merge commit SHA from `github.event.pull_request.merge_commit_sha` +3. Fetch changeset from `changesets/{pr-head-sha}.json` (if missing, treat as no changes) +4. Fetch first parent of merge commit via GitHub API (`parents[0].sha`) → load `manifests/{parent-sha}.json` +5. If no changeset: copy parent manifest as-is to `manifests/{merge-commit-sha}.json`, done +6. Overlay changeset onto HEAD manifest: + - Non-null entries: update hash + - Null entries: remove key +7. Write result to `manifests/{merge-commit-sha}.json` +8. Update base images: for each non-null changeset entry, copy `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png`. For null entries, delete `base-images/path/base.png`. + +## Design Decisions + +- **Coexistence:** New modes alongside existing `pr`/`merge` — consumers opt in +- **Hashing:** MD5 via Node.js `crypto`. Always computed from the full-size image regardless of resize settings +- **Missing ancestor manifest:** Fail with rebase instruction (only during initial adoption) +- **Staleness handling:** Changeset overlay at merge time ensures concurrent merges are handled correctly +- **Merge concurrency:** Consumer should use a `concurrency` group on their `manifest-merge` workflow to serialize merge jobs +- **Post-merge conflicts:** `manifest-merge` applies the changeset without re-validating. Accepted risk — consumers should ensure `manifest-compare` status is required before merge + +## No Changes To + +- Existing `pr` and `merge` workflow modes +- Comparadise web app (uses same `new-images/` structure) +- `comparadise-utils` npm package (for now) +- GitHub Action inputs (only `workflow` gets new valid values) + +## Implementation Order + +1. Manifest utilities: hashing, reading/writing manifests to S3, changeset computation +2. `manifest-generate` mode +3. `manifest-compare` mode (3-way logic + diff generation) +4. `manifest-merge` mode (overlay + base image updates) +5. Tests for each mode +6. Documentation update From f697ab317f8d3058fb4cd8d79dceb954f36eb55c Mon Sep 17 00:00:00 2001 From: dadajian Date: Tue, 26 May 2026 08:51:49 -0500 Subject: [PATCH 02/20] clarify --- MANIFEST_PLAN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index 9b1da096..73c991ff 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -103,7 +103,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **Hashing:** MD5 via Node.js `crypto`. Always computed from the full-size image regardless of resize settings - **Missing ancestor manifest:** Fail with rebase instruction (only during initial adoption) - **Staleness handling:** Changeset overlay at merge time ensures concurrent merges are handled correctly -- **Merge concurrency:** Consumer should use a `concurrency` group on their `manifest-merge` workflow to serialize merge jobs +- **Merge concurrency:** Consumers **must** set a `concurrency` group (with `cancel-in-progress: false`) on their `manifest-merge` workflow to serialize merge jobs. Without it, two simultaneous merges can both update `base-images/` at the same time, producing a corrupted or interleaved state that `manifest-compare` jobs running in parallel will read. The concrete race: PR A and PR B merge within seconds of each other; both `manifest-merge` jobs start concurrently, each overwriting overlapping `base-images/` keys; a `manifest-compare` job for an open PR C reads `base-images/` mid-update and generates a diff against a partially-applied base, producing a wrong or misleading visual result. Serializing merges via `concurrency` eliminates this window entirely. - **Post-merge conflicts:** `manifest-merge` applies the changeset without re-validating. Accepted risk — consumers should ensure `manifest-compare` status is required before merge ## No Changes To From a45822945ce84f16af9313b0b16208d98db37ca4 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Tue, 26 May 2026 16:31:06 -0500 Subject: [PATCH 03/20] stale detection (#775) --- MANIFEST_PLAN.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index 73c991ff..dbfeb0a5 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -39,12 +39,13 @@ A flat object mapping each screenshot's relative path (prefixed with package pat ```json { + "_headSha": "abc123def456...", "components/Button/screenshot.png": "a3c2f8d1b4e6a9c7d2f0e1b3a5c7d9f1", "components/Removed/screenshot.png": null } ``` -A flat object containing only entries the PR changed. Non-null values are the PR's new hash. `null` indicates the screenshot was deleted by the PR. +A flat object containing only entries the PR changed. Non-null values are the PR's new hash. `null` indicates the screenshot was deleted by the PR. `_headSha` records the main HEAD SHA that `manifest-compare` resolved when the changeset was written — used by `manifest-merge` to detect stale changesets. ## Flow Details @@ -79,6 +80,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - Any Scenario 1 (and no Scenario 3) → pending status + Comparadise comment - Any Scenario 3 → failure status + comment listing conflicts with rebase instruction 9. If no Scenario 3 conflicts, write changeset to `changesets/{pr-head-sha}.json`: + - `_headSha`: the HEAD SHA resolved in step 2 - Changed entries (Scenario 1): `"path": "pr-hash"` - Deleted entries (in ancestor but not in PR): `"path": null` - Scenario 2 entries: not included (HEAD's values are correct) @@ -91,11 +93,17 @@ A flat object containing only entries the PR changed. Non-null values are the PR 3. Fetch changeset from `changesets/{pr-head-sha}.json` (if missing, treat as no changes) 4. Fetch first parent of merge commit via GitHub API (`parents[0].sha`) → load `manifests/{parent-sha}.json` 5. If no changeset: copy parent manifest as-is to `manifests/{merge-commit-sha}.json`, done -6. Overlay changeset onto HEAD manifest: +6. Stale changeset check: if `changeset._headSha !== parents[0].sha`: + - Fetch `manifests/{changeset._headSha}.json` (the manifest HEAD at compare time) + - For each screenshot key in the changeset (excluding `_headSha`), compare its hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` + - If any key differs between the two manifests: record the conflicting paths and continue (base images will not be updated) + - If no keys differ: proceed (the intervening merges didn't touch the same screenshots) +7. Overlay changeset onto parent manifest, skipping any conflicting paths identified in step 6: - Non-null entries: update hash - Null entries: remove key -7. Write result to `manifests/{merge-commit-sha}.json` -8. Update base images: for each non-null changeset entry, copy `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png`. For null entries, delete `base-images/path/base.png`. +8. Write result to `manifests/{merge-commit-sha}.json` +9. If conflicting paths were recorded in step 6: fail with the list of conflicting paths +10. Update base images: for each non-null changeset entry, copy `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png`. For null entries, delete `base-images/path/base.png`. ## Design Decisions @@ -104,7 +112,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **Missing ancestor manifest:** Fail with rebase instruction (only during initial adoption) - **Staleness handling:** Changeset overlay at merge time ensures concurrent merges are handled correctly - **Merge concurrency:** Consumers **must** set a `concurrency` group (with `cancel-in-progress: false`) on their `manifest-merge` workflow to serialize merge jobs. Without it, two simultaneous merges can both update `base-images/` at the same time, producing a corrupted or interleaved state that `manifest-compare` jobs running in parallel will read. The concrete race: PR A and PR B merge within seconds of each other; both `manifest-merge` jobs start concurrently, each overwriting overlapping `base-images/` keys; a `manifest-compare` job for an open PR C reads `base-images/` mid-update and generates a diff against a partially-applied base, producing a wrong or misleading visual result. Serializing merges via `concurrency` eliminates this window entirely. -- **Post-merge conflicts:** `manifest-merge` applies the changeset without re-validating. Accepted risk — consumers should ensure `manifest-compare` status is required before merge +- **Stale changeset detection:** The changeset stores `_headSha` (the HEAD SHA at compare time). If `manifest-merge` finds a different parent, it fetches both manifests and checks only the keys present in the changeset. If none overlap, the merge proceeds. If any overlap, the manifest is still written (so future PRs branching off the merge commit get a correct ancestor manifest and auto-pass via Scenario 2), but the MD5 hashes and base images are not updated for conflicting paths, and the job fails with the conflicting paths listed. ## No Changes To From 52510fb8a49f3b751cd2caeb99c85f8f6ae85f4c Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Wed, 27 May 2026 10:36:39 -0500 Subject: [PATCH 04/20] proactive check (#777) --- MANIFEST_PLAN.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index dbfeb0a5..aa4dc816 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -91,18 +91,18 @@ A flat object containing only entries the PR changed. Non-null values are the PR 1. Get PR head SHA from `github.event.pull_request.head.sha` 2. Get merge commit SHA from `github.event.pull_request.merge_commit_sha` 3. Fetch changeset from `changesets/{pr-head-sha}.json` (if missing, treat as no changes) -4. Fetch first parent of merge commit via GitHub API (`parents[0].sha`) → load `manifests/{parent-sha}.json` -5. If no changeset: copy parent manifest as-is to `manifests/{merge-commit-sha}.json`, done -6. Stale changeset check: if `changeset._headSha !== parents[0].sha`: +4. Conflict prevention: if changeset exists, list all open PRs via GitHub API; for each open PR, fetch `changesets/{pr-head-sha}.json` from S3 (skip if missing); for each open PR whose changeset shares at least one screenshot key with this PR's changeset, set a failure commit status on that PR's head SHA (message: "Visual comparison outdated — please rebase") +5. Fetch first parent of merge commit via GitHub API (`parents[0].sha`) → load `manifests/{parent-sha}.json` +6. If no changeset: copy parent manifest as-is to `manifests/{merge-commit-sha}.json`, done +7. Stale changeset check: if `changeset._headSha !== parents[0].sha`: - Fetch `manifests/{changeset._headSha}.json` (the manifest HEAD at compare time) - For each screenshot key in the changeset (excluding `_headSha`), compare its hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` - - If any key differs between the two manifests: record the conflicting paths and continue (base images will not be updated) + - If any key differs between the two manifests: fail with the list of conflicting paths (conflict prevention in step 4 should have prevented this; treat as a safeguard) - If no keys differ: proceed (the intervening merges didn't touch the same screenshots) -7. Overlay changeset onto parent manifest, skipping any conflicting paths identified in step 6: +8. Overlay changeset onto parent manifest: - Non-null entries: update hash - Null entries: remove key -8. Write result to `manifests/{merge-commit-sha}.json` -9. If conflicting paths were recorded in step 6: fail with the list of conflicting paths +9. Write result to `manifests/{merge-commit-sha}.json` 10. Update base images: for each non-null changeset entry, copy `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png`. For null entries, delete `base-images/path/base.png`. ## Design Decisions @@ -112,7 +112,8 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **Missing ancestor manifest:** Fail with rebase instruction (only during initial adoption) - **Staleness handling:** Changeset overlay at merge time ensures concurrent merges are handled correctly - **Merge concurrency:** Consumers **must** set a `concurrency` group (with `cancel-in-progress: false`) on their `manifest-merge` workflow to serialize merge jobs. Without it, two simultaneous merges can both update `base-images/` at the same time, producing a corrupted or interleaved state that `manifest-compare` jobs running in parallel will read. The concrete race: PR A and PR B merge within seconds of each other; both `manifest-merge` jobs start concurrently, each overwriting overlapping `base-images/` keys; a `manifest-compare` job for an open PR C reads `base-images/` mid-update and generates a diff against a partially-applied base, producing a wrong or misleading visual result. Serializing merges via `concurrency` eliminates this window entirely. -- **Stale changeset detection:** The changeset stores `_headSha` (the HEAD SHA at compare time). If `manifest-merge` finds a different parent, it fetches both manifests and checks only the keys present in the changeset. If none overlap, the merge proceeds. If any overlap, the manifest is still written (so future PRs branching off the merge commit get a correct ancestor manifest and auto-pass via Scenario 2), but the MD5 hashes and base images are not updated for conflicting paths, and the job fails with the conflicting paths listed. +- **Stale changeset detection:** The changeset stores `_headSha` (the HEAD SHA at compare time). + At merge time, open PRs whose changesets overlap with the merging PR's changeset are proactively failed with a "rebase required" commit status — preventing the stale scenario from occurring. If `manifest-merge` nonetheless finds a stale changeset with overlapping paths (e.g. due to a race or the conflict prevention step being skipped), it fails the merge job with the conflicting paths listed as a safeguard. If the stale changeset has no overlapping paths, the merge proceeds normally. ## No Changes To From f4a0e70f11264b0c972bf1a62debf9171efdab5c Mon Sep 17 00:00:00 2001 From: Erik Krietsch Date: Wed, 27 May 2026 11:41:44 -0500 Subject: [PATCH 05/20] Add semantic names for compare mode scenarios --- MANIFEST_PLAN.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index aa4dc816..763ac899 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -69,12 +69,12 @@ A flat object containing only entries the PR changed. Non-null values are the PR 5. Resolve ancestor SHA via GitHub Compare API (`GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` → `merge_base_commit.sha`) 6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail with rebase instruction if missing) 7. For each differing screenshot, run 3-way comparison (treat missing entries as a distinct state): - - **Scenario 1 (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`, generate diff.png via pixelmatch, upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` + - **PR Owns (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`, generate diff.png via pixelmatch, upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` - Special case: new screenshot (not in HEAD or ancestor) → no base.png or diff.png, just new.png - Special case: PR deletes screenshot (not in PR, HEAD = ancestor) → note deletion, no images to upload - - **Scenario 2 (PR = ancestor):** Main changed, PR is clean → pass (log informational message) + - **Main Owns (PR = ancestor):** Main changed, PR is clean → pass (log informational message) - Includes: screenshot added on main since branching (in HEAD only) - - **Scenario 3 (all different):** Conflict → collect conflicting paths + - **Conflict (all different):** Conflict → collect conflicting paths 8. Determine outcome: - All Scenario 2 → success status - Any Scenario 1 (and no Scenario 3) → pending status + Comparadise comment From 76527d4a44bb2e450538f78c0084aae3c73a1b61 Mon Sep 17 00:00:00 2001 From: Dan Adajian Date: Thu, 28 May 2026 10:15:06 -0500 Subject: [PATCH 06/20] docs(manifest-plan): use directory paths as manifest keys (#783) Co-authored-by: Claude Sonnet 4.6 --- MANIFEST_PLAN.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index 763ac899..806a3aa5 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -14,8 +14,8 @@ Three new workflow modes added to the existing action: ``` bucket/ -├── manifests/{commit-sha}.json # Full manifest: { "pkg/path/screenshot.png": "md5hash", ... } -├── changesets/{pr-head-sha}.json # Changeset: { "changed.png": "newhash", "deleted.png": null } +├── manifests/{commit-sha}.json # Full manifest: { "pkg/path/component-dir": "md5hash", ... } +├── changesets/{pr-head-sha}.json # Changeset: { "pkg/path/component-dir": "newhash", "pkg/path/other-dir": null } ├── base-images/[path]/base.png # (existing) Updated by manifest-merge ├── new-images/{sha}/[path]/new.png # (existing) Only changed images uploaded per commit └── original-new-images/{sha}/[path]/new.png # (existing) Full-size originals if resize enabled @@ -27,21 +27,21 @@ bucket/ ```json { - "components/Button/screenshot.png": "d41d8cd98f00b204e9800998ecf8427e", - "components/Modal/screenshot.png": "7d793037a076d2e1f3eb15d3a5e4389a", - "pages/Home/screenshot.png": "098f6bcd4621d373cade4e832627b4f6" + "components/Button": "d41d8cd98f00b204e9800998ecf8427e", + "components/Modal": "7d793037a076d2e1f3eb15d3a5e4389a", + "pages/Home": "098f6bcd4621d373cade4e832627b4f6" } ``` -A flat object mapping each screenshot's relative path (prefixed with package path for monorepos) to its MD5 hash of the full-size image. +A flat object mapping each screenshot directory's relative path (prefixed with package path for monorepos) to the MD5 hash of the image file in that directory. ### Changeset (`changesets/{pr-head-sha}.json`) ```json { "_headSha": "abc123def456...", - "components/Button/screenshot.png": "a3c2f8d1b4e6a9c7d2f0e1b3a5c7d9f1", - "components/Removed/screenshot.png": null + "components/Button": "a3c2f8d1b4e6a9c7d2f0e1b3a5c7d9f1", + "components/Removed": null } ``` @@ -52,7 +52,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR ### `manifest-generate` mode 1. Run `visual-test-command` (no base image download, no diff expected) -2. Walk screenshots directory, compute MD5 hash of each full-size image +2. Walk screenshots directory, compute MD5 hash of each new.png image file; key each entry by the containing directory's path relative to the screenshots root 3. Fetch the HEAD manifest from S3 to determine which hashes changed (if no manifest exists, treat as empty — all images upload) 4. Upload only changed images to `new-images/{commit-sha}/path/new.png` 5. If resize enabled: upload resized to `new-images/`, upload full-size original to `original-new-images/` @@ -68,7 +68,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **At least one differs** → proceed to 3-way comparison 5. Resolve ancestor SHA via GitHub Compare API (`GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` → `merge_base_commit.sha`) 6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail with rebase instruction if missing) -7. For each differing screenshot, run 3-way comparison (treat missing entries as a distinct state): +7. For each differing hash, run 3-way comparison (treat missing entries as a distinct state): - **PR Owns (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`, generate diff.png via pixelmatch, upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` - Special case: new screenshot (not in HEAD or ancestor) → no base.png or diff.png, just new.png - Special case: PR deletes screenshot (not in PR, HEAD = ancestor) → note deletion, no images to upload From 7905a0357a2552d33eaacc6dc6eee3269f1879f4 Mon Sep 17 00:00:00 2001 From: dadajian Date: Thu, 28 May 2026 17:14:27 -0500 Subject: [PATCH 07/20] docs(manifest-plan): clarify and simplify resize behavior across all three modes Co-Authored-By: Claude Sonnet 4.6 --- MANIFEST_PLAN.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index 806a3aa5..b3811dee 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -17,8 +17,7 @@ bucket/ ├── manifests/{commit-sha}.json # Full manifest: { "pkg/path/component-dir": "md5hash", ... } ├── changesets/{pr-head-sha}.json # Changeset: { "pkg/path/component-dir": "newhash", "pkg/path/other-dir": null } ├── base-images/[path]/base.png # (existing) Updated by manifest-merge -├── new-images/{sha}/[path]/new.png # (existing) Only changed images uploaded per commit -└── original-new-images/{sha}/[path]/new.png # (existing) Full-size originals if resize enabled +└── new-images/{sha}/[path]/new.png # (existing) Only changed images uploaded per commit ``` ## File Schemas @@ -54,9 +53,8 @@ A flat object containing only entries the PR changed. Non-null values are the PR 1. Run `visual-test-command` (no base image download, no diff expected) 2. Walk screenshots directory, compute MD5 hash of each new.png image file; key each entry by the containing directory's path relative to the screenshots root 3. Fetch the HEAD manifest from S3 to determine which hashes changed (if no manifest exists, treat as empty — all images upload) -4. Upload only changed images to `new-images/{commit-sha}/path/new.png` -5. If resize enabled: upload resized to `new-images/`, upload full-size original to `original-new-images/` -6. Upload manifest to `manifests/{commit-sha}.json` +4. Upload only changed images to `new-images/{commit-sha}/path/new.png`; if resize enabled, resize before upload +5. Upload manifest to `manifests/{commit-sha}.json` ### `manifest-compare` mode @@ -69,7 +67,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR 5. Resolve ancestor SHA via GitHub Compare API (`GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` → `merge_base_commit.sha`) 6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail with rebase instruction if missing) 7. For each differing hash, run 3-way comparison (treat missing entries as a distinct state): - - **PR Owns (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`, generate diff.png via pixelmatch, upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` + - **PR Owns (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`; generate diff.png via pixelmatch; upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` (resize if enabled) - Special case: new screenshot (not in HEAD or ancestor) → no base.png or diff.png, just new.png - Special case: PR deletes screenshot (not in PR, HEAD = ancestor) → note deletion, no images to upload - **Main Owns (PR = ancestor):** Main changed, PR is clean → pass (log informational message) From 2344bec2b9a678d2016c7cbfe25dc1374e56416e Mon Sep 17 00:00:00 2001 From: dadajian Date: Thu, 28 May 2026 17:29:12 -0500 Subject: [PATCH 08/20] docs(manifest-plan): update ancestor manifest error message and AC doc Co-Authored-By: Claude Sonnet 4.6 --- MANIFEST_AC.md | 291 +++++++++++++++++++++++++++++++++++++++++++++++ MANIFEST_PLAN.md | 4 +- 2 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 MANIFEST_AC.md diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md new file mode 100644 index 00000000..76f33a61 --- /dev/null +++ b/MANIFEST_AC.md @@ -0,0 +1,291 @@ +# Manifest-Based Visual Comparison — Acceptance Criteria + +This document is the authoritative acceptance criteria for the `manifest-generate`, `manifest-compare`, and `manifest-merge` workflow modes described in `MANIFEST_PLAN.md`. It is intended for use during PR review to verify the implementation satisfies every behavioral requirement. + +--- + +## 1. `manifest-generate` mode + +### 1.1 Normal run — differential upload + +**Given** the `workflow` input is `manifest-generate`, a HEAD manifest exists at `manifests/{head-sha}.json` in S3, and `visual-test-command` completes successfully +**When** the action runs +**Then**: + +- The MD5 hash of each `new.png` in the screenshots directory is computed using Node.js `crypto` from the full-size image on disk +- Each entry is keyed by the screenshot directory's path relative to the screenshots root (prefixed with package path for monorepos) +- Only images whose hash differs from the HEAD manifest are uploaded to `new-images/{commit-sha}/path/new.png` +- If resize is enabled: changed images are resized before upload +- If resize is not enabled: changed images are uploaded full-size +- Images whose hash matches the HEAD manifest are not uploaded +- Nothing is ever written to `original-new-images/` +- A manifest is written to `manifests/{commit-sha}.json` mapping every screenshot directory path to its MD5 hash (all screenshots, not just changed ones) + +### 1.2 First run — no HEAD manifest exists + +**Given** the `workflow` input is `manifest-generate` and no manifest exists at `manifests/{head-sha}.json` in S3 +**When** the action runs +**Then**: + +- The missing manifest is treated as an empty object (no hash comparisons are performed) +- All images are uploaded to `new-images/{commit-sha}/path/new.png` following the same resize rules as 1.1 +- A manifest is written to `manifests/{commit-sha}.json` for all screenshots + +### 1.3 Hashing is always from full-size image + +**Given** any run of `manifest-generate` +**When** hashes are computed +**Then** each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied + +--- + +## 2. `manifest-compare` mode + +### 2.1 All hashes match — success + +**Given** the `workflow` input is `manifest-compare`, the PR manifest at `manifests/{pr-head-sha}.json` and the HEAD manifest at `manifests/{head-sha}.json` both exist, and every hash in the PR manifest matches the corresponding hash in the HEAD manifest +**When** the action runs +**Then**: + +- A success commit status is set on the PR head SHA +- No changeset is written +- No Comparadise comment is posted + +### 2.2 PR Owns — PR introduced a visual diff (normal case) + +**Given** at least one hash differs between the PR manifest and the HEAD manifest, and for a given differing path the HEAD hash equals the ancestor hash (PR introduced the change) +**When** the 3-way comparison runs for that path +**Then**: + +- `base.png` is downloaded from `base-images/path/base.png` +- `new.png` is downloaded from `new-images/{pr-sha}/path/new.png` (as uploaded by `manifest-generate`) +- A `diff.png` is generated via pixelmatch +- `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload + +### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) + +**Given** a path exists in the PR manifest but does not exist in either the HEAD manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +- No `base.png` is downloaded +- No `diff.png` is generated or uploaded +- Only `new.png` is referenced (already uploaded by `manifest-generate`) +- The path is treated as a PR-owned change (contributes to pending status and Comparadise comment) + +### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) + +**Given** a path exists in the ancestor manifest and the HEAD manifest with the same hash, but does not exist in the PR manifest +**When** the 3-way comparison runs for that path +**Then**: + +- The deletion is logged as an informational message +- The path is recorded as a `null` entry in the changeset +- The path does not contribute to pending status or Comparadise comment + +### 2.5 Main Owns — main changed, PR is clean + +**Given** for a given differing path the PR hash equals the ancestor hash (main introduced the change, not the PR) +**When** the 3-way comparison runs for that path +**Then**: + +- No diff is generated for that path +- The path is not included in the changeset +- This path alone does not cause a pending or failure status + +### 2.6 Main Owns — screenshot added on main since branching + +**Given** a path exists in the HEAD manifest but not in the PR manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +- The path is treated as Main Owns (main added it, PR didn't touch it) +- No diff is generated, no failure or pending status is set for this path alone + +### 2.7 Conflict — all three hashes differ + +**Given** for a given path the PR hash, HEAD hash, and ancestor hash are all different +**When** the 3-way comparison runs for that path +**Then** the path is collected as a conflict + +### 2.8 Outcome: any conflict → failure + +**Given** at least one path was collected as a conflict +**When** the outcome is determined +**Then**: + +- A failure commit status is set on the PR head SHA +- A comment is posted listing the conflicting paths with a rebase instruction +- No changeset is written + +### 2.10 Outcome: all Main Owns (no PR Owns, no conflicts) → success + +**Given** all differing hashes are Main Owns (no PR Owns, no conflicts) +**When** the outcome is determined +**Then** a success commit status is set on the PR head SHA + +### 2.11 Outcome: at least one PR Owns, no conflicts → pending + +**Given** at least one path is PR Owns and no paths are conflicts +**When** the outcome is determined +**Then**: + +- A pending commit status is set on the PR head SHA +- A Comparadise comment is posted +- A changeset is written to `changesets/{pr-head-sha}.json` + +### 2.12 Changeset schema + +**Given** a changeset is written +**Then**: + +- The file is at `changesets/{pr-head-sha}.json` +- It contains `_headSha`: the HEAD SHA resolved in step 2 of the compare flow +- It contains one entry per PR-owned changed path with the PR's new hash as the value +- It contains one entry per PR-deleted path with `null` as the value +- Main Owns paths are not included +- The changeset contains only paths where the PR introduced a change + +### 2.13 Missing ancestor manifest + +**Given** the ancestor SHA is resolved but `manifests/{ancestor-sha}.json` does not exist in S3 +**When** the compare runs +**Then**: + +- The action fails +- An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest + +### 2.14 HEAD SHA resolution + +**Given** the compare mode runs +**When** the HEAD SHA is resolved +**Then** the GitHub API is called at `GET /repos/{owner}/{repo}/branches/{base.ref}` to get the latest main HEAD SHA (not a stale cached value) + +### 2.15 Ancestor SHA resolution + +**Given** the HEAD SHA and PR SHA are known +**When** the ancestor SHA is resolved +**Then** the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA + +--- + +## 3. `manifest-merge` mode + +### 3.1 Manifest always written for merge commit + +**Given** the `workflow` input is `manifest-merge` +**When** the action runs +**Then** a manifest is always written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists + +### 3.2 No changeset — copy parent manifest + +**Given** no changeset exists at `changesets/{pr-head-sha}.json` +**When** the action runs +**Then**: + +- The parent manifest (`manifests/{parent-sha}.json`) is copied as-is to `manifests/{merge-commit-sha}.json` +- No base images are updated +- No failure status is set + +### 3.3 Conflict prevention — overlapping open PR changesets + +**Given** a changeset exists for the merging PR, and at least one other open PR has a changeset that shares at least one screenshot path key with the merging PR's changeset +**When** the merge action runs +**Then**: + +- A failure commit status is set on that other open PR's head SHA +- The failure status message indicates visual comparison is outdated and a rebase is required +- This check is performed for every open PR that has a changeset in S3 + +### 3.4 Stale changeset — no overlapping paths → proceed + +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, but none of the changeset's screenshot keys differ between `manifests/{changeset._headSha}.json` and `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then** the merge proceeds normally (the intervening merges didn't touch the same screenshots) + +### 3.5 Stale changeset — overlapping paths → fail + +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, and at least one changeset key has a different hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then**: + +- The action fails +- The conflicting paths are listed in the failure output + +### 3.6 Overlay — non-null entries update hash + +**Given** a valid (non-stale) changeset exists +**When** the changeset is overlaid onto the parent manifest +**Then** for each non-null entry in the changeset, the corresponding key in the resulting manifest has the changeset's hash value + +### 3.7 Overlay — null entries remove key + +**Given** a valid changeset contains a `null` entry for a path +**When** the changeset is overlaid onto the parent manifest +**Then** that path is absent from `manifests/{merge-commit-sha}.json` + +### 3.8 Base image update — non-null entries + +**Given** a valid changeset is applied +**When** base images are updated +**Then** for each non-null changeset entry, `new-images/{pr-sha}/path/new.png` is copied to `base-images/path/base.png` + +### 3.9 Base image update — null entries delete base image + +**Given** a valid changeset contains a `null` entry for a path +**When** base images are updated +**Then** `base-images/path/base.png` is deleted from S3 + +### 3.10 Parent SHA resolution + +**Given** the merge commit SHA is known +**When** the parent manifest is fetched +**Then** the GitHub API is called to get `parents[0].sha` of the merge commit, and `manifests/{parents[0].sha}.json` is used as the parent manifest + +--- + +## 4. General / Cross-cutting + +### 4.1 New modes do not affect existing modes + +**Given** the `workflow` input is `pr` or `merge` +**When** the action runs +**Then** behavior is identical to before this implementation — no regressions in existing modes + +### 4.2 Manifest key format — monorepo + +**Given** the action is run in a monorepo with a non-empty `package-paths` input +**When** manifest entries are keyed +**Then** each key is prefixed with the package path (e.g. `packages/ui/components/Button`, not just `components/Button`) + +### 4.3 Manifest key format — single package + +**Given** no `package-paths` input is set +**When** manifest entries are keyed +**Then** each key is the screenshot directory's path relative to the screenshots root (no prefix) + +### 4.4 S3 key structure is exact + +**Given** the implementation writes or reads any manifest, changeset, or image +**Then** the following exact S3 key patterns are used: + +- Manifests: `manifests/{commit-sha}.json` +- Changesets: `changesets/{pr-head-sha}.json` +- New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) +- Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) +- `original-new-images/` is never written by any manifest mode + +### 4.5 MD5 hashing implementation + +**Given** a screenshot file is hashed +**Then** Node.js `crypto` is used to compute the MD5 hash of the full-size image file + +### 4.6 Concurrency constraint is documented + +**Given** the implementation is complete +**Then** the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races + +### 4.7 `action/dist/` is rebuilt and committed + +**Given** any file in `action/src/` is changed +**Then** `bunx nx build action` has been run and the updated `action/dist/` files are included in the PR diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md index b3811dee..6d0799cb 100644 --- a/MANIFEST_PLAN.md +++ b/MANIFEST_PLAN.md @@ -65,7 +65,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **All match** → set success status, done - **At least one differs** → proceed to 3-way comparison 5. Resolve ancestor SHA via GitHub Compare API (`GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` → `merge_base_commit.sha`) -6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail with rebase instruction if missing) +6. Fetch ancestor manifest from `manifests/{ancestor-sha}.json` (fail if missing — error message should explain that the ancestor manifest was not found and instruct the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest) 7. For each differing hash, run 3-way comparison (treat missing entries as a distinct state): - **PR Owns (HEAD = ancestor):** PR introduced the diff → download base.png from `base-images/`, download PR's new.png from `new-images/{pr-sha}/path/new.png`; generate diff.png via pixelmatch; upload base.png and diff.png to `new-images/{pr-sha}/path/{base,diff}.png` (resize if enabled) - Special case: new screenshot (not in HEAD or ancestor) → no base.png or diff.png, just new.png @@ -107,7 +107,7 @@ A flat object containing only entries the PR changed. Non-null values are the PR - **Coexistence:** New modes alongside existing `pr`/`merge` — consumers opt in - **Hashing:** MD5 via Node.js `crypto`. Always computed from the full-size image regardless of resize settings -- **Missing ancestor manifest:** Fail with rebase instruction (only during initial adoption) +- **Missing ancestor manifest:** Fail with an error explaining the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest (most likely during initial adoption) - **Staleness handling:** Changeset overlay at merge time ensures concurrent merges are handled correctly - **Merge concurrency:** Consumers **must** set a `concurrency` group (with `cancel-in-progress: false`) on their `manifest-merge` workflow to serialize merge jobs. Without it, two simultaneous merges can both update `base-images/` at the same time, producing a corrupted or interleaved state that `manifest-compare` jobs running in parallel will read. The concrete race: PR A and PR B merge within seconds of each other; both `manifest-merge` jobs start concurrently, each overwriting overlapping `base-images/` keys; a `manifest-compare` job for an open PR C reads `base-images/` mid-update and generates a diff against a partially-applied base, producing a wrong or misleading visual result. Serializing merges via `concurrency` eliminates this window entirely. - **Stale changeset detection:** The changeset stores `_headSha` (the HEAD SHA at compare time). From cfdbb72de43143f1d608f150bc4395b3f506c7b6 Mon Sep 17 00:00:00 2001 From: dadajian Date: Fri, 12 Jun 2026 11:26:18 -0500 Subject: [PATCH 09/20] review #1 --- MANIFEST_AC_REVIEW_1.md | 307 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 307 insertions(+) create mode 100644 MANIFEST_AC_REVIEW_1.md diff --git a/MANIFEST_AC_REVIEW_1.md b/MANIFEST_AC_REVIEW_1.md new file mode 100644 index 00000000..108b25ad --- /dev/null +++ b/MANIFEST_AC_REVIEW_1.md @@ -0,0 +1,307 @@ +# Manifest-Based Visual Comparison — Acceptance Criteria (Review #1) + +This is `MANIFEST_AC.md` annotated with implementation status for PR #786. Source-only review of `action/src/`; local `HEAD` (`022458d`) matches the PR head SHA. Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (partial / unverifiable), with a reason for anything not satisfied. + +--- + +## 1. `manifest-generate` mode + +### 1.1 Normal run — differential upload + +**Given** the `workflow` input is `manifest-generate`, a HEAD manifest exists at `manifests/{head-sha}.json` in S3, and `visual-test-command` completes successfully +**When** the action runs +**Then**: + +- ✅ The MD5 hash of each `new.png` in the screenshots directory is computed using Node.js `crypto` from the full-size image on disk +- ❌ Each entry is keyed by the screenshot directory's path relative to the screenshots root (prefixed with package path for monorepos) — _`manifest-generate.ts:38-43` never reads `package-paths`; keys get no package prefix (see 4.2)_ +- ✅ Only images whose hash differs from the HEAD manifest are uploaded to `new-images/{commit-sha}/path/new.png` +- ✅ If resize is enabled: changed images are resized before upload +- ✅ If resize is not enabled: changed images are uploaded full-size +- ✅ Images whose hash matches the HEAD manifest are not uploaded +- ❌ Nothing is ever written to `original-new-images/` — _`manifest-generate.ts:70-74` writes `original-new-images/{commit-sha}/{key}/new.png` when resize is enabled; asserted by `manifest-generate.test.ts:206-237`_ +- ✅ A manifest is written to `manifests/{commit-sha}.json` mapping every screenshot directory path to its MD5 hash (all screenshots, not just changed ones) + +### 1.2 First run — no HEAD manifest exists + +**Given** the `workflow` input is `manifest-generate` and no manifest exists at `manifests/{head-sha}.json` in S3 +**When** the action runs +**Then**: + +- ✅ The missing manifest is treated as an empty object (no hash comparisons are performed) — _`fetchHeadManifest` returns `null` on `NoSuchKey`; filter falls back to "all"_ +- ✅ All images are uploaded to `new-images/{commit-sha}/path/new.png` following the same resize rules as 1.1 +- ✅ A manifest is written to `manifests/{commit-sha}.json` for all screenshots + +### 1.3 Hashing is always from full-size image + +**Given** any run of `manifest-generate` +**When** hashes are computed +**Then** ✅ each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied — _`manifest-generate.ts:41` hashes the on-disk file; resize only touches the upload buffer_ + +--- + +## 2. `manifest-compare` mode + +### 2.1 All hashes match — success + +**Given** the `workflow` input is `manifest-compare`, the PR manifest at `manifests/{pr-head-sha}.json` and the HEAD manifest at `manifests/{head-sha}.json` both exist, and every hash in the PR manifest matches the corresponding hash in the HEAD manifest +**When** the action runs +**Then**: + +- ✅ A success commit status is set on the PR head SHA +- ✅ No changeset is written +- ✅ No Comparadise comment is posted + +_`manifest-compare.ts:56-65`_ + +### 2.2 PR Owns — PR introduced a visual diff (normal case) + +**Given** at least one hash differs between the PR manifest and the HEAD manifest, and for a given differing path the HEAD hash equals the ancestor hash (PR introduced the change) +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ `base.png` is downloaded from `base-images/path/base.png` +- ✅ `new.png` is downloaded from `new-images/{pr-sha}/path/new.png` (as uploaded by `manifest-generate`) +- ✅ A `diff.png` is generated via pixelmatch +- ✅ `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload — _`manifest-diff.ts:30-53`; resize satisfied transitively since both source images are already resized in S3_ + +### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) + +**Given** a path exists in the PR manifest but does not exist in either the HEAD manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ No `base.png` is downloaded +- ✅ No `diff.png` is generated or uploaded +- ✅ Only `new.png` is referenced (already uploaded by `manifest-generate`) +- ✅ The path is treated as a PR-owned change (contributes to pending status and Comparadise comment) + +_`manifest-compare-classify.ts:75-76` (`added`); `manifest-diff.ts:23` only diffs `changed`_ + +### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) + +**Given** a path exists in the ancestor manifest and the HEAD manifest with the same hash, but does not exist in the PR manifest +**When** the 3-way comparison runs for that path +**Then**: + +- ❌ The deletion is logged as an informational message — _no `core.info` is emitted for deletions in `classify`_ +- ✅ The path is recorded as a `null` entry in the changeset — _`buildChangeset` (`manifest-compare.ts:145-146`)_ +- ❌ The path does not contribute to pending status or Comparadise comment — _deletions go into the same `prOwns` array as reviewable changes (`manifest-compare-classify.ts:79-81`), so a delete-only PR fails the `prOwns.length === 0` gate (`manifest-compare.ts:72`) and is forced to **pending**, and the comment renders `Deleted screenshots: N` (`run.ts:433-441`). The code conflates "PR introduced this change" (correct for the changeset) with "this needs to gate/be shown."_ + +### 2.5 Main Owns — main changed, PR is clean + +**Given** for a given differing path the PR hash equals the ancestor hash (main introduced the change, not the PR) +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ No diff is generated for that path +- ✅ The path is not included in the changeset +- ✅ This path alone does not cause a pending or failure status + +_`manifest-compare-classify.ts:82-84`; `buildChangeset` iterates only `prOwns`_ + +### 2.6 Main Owns — screenshot added on main since branching + +**Given** a path exists in the HEAD manifest but not in the PR manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ The path is treated as Main Owns (main added it, PR didn't touch it) — _`prHash === ancestorHash === null` → `mainOwns`_ +- ✅ No diff is generated, no failure or pending status is set for this path alone + +### 2.7 Conflict — all three hashes differ + +**Given** for a given path the PR hash, HEAD hash, and ancestor hash are all different +**When** the 3-way comparison runs for that path +**Then** ✅ the path is collected as a conflict — _`manifest-compare-classify.ts:85-87`_ + +### 2.8 Outcome: any conflict → failure + +**Given** at least one path was collected as a conflict +**When** the outcome is determined +**Then**: + +- ✅ A failure commit status is set on the PR head SHA +- ✅ A comment is posted listing the conflicting paths with a rebase instruction +- ✅ No changeset is written + +_`manifest-compare.ts:67-70, 88-107`_ + +### 2.10 Outcome: all Main Owns (no PR Owns, no conflicts) → success + +**Given** all differing hashes are Main Owns (no PR Owns, no conflicts) +**When** the outcome is determined +**Then** ✅ a success commit status is set on the PR head SHA — _`manifest-compare.ts:72-83`_ + +### 2.11 Outcome: at least one PR Owns, no conflicts → pending + +**Given** at least one path is PR Owns and no paths are conflicts +**When** the outcome is determined +**Then**: + +- ✅ A pending commit status is set on the PR head SHA +- ✅ A Comparadise comment is posted +- ✅ A changeset is written to `changesets/{pr-head-sha}.json` + +_`manifest-compare.ts:109-136`. Note: interacts with the 2.4 defect — a deletion-only PR also lands here instead of being treated as non-contributing._ + +### 2.12 Changeset schema + +**Given** a changeset is written +**Then**: + +- ✅ The file is at `changesets/{pr-head-sha}.json` — _`manifest-s3.ts:39-50`_ +- ✅ It contains `_headSha`: the HEAD SHA resolved in step 2 of the compare flow +- ✅ It contains one entry per PR-owned changed path with the PR's new hash as the value +- ✅ It contains one entry per PR-deleted path with `null` as the value +- ✅ Main Owns paths are not included +- ✅ The changeset contains only paths where the PR introduced a change + +_`buildChangeset` (`manifest-compare.ts:138-158`)_ + +### 2.13 Missing ancestor manifest + +**Given** the ancestor SHA is resolved but `manifests/{ancestor-sha}.json` does not exist in S3 +**When** the compare runs +**Then**: + +- ✅ The action fails +- ⚠️ An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest — _`manifest-compare-classify.ts:143-146` mentions only "rebase your branch"; it omits the "ensure `manifest-generate` ran on the base branch" instruction_ + +### 2.14 HEAD SHA resolution + +**Given** the compare mode runs +**When** the HEAD SHA is resolved +**Then** ✅ the GitHub API is called at `GET /repos/{owner}/{repo}/branches/{base.ref}` to get the latest main HEAD SHA (not a stale cached value) — _`resolveHeadSha` → `repos.getBranch`_ + +### 2.15 Ancestor SHA resolution + +**Given** the HEAD SHA and PR SHA are known +**When** the ancestor SHA is resolved +**Then** ✅ the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA — _`resolveAncestorSha` → `compareCommitsWithBasehead`_ + +--- + +## 3. `manifest-merge` mode + +### 3.1 Manifest always written for merge commit + +**Given** the `workflow` input is `manifest-merge` +**When** the action runs +**Then** ✅ a manifest is always written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists — _`manifest-merge.ts:53` (no changeset) and `:69` (changeset)_ + +### 3.2 No changeset — copy parent manifest + +**Given** no changeset exists at `changesets/{pr-head-sha}.json` +**When** the action runs +**Then**: + +- ✅ The parent manifest (`manifests/{parent-sha}.json`) is copied as-is to `manifests/{merge-commit-sha}.json` +- ✅ No base images are updated +- ✅ No failure status is set + +_`manifest-merge.ts:49-55`_ + +### 3.3 Conflict prevention — overlapping open PR changesets + +**Given** a changeset exists for the merging PR, and at least one other open PR has a changeset that shares at least one screenshot path key with the merging PR's changeset +**When** the merge action runs +**Then**: + +- ✅ A failure commit status is set on that other open PR's head SHA +- ✅ The failure status message indicates visual comparison is outdated and a rebase is required +- ❌ This check is performed for every open PR that has a changeset in S3 — _`manifest-merge-flag-prs.ts:38-41` calls `pulls.list` with no pagination/`per_page` (default 30); open PRs beyond the first page are never inspected_ + +### 3.4 Stale changeset — no overlapping paths → proceed + +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, but none of the changeset's screenshot keys differ between `manifests/{changeset._headSha}.json` and `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then** ✅ the merge proceeds normally (the intervening merges didn't touch the same screenshots) — _`detectStaleConflicts` returns `[]` → `assertNoStaleConflicts` returns early_ + +### 3.5 Stale changeset — overlapping paths → fail + +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, and at least one changeset key has a different hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then**: + +- ✅ The action fails +- ✅ The conflicting paths are listed in the failure output + +_`manifest-merge.ts:73-92`_ + +### 3.6 Overlay — non-null entries update hash + +**Given** a valid (non-stale) changeset exists +**When** the changeset is overlaid onto the parent manifest +**Then** ✅ for each non-null entry in the changeset, the corresponding key in the resulting manifest has the changeset's hash value — _`manifest-merge-overlay.ts:21-22`_ + +### 3.7 Overlay — null entries remove key + +**Given** a valid changeset contains a `null` entry for a path +**When** the changeset is overlaid onto the parent manifest +**Then** ✅ that path is absent from `manifests/{merge-commit-sha}.json` — _`manifest-merge-overlay.ts:19-20`_ + +### 3.8 Base image update — non-null entries + +**Given** a valid changeset is applied +**When** base images are updated +**Then** ✅ for each non-null changeset entry, `new-images/{pr-sha}/path/new.png` is copied to `base-images/path/base.png` — _`manifest-merge-base-images.ts:56-66`_ + +### 3.9 Base image update — null entries delete base image + +**Given** a valid changeset contains a `null` entry for a path +**When** base images are updated +**Then** ✅ `base-images/path/base.png` is deleted from S3 — _`manifest-merge-base-images.ts:67-76`_ + +### 3.10 Parent SHA resolution + +**Given** the merge commit SHA is known +**When** the parent manifest is fetched +**Then** ✅ the GitHub API is called to get `parents[0].sha` of the merge commit, and `manifests/{parents[0].sha}.json` is used as the parent manifest — _`run.ts:355-366`_ + +--- + +## 4. General / Cross-cutting + +### 4.1 New modes do not affect existing modes + +**Given** the `workflow` input is `pr` or `merge` +**When** the action runs +**Then** ✅ behavior is identical to before this implementation — no regressions in existing modes — _`run.ts:37-50` dispatches new modes and returns early; legacy code path unchanged_ + +### 4.2 Manifest key format — monorepo + +**Given** the action is run in a monorepo with a non-empty `package-paths` input +**When** manifest entries are keyed +**Then** ❌ each key is prefixed with the package path (e.g. `packages/ui/components/Button`, not just `components/Button`) — _`manifest-generate.ts:38-43` derives keys solely from the path relative to `screenshots-directory` and ignores `package-paths`. Additionally, the documented matrix flow (`docs/.../manifest-workflows.md:89-129`) has each per-package job write the same `manifests/{commit-sha}.json`, so jobs overwrite one another_ + +### 4.3 Manifest key format — single package + +**Given** no `package-paths` input is set +**When** manifest entries are keyed +**Then** ✅ each key is the screenshot directory's path relative to the screenshots root (no prefix) — _`manifest-generate.ts:39-40`_ + +### 4.4 S3 key structure is exact + +**Given** the implementation writes or reads any manifest, changeset, or image +**Then** the following exact S3 key patterns are used: + +- ✅ Manifests: `manifests/{commit-sha}.json` +- ✅ Changesets: `changesets/{pr-head-sha}.json` +- ✅ New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) +- ✅ Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) +- ❌ `original-new-images/` is never written by any manifest mode — _violated by `manifest-generate.ts:70-74` (see 1.1)_ + +### 4.5 MD5 hashing implementation + +**Given** a screenshot file is hashed +**Then** ✅ Node.js `crypto` is used to compute the MD5 hash of the full-size image file — _`hash.ts:1-7`_ + +### 4.6 Concurrency constraint is documented + +**Given** the implementation is complete +**Then** ✅ the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races — _`docs/docs/setup/manifest-workflows.md:135,145-147`_ + +### 4.7 `action/dist/` is rebuilt and committed + +**Given** any file in `action/src/` is changed +**Then** ✅`bunx nx build action` has been run and the updated `action/dist/` files are included in the PR From d58951b7881ab3c6c133c2461ed4ff25855b4c34 Mon Sep 17 00:00:00 2001 From: dadajian Date: Tue, 23 Jun 2026 15:52:46 -0500 Subject: [PATCH 10/20] docs(manifest-plan): add monorepo per-package manifest path AC Co-Authored-By: Claude Sonnet 4.6 --- MANIFEST_AC.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md index 76f33a61..5242902c 100644 --- a/MANIFEST_AC.md +++ b/MANIFEST_AC.md @@ -37,6 +37,12 @@ This document is the authoritative acceptance criteria for the `manifest-generat **When** hashes are computed **Then** each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied +### 1.4 Monorepo — per-package manifest path + +**Given** the `workflow` input is `manifest-generate` and `package-paths` is non-empty +**When** the manifest is written to S3 +**Then** the manifest is uploaded to `manifests/{commit-sha}/{package-path}.json` (one file per package invocation) instead of `manifests/{commit-sha}.json` + --- ## 2. `manifest-compare` mode @@ -167,6 +173,16 @@ This document is the authoritative acceptance criteria for the `manifest-generat **When** the ancestor SHA is resolved **Then** the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA +### 2.16 Monorepo — squash per-package manifests before comparison + +**Given** the `workflow` input is `manifest-compare` and `package-paths` is non-empty +**When** the action resolves the PR manifest +**Then**: + +- All per-package manifests at `manifests/{pr-sha}/{package-path}.json` are downloaded and merged into a single manifest +- The squashed manifest is uploaded to `manifests/{pr-sha}.json` +- The squashed manifest is used alongside the pre-existing `manifests/{head-sha}.json` and `manifests/{ancestor-sha}.json` for the 3-way comparison + --- ## 3. `manifest-merge` mode @@ -269,7 +285,8 @@ This document is the authoritative acceptance criteria for the `manifest-generat **Given** the implementation writes or reads any manifest, changeset, or image **Then** the following exact S3 key patterns are used: -- Manifests: `manifests/{commit-sha}.json` +- Manifests (single-package, or squashed by compare for monorepo): `manifests/{commit-sha}.json` +- Manifests (monorepo, written by generate per package): `manifests/{commit-sha}/{package-path}.json` - Changesets: `changesets/{pr-head-sha}.json` - New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) - Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) From 8056c5e8da670be294bb8dcf9a8a4764adcf4a30 Mon Sep 17 00:00:00 2001 From: dadajian Date: Tue, 23 Jun 2026 15:55:15 -0500 Subject: [PATCH 11/20] update review #1 --- MANIFEST_AC_REVIEW_1.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/MANIFEST_AC_REVIEW_1.md b/MANIFEST_AC_REVIEW_1.md index 108b25ad..b9a78407 100644 --- a/MANIFEST_AC_REVIEW_1.md +++ b/MANIFEST_AC_REVIEW_1.md @@ -37,6 +37,12 @@ This is `MANIFEST_AC.md` annotated with implementation status for PR #786. Sourc **When** hashes are computed **Then** ✅ each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied — _`manifest-generate.ts:41` hashes the on-disk file; resize only touches the upload buffer_ +### 1.4 Monorepo — per-package manifest path + +**Given** the `workflow` input is `manifest-generate` and `package-paths` is non-empty +**When** the manifest is written to S3 +**Then** ❌ the manifest is uploaded to `manifests/{commit-sha}/{package-path}.json` (one file per package invocation) instead of `manifests/{commit-sha}.json` — _not implemented_ + --- ## 2. `manifest-compare` mode @@ -179,6 +185,16 @@ _`buildChangeset` (`manifest-compare.ts:138-158`)_ **When** the ancestor SHA is resolved **Then** ✅ the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA — _`resolveAncestorSha` → `compareCommitsWithBasehead`_ +### 2.16 Monorepo — squash per-package manifests before comparison + +**Given** the `workflow` input is `manifest-compare` and `package-paths` is non-empty +**When** the action resolves the PR manifest +**Then**: + +- ❌ All per-package manifests at `manifests/{pr-sha}/{package-path}.json` are downloaded and merged into a single manifest — _not implemented_ +- ❌ The squashed manifest is uploaded to `manifests/{pr-sha}.json` — _not implemented_ +- ❌ The squashed manifest is used alongside the pre-existing `manifests/{head-sha}.json` and `manifests/{ancestor-sha}.json` for the 3-way comparison — _not implemented_ + --- ## 3. `manifest-merge` mode @@ -285,7 +301,8 @@ _`manifest-merge.ts:73-92`_ **Given** the implementation writes or reads any manifest, changeset, or image **Then** the following exact S3 key patterns are used: -- ✅ Manifests: `manifests/{commit-sha}.json` +- ✅ Manifests (single-package, or squashed by compare for monorepo): `manifests/{commit-sha}.json` +- ❌ Manifests (monorepo, written by generate per package): `manifests/{commit-sha}/{package-path}.json` — _not implemented_ - ✅ Changesets: `changesets/{pr-head-sha}.json` - ✅ New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) - ✅ Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) From 2da625f69e9bc83635f547d6177a427d0dcdca86 Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 11:35:08 -0500 Subject: [PATCH 12/20] Add MANIFEST_AC_REVIEW_2.md for PR #786 Source-level re-review against MANIFEST_AC.md at PR head 02869c8. All 41 criteria satisfied; Review #1 defects resolved; 175 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC_REVIEW_2.md | 204 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 MANIFEST_AC_REVIEW_2.md diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md new file mode 100644 index 00000000..b6f87e1f --- /dev/null +++ b/MANIFEST_AC_REVIEW_2.md @@ -0,0 +1,204 @@ +# Manifest-Based Visual Comparison — Acceptance Criteria (Review #2) + +This is `MANIFEST_AC.md` annotated with implementation status for PR [#786](https://github.com/ExpediaGroup/comparadise/pull/786). + +Source-only review of `action/src/` at the PR head SHA `02869c8`. (Local review HEAD was one commit ahead, `e8a6575`, whose only difference is an edit to `MANIFEST_AC.md` itself — the reviewed `action/src/` is byte-identical to the PR head.) The full test suite passes: **175 pass / 0 fail across 14 files** (`bunx nx test action`). + +Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisfied with a caveat worth noting), with a reason for anything not plainly satisfied. + +**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 41 criteria are satisfied. Three minor caveats are noted (2.2, 2.13, 3.1) — none are behavioral failures. `dist/` diffs were excluded from this review at the reviewer's direction; the committed `dist/main.js` does contain all new manifest functions. + +--- + +## 1. `manifest-generate` mode + +### 1.1 Normal run — differential upload + +- ✅ MD5 of each `new.png` via Node `crypto` from the full-size file on disk — `hash.ts` reads the file and `createHash('md5')`; called at `manifest-generate.ts:56` before any resize. +- ✅ Each entry keyed by path relative to the screenshots root, prefixed with package path for monorepos — `manifest-generate.ts:53-55` (`manifestKey = packagePath ? \`${packagePath}/${localKey}\` : localKey`). _(Review #1 ❌ → now fixed.)_ +- ✅ Only changed images uploaded to `new-images/{commit-sha}/path/new.png` — `changedEntries` filter (`:65-67`), key at `:80`. +- ✅ Resize enabled → resized before upload — `:75-77`. +- ✅ Resize not enabled → full-size — `:75-77` (`resizeEnabled` false path passes the raw buffer). +- ✅ Matching-hash images not uploaded — filter excludes them. +- ✅ Nothing written to `original-new-images/` — no such write exists in `manifest-generate.ts`. _(Review #1 ❌ → now fixed.)_ +- ✅ Manifest written to `manifests/{commit-sha}.json` mapping **all** screenshots — `manifest` built from every entry (`:57`), written at `:89-94`. + +### 1.2 First run — no HEAD manifest exists + +- ✅ Missing manifest treated as empty — `fetchHeadManifest` returns `null` on `NoSuchKey` (`:113-118`); also `null` when `head-sha` not supplied (`:61-63`). Filter `!headManifest || ...` ⇒ upload all. +- ✅ All images uploaded following 1.1 resize rules. +- ✅ Manifest written for all screenshots. + +### 1.3 Hashing always from full-size image + +- ✅ Hash taken at `:56` from the on-disk file; resize only ever touches the upload buffer (`:75`), never the hash input. + +### 1.4 Monorepo — per-package manifest path + +- ✅ When `packagePath` is set, manifest written to `manifests/{commit-sha}/{package-path}.json` (`:86-88`); falls back to `manifests/{commit-sha}.json` otherwise. A `package-paths` value with >1 entry is rejected with a clear `setFailed` (`:22-28`), enforcing one package per matrix job. _(Review #1 ❌ → now fixed.)_ + +--- + +## 2. `manifest-compare` mode + +### 2.1 All hashes match — success + +- ✅ `classifyManifests` returns `{ outcome: 'match' }` when `differingPaths` is empty (`manifest-compare-classify.ts:49-55`); `manifestCompare` sets a success status and returns (`manifest-compare.ts:62-71`) — no changeset, no comment. + +### 2.2 PR Owns — PR introduced a visual diff (changed) + +- ✅ Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are non-null (`classify:73-81`). +- ✅ `base.png` downloaded from `base-images/path/base.png` (`manifest-diff.ts:31,34`). +- ✅ `new.png` downloaded from `new-images/{pr-sha}/path/new.png` (`:32`). +- ✅ `diff.png` generated via pixelmatch (`diff-png.ts`, `manifest-diff.ts:39`). +- ⚠️ `base.png` and `diff.png` uploaded to `new-images/{pr-sha}/path/{base,diff}.png` (`:41-52`). **Caveat:** they are uploaded as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest — `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it just achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing. + +### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) + +- ✅ Classified `added` (`headHash === ancestorHash === null`, `classify:75-76`). +- ✅ No `base.png` download / no `diff.png` — `generateDiffs` processes only `type === 'changed'` (`manifest-diff.ts:23`). +- ✅ Only `new.png` referenced (already uploaded by generate). +- ✅ Counts toward pending status and comment — `reviewable` includes `added` (`manifest-compare.ts:122`); comment reports `addedCount` (`run.ts:437`). + +### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) + +- ✅ Classified `deleted` (`prHash === null`, ancestor non-null, `classify:77-78`). +- ✅ Deletion logged as info — `manifest-compare.ts:125-127`. +- ✅ Recorded as `null` in the changeset — `buildChangeset:170-171`. +- ✅ Does not contribute to pending/comment — excluded from `reviewable`; a deletions-only PR yields a **success** status and no comment (`:133-144`). + +### 2.5 Main Owns — main changed, PR clean + +- ✅ `prHash === ancestorHash` ⇒ `mainOwns` (`classify:82-84`). No diff, not in changeset (`buildChangeset` iterates only `prOwns`), no pending/failure from this path alone. + +### 2.6 Main Owns — screenshot added on main since branching + +- ✅ HEAD has it, PR/ancestor don't ⇒ `prHash === ancestorHash === null` ⇒ `mainOwns` (`classify:82-84`). + +### 2.7 Conflict — all three hashes differ + +- ✅ Reaches the `else` (conflict) branch only when `headHash !== ancestorHash` **and** `prHash !== ancestorHash` (`classify:85-87`). Because `differingPaths` is pre-filtered to `prManifest[p] !== headManifest[p]` (`:49-51`), `prHash !== headHash` is also guaranteed here — so the conflict branch correctly fires only when all three differ, and never when PR and main coincidentally made the same change. + +### 2.8 Outcome: any conflict → failure + +- ✅ `handleConflicts` sets `setFailed`, a failure commit status, and posts a conflict comment (`manifest-compare.ts:94-113`). No changeset is written (returns before `putChangeset`). Comment lists conflicting paths with a rebase instruction (`run.ts:430-431`). + +### 2.10 Outcome: all Main Owns → success + +- ✅ `prOwns.length === 0` ⇒ success status (`manifest-compare.ts:78-89`). + +### 2.11 Outcome: ≥1 PR Owns, no conflicts → pending + +- ✅ `handlePrOwns` writes the changeset (`:131`), sets pending status with `target_url` (`:148-154`), and posts the diffs comment (`:156-160`). _(Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields success + no comment, which is the documented deletions behavior, not a regression.)_ + +### 2.12 Changeset schema + +- ✅ Written to `changesets/{pr-head-sha}.json` (`manifest-s3.ts:39-50`, `sha = prSha`). +- ✅ `_headSha` = the HEAD SHA resolved in compare step 2 (`result.headSha`, `buildChangeset:168`). +- ✅ One entry per PR-owned changed/added path with the PR's new hash (`buildChangeset:172-180`). +- ✅ One `null` entry per PR-deleted path (`:170-171`). +- ✅ Main Owns excluded (loop iterates only `prOwns`). +- ✅ Contains only PR-introduced paths. Defensive guard throws if a non-deleted entry is missing its PR hash (`:174-178`). + +### 2.13 Missing ancestor manifest + +- ⚠️ `requireAncestorManifest` throws with the exact required guidance ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest", `classify:137-149`). The action does fail and the message surfaces. **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation. + +### 2.14 HEAD SHA resolution + +- ✅ `octokit.rest.repos.getBranch({ ...repo, branch: baseRef })` → `data.commit.sha` (`classify:151-161`) — the live branches endpoint, not a cached value. + +### 2.15 Ancestor SHA resolution + +- ✅ `octokit.rest.repos.compareCommitsWithBasehead({ basehead: \`${headSha}...${prSha}\` })`→`merge_base_commit.sha` (`classify:163-174`). + +### 2.16 Monorepo — squash per-package manifests before comparison + +- ✅ `squashPrManifest` lists `manifests/{pr-sha}/`, merges all parts, and uploads the combined `manifests/{pr-sha}.json` (`manifest-s3.ts:80-109`); duplicate keys across packages throw. No-op (returns `null`, writes nothing) for single-package PRs. `classify` then reads the squashed `manifests/{pr-sha}.json` alongside HEAD and ancestor manifests. + +--- + +## 3. `manifest-merge` mode + +### 3.1 Manifest always written for merge commit + +- ⚠️ Written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists." + +### 3.2 No changeset — copy parent manifest + +- ✅ `parentManifest` copied verbatim to `manifests/{merge-commit-sha}.json` (`:49-55`); returns before any base-image update or status change. + +### 3.3 Conflict prevention — overlapping open PR changesets + +- ✅ `flagOverlappingOpenPrs` paginates **all** open PRs, loads each one's changeset, and on any shared screenshot path sets a `failure` commit status on that PR's head SHA with "Visual comparison outdated — please rebase." (`manifest-merge-flag-prs.ts:38-69`). Skips the merging PR itself (`:46`). + +### 3.4 Stale changeset — no overlapping paths → proceed + +- ✅ When `_headSha !== parentSha`, `detectStaleConflicts` compares `manifests/{_headSha}` vs `manifests/{parent}` on changeset keys; empty result ⇒ returns and the merge proceeds (`manifest-merge.ts:64-66`, `:88`). + +### 3.5 Stale changeset — overlapping paths → fail + +- ✅ Non-empty conflicts ⇒ `core.setFailed` with the conflicting paths listed, then throw (`manifest-merge.ts:88-92`). + +### 3.6 Overlay — non-null entries update hash + +- ✅ `overlayChangeset` sets `result[path] = hash` for non-null entries (`manifest-merge-overlay.ts:17-24`); parent manifest not mutated (spread copy). + +### 3.7 Overlay — null entries remove key + +- ✅ `delete result[path]` for null entries (`:19-21`). + +### 3.8 Base image update — non-null entries + +- ✅ `applyChangesetToBaseImages` copies `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png` (`manifest-merge-base-images.ts:55-66`), with a properly URI-encoded `CopySource`. + +### 3.9 Base image update — null entries delete base image + +- ✅ `deleteObjects` removes `base-images/path/base.png` for null entries (`:67-77`). + +### 3.10 Parent SHA resolution + +- ✅ `octokit.rest.repos.getCommit({ ref: mergeSha })` → `data.parents[0].sha`, used as the parent manifest key (`run.ts:356-368`); throws if no parent exists. + +--- + +## 4. General / Cross-cutting + +### 4.1 New modes do not affect existing modes + +- ✅ `run.ts:37-50` dispatches the three manifest modes and returns early; the `pr`/`merge` code paths below are untouched. Covered by `run.test.ts`. + +### 4.2 Manifest key format — monorepo + +- ✅ Keys prefixed with the package path (`manifest-generate.ts:55`). + +### 4.3 Manifest key format — single package + +- ✅ No prefix when `package-paths` unset (`packagePath = ''` ⇒ `manifestKey = localKey`). + +### 4.4 S3 key structure is exact + +- ✅ Manifests (single/squashed): `manifests/{sha}.json` (`manifest-s3.ts`, `manifest-generate.ts:88`). +- ✅ Manifests (monorepo generate): `manifests/{sha}/{package-path}.json` (`manifest-generate.ts:86-87`). +- ✅ Changesets: `changesets/{sha}.json` (`manifest-s3.ts:43`). +- ✅ New images: `new-images/{sha}/path/new.png`, resized when enabled (`manifest-generate.ts:80`). +- ✅ Base images: `base-images/path/base.png`, populated by copy from already-resized `new-images/` (`manifest-merge-base-images.ts:63`). +- ✅ `original-new-images/` never written by any manifest mode (grep-confirmed; constant `ORIGINAL_NEW_IMAGES_DIRECTORY` is unused outside legacy `pr` mode). + +### 4.5 MD5 hashing implementation + +- ✅ `hash.ts` uses Node `crypto` `createHash('md5')` over the full-size file buffer. + +### 4.6 Concurrency constraint is documented + +- ✅ `docs/docs/setup/manifest-workflows.md:137-149` states the `manifest-merge` workflow must set a `concurrency` group with `cancel-in-progress: false`, with the rationale, and the example YAML includes it. + +### 4.7 `action/dist/` is rebuilt and committed + +- ✅ `action/dist/main.js` and `action/dist/main.js.map` are part of the PR and contain all new manifest functions (`manifestGenerate`, `manifestCompare`, `manifestMerge`, `classifyManifests`, `overlayChangeset`, `detectStaleConflicts`, `flagOverlappingOpenPrs`, `hashFile`). _(`dist/` content diffs excluded from this review at the reviewer's direction; a local rebuild differs only in absolute `__dirname` paths and `node_modules` hoisting — pure build-environment noise, no source-level change.)_ + +--- + +## Verdict + +All 41 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. The three ⚠️ caveats (2.2 implicit resize, 2.13 thrown error vs `core.setFailed`, 3.1 no manifest on the stale-conflict failure path) are correctness-neutral and optional to address; none block acceptance. From 673a5ad3468245714d4a3b91d0289c8ab5f0ae4d Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 11:36:26 -0500 Subject: [PATCH 13/20] Remove AC 4.7 (dist rebuild) from manifest AC and Review #2 Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC.md | 5 ----- MANIFEST_AC_REVIEW_2.md | 8 ++------ 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md index 5242902c..36e0deb1 100644 --- a/MANIFEST_AC.md +++ b/MANIFEST_AC.md @@ -301,8 +301,3 @@ This document is the authoritative acceptance criteria for the `manifest-generat **Given** the implementation is complete **Then** the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races - -### 4.7 `action/dist/` is rebuilt and committed - -**Given** any file in `action/src/` is changed -**Then** `bunx nx build action` has been run and the updated `action/dist/` files are included in the PR diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md index b6f87e1f..e7537d1a 100644 --- a/MANIFEST_AC_REVIEW_2.md +++ b/MANIFEST_AC_REVIEW_2.md @@ -6,7 +6,7 @@ Source-only review of `action/src/` at the PR head SHA `02869c8`. (Local review Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisfied with a caveat worth noting), with a reason for anything not plainly satisfied. -**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 41 criteria are satisfied. Three minor caveats are noted (2.2, 2.13, 3.1) — none are behavioral failures. `dist/` diffs were excluded from this review at the reviewer's direction; the committed `dist/main.js` does contain all new manifest functions. +**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 40 criteria are satisfied. Three minor caveats are noted (2.2, 2.13, 3.1) — none are behavioral failures. --- @@ -193,12 +193,8 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf - ✅ `docs/docs/setup/manifest-workflows.md:137-149` states the `manifest-merge` workflow must set a `concurrency` group with `cancel-in-progress: false`, with the rationale, and the example YAML includes it. -### 4.7 `action/dist/` is rebuilt and committed - -- ✅ `action/dist/main.js` and `action/dist/main.js.map` are part of the PR and contain all new manifest functions (`manifestGenerate`, `manifestCompare`, `manifestMerge`, `classifyManifests`, `overlayChangeset`, `detectStaleConflicts`, `flagOverlappingOpenPrs`, `hashFile`). _(`dist/` content diffs excluded from this review at the reviewer's direction; a local rebuild differs only in absolute `__dirname` paths and `node_modules` hoisting — pure build-environment noise, no source-level change.)_ - --- ## Verdict -All 41 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. The three ⚠️ caveats (2.2 implicit resize, 2.13 thrown error vs `core.setFailed`, 3.1 no manifest on the stale-conflict failure path) are correctness-neutral and optional to address; none block acceptance. +All 40 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. The three ⚠️ caveats (2.2 implicit resize, 2.13 thrown error vs `core.setFailed`, 3.1 no manifest on the stale-conflict failure path) are correctness-neutral and optional to address; none block acceptance. From 673e0848b48893848e2ef3178e2c680bb9735b7d Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 11:37:07 -0500 Subject: [PATCH 14/20] Remove AC 4.7 (dist rebuild) from Review #1 Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC_REVIEW_1.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/MANIFEST_AC_REVIEW_1.md b/MANIFEST_AC_REVIEW_1.md index b9a78407..67b7054b 100644 --- a/MANIFEST_AC_REVIEW_1.md +++ b/MANIFEST_AC_REVIEW_1.md @@ -317,8 +317,3 @@ _`manifest-merge.ts:73-92`_ **Given** the implementation is complete **Then** ✅ the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races — _`docs/docs/setup/manifest-workflows.md:135,145-147`_ - -### 4.7 `action/dist/` is rebuilt and committed - -**Given** any file in `action/src/` is changed -**Then** ✅`bunx nx build action` has been run and the updated `action/dist/` files are included in the PR From 790046c39093b10e3f16ac2917183deef6cb18fb Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 11:42:27 -0500 Subject: [PATCH 15/20] Make all citations explicit in manifest review docs Replace bare line refs (`:NN`) and pseudo-file refs (`classify:NN`, `buildChangeset:NN`) with explicit `.ts:line` citations, and pair every prose function name with its source filename. Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC_REVIEW_1.md | 6 ++--- MANIFEST_AC_REVIEW_2.md | 58 ++++++++++++++++++++--------------------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/MANIFEST_AC_REVIEW_1.md b/MANIFEST_AC_REVIEW_1.md index 67b7054b..38b90556 100644 --- a/MANIFEST_AC_REVIEW_1.md +++ b/MANIFEST_AC_REVIEW_1.md @@ -27,7 +27,7 @@ This is `MANIFEST_AC.md` annotated with implementation status for PR #786. Sourc **When** the action runs **Then**: -- ✅ The missing manifest is treated as an empty object (no hash comparisons are performed) — _`fetchHeadManifest` returns `null` on `NoSuchKey`; filter falls back to "all"_ +- ✅ The missing manifest is treated as an empty object (no hash comparisons are performed) — _`fetchHeadManifest` (`manifest-generate.ts`) returns `null` on `NoSuchKey`; filter falls back to "all"_ - ✅ All images are uploaded to `new-images/{commit-sha}/path/new.png` following the same resize rules as 1.1 - ✅ A manifest is written to `manifests/{commit-sha}.json` for all screenshots @@ -89,7 +89,7 @@ _`manifest-compare-classify.ts:75-76` (`added`); `manifest-diff.ts:23` only diff **When** the 3-way comparison runs for that path **Then**: -- ❌ The deletion is logged as an informational message — _no `core.info` is emitted for deletions in `classify`_ +- ❌ The deletion is logged as an informational message — _no `core.info` is emitted for deletions in `manifest-compare-classify.ts`_ - ✅ The path is recorded as a `null` entry in the changeset — _`buildChangeset` (`manifest-compare.ts:145-146`)_ - ❌ The path does not contribute to pending status or Comparadise comment — _deletions go into the same `prOwns` array as reviewable changes (`manifest-compare-classify.ts:79-81`), so a delete-only PR fails the `prOwns.length === 0` gate (`manifest-compare.ts:72`) and is forced to **pending**, and the comment renders `Deleted screenshots: N` (`run.ts:433-441`). The code conflates "PR introduced this change" (correct for the changeset) with "this needs to gate/be shown."_ @@ -103,7 +103,7 @@ _`manifest-compare-classify.ts:75-76` (`added`); `manifest-diff.ts:23` only diff - ✅ The path is not included in the changeset - ✅ This path alone does not cause a pending or failure status -_`manifest-compare-classify.ts:82-84`; `buildChangeset` iterates only `prOwns`_ +_`manifest-compare-classify.ts:82-84`; `buildChangeset` (`manifest-compare.ts`) iterates only `prOwns`_ ### 2.6 Main Owns — screenshot added on main since branching diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md index e7537d1a..9f393857 100644 --- a/MANIFEST_AC_REVIEW_2.md +++ b/MANIFEST_AC_REVIEW_2.md @@ -16,26 +16,26 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf - ✅ MD5 of each `new.png` via Node `crypto` from the full-size file on disk — `hash.ts` reads the file and `createHash('md5')`; called at `manifest-generate.ts:56` before any resize. - ✅ Each entry keyed by path relative to the screenshots root, prefixed with package path for monorepos — `manifest-generate.ts:53-55` (`manifestKey = packagePath ? \`${packagePath}/${localKey}\` : localKey`). _(Review #1 ❌ → now fixed.)_ -- ✅ Only changed images uploaded to `new-images/{commit-sha}/path/new.png` — `changedEntries` filter (`:65-67`), key at `:80`. +- ✅ Only changed images uploaded to `new-images/{commit-sha}/path/new.png` — `changedEntries` filter (`manifest-generate.ts:65-67`), key at `manifest-generate.ts:80`. - ✅ Resize enabled → resized before upload — `:75-77`. - ✅ Resize not enabled → full-size — `:75-77` (`resizeEnabled` false path passes the raw buffer). - ✅ Matching-hash images not uploaded — filter excludes them. - ✅ Nothing written to `original-new-images/` — no such write exists in `manifest-generate.ts`. _(Review #1 ❌ → now fixed.)_ -- ✅ Manifest written to `manifests/{commit-sha}.json` mapping **all** screenshots — `manifest` built from every entry (`:57`), written at `:89-94`. +- ✅ Manifest written to `manifests/{commit-sha}.json` mapping **all** screenshots — `manifest` built from every entry (`manifest-generate.ts:57`), written at `manifest-generate.ts:89-94`. ### 1.2 First run — no HEAD manifest exists -- ✅ Missing manifest treated as empty — `fetchHeadManifest` returns `null` on `NoSuchKey` (`:113-118`); also `null` when `head-sha` not supplied (`:61-63`). Filter `!headManifest || ...` ⇒ upload all. +- ✅ Missing manifest treated as empty — `fetchHeadManifest` returns `null` on `NoSuchKey` (`manifest-generate.ts:113-118`); also `null` when `head-sha` not supplied (`manifest-generate.ts:61-63`). Filter `!headManifest || ...` ⇒ upload all. - ✅ All images uploaded following 1.1 resize rules. - ✅ Manifest written for all screenshots. ### 1.3 Hashing always from full-size image -- ✅ Hash taken at `:56` from the on-disk file; resize only ever touches the upload buffer (`:75`), never the hash input. +- ✅ Hash taken at `manifest-generate.ts:56` from the on-disk file; resize only ever touches the upload buffer (`manifest-generate.ts:75`), never the hash input. ### 1.4 Monorepo — per-package manifest path -- ✅ When `packagePath` is set, manifest written to `manifests/{commit-sha}/{package-path}.json` (`:86-88`); falls back to `manifests/{commit-sha}.json` otherwise. A `package-paths` value with >1 entry is rejected with a clear `setFailed` (`:22-28`), enforcing one package per matrix job. _(Review #1 ❌ → now fixed.)_ +- ✅ When `packagePath` is set, manifest written to `manifests/{commit-sha}/{package-path}.json` (`manifest-generate.ts:86-88`); falls back to `manifests/{commit-sha}.json` otherwise. A `package-paths` value with >1 entry is rejected with a clear `setFailed` (`manifest-generate.ts:22-28`), enforcing one package per matrix job. _(Review #1 ❌ → now fixed.)_ --- @@ -47,37 +47,37 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 2.2 PR Owns — PR introduced a visual diff (changed) -- ✅ Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are non-null (`classify:73-81`). +- ✅ Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are non-null (`manifest-compare-classify.ts:73-81`). - ✅ `base.png` downloaded from `base-images/path/base.png` (`manifest-diff.ts:31,34`). -- ✅ `new.png` downloaded from `new-images/{pr-sha}/path/new.png` (`:32`). +- ✅ `new.png` downloaded from `new-images/{pr-sha}/path/new.png` (`manifest-diff.ts:32`). - ✅ `diff.png` generated via pixelmatch (`diff-png.ts`, `manifest-diff.ts:39`). -- ⚠️ `base.png` and `diff.png` uploaded to `new-images/{pr-sha}/path/{base,diff}.png` (`:41-52`). **Caveat:** they are uploaded as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest — `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it just achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing. +- ⚠️ `base.png` and `diff.png` uploaded to `new-images/{pr-sha}/path/{base,diff}.png` (`manifest-diff.ts:41-52`). **Caveat:** they are uploaded as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest — `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it just achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing. ### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) -- ✅ Classified `added` (`headHash === ancestorHash === null`, `classify:75-76`). +- ✅ Classified `added` (`headHash === ancestorHash === null`, `manifest-compare-classify.ts:75-76`). - ✅ No `base.png` download / no `diff.png` — `generateDiffs` processes only `type === 'changed'` (`manifest-diff.ts:23`). - ✅ Only `new.png` referenced (already uploaded by generate). - ✅ Counts toward pending status and comment — `reviewable` includes `added` (`manifest-compare.ts:122`); comment reports `addedCount` (`run.ts:437`). ### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) -- ✅ Classified `deleted` (`prHash === null`, ancestor non-null, `classify:77-78`). +- ✅ Classified `deleted` (`prHash === null`, ancestor non-null, `manifest-compare-classify.ts:77-78`). - ✅ Deletion logged as info — `manifest-compare.ts:125-127`. -- ✅ Recorded as `null` in the changeset — `buildChangeset:170-171`. -- ✅ Does not contribute to pending/comment — excluded from `reviewable`; a deletions-only PR yields a **success** status and no comment (`:133-144`). +- ✅ Recorded as `null` in the changeset — `manifest-compare.ts:170-171`. +- ✅ Does not contribute to pending/comment — excluded from `reviewable`; a deletions-only PR yields a **success** status and no comment (`manifest-compare.ts:133-144`). ### 2.5 Main Owns — main changed, PR clean -- ✅ `prHash === ancestorHash` ⇒ `mainOwns` (`classify:82-84`). No diff, not in changeset (`buildChangeset` iterates only `prOwns`), no pending/failure from this path alone. +- ✅ `prHash === ancestorHash` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`). No diff, not in changeset (`buildChangeset` at `manifest-compare.ts:163-183` iterates only `prOwns`), no pending/failure from this path alone. ### 2.6 Main Owns — screenshot added on main since branching -- ✅ HEAD has it, PR/ancestor don't ⇒ `prHash === ancestorHash === null` ⇒ `mainOwns` (`classify:82-84`). +- ✅ HEAD has it, PR/ancestor don't ⇒ `prHash === ancestorHash === null` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`). ### 2.7 Conflict — all three hashes differ -- ✅ Reaches the `else` (conflict) branch only when `headHash !== ancestorHash` **and** `prHash !== ancestorHash` (`classify:85-87`). Because `differingPaths` is pre-filtered to `prManifest[p] !== headManifest[p]` (`:49-51`), `prHash !== headHash` is also guaranteed here — so the conflict branch correctly fires only when all three differ, and never when PR and main coincidentally made the same change. +- ✅ Reaches the `else` (conflict) branch only when `headHash !== ancestorHash` **and** `prHash !== ancestorHash` (`manifest-compare-classify.ts:85-87`). Because `differingPaths` is pre-filtered to `prManifest[p] !== headManifest[p]` (`manifest-compare-classify.ts:49-51`), `prHash !== headHash` is also guaranteed here — so the conflict branch correctly fires only when all three differ, and never when PR and main coincidentally made the same change. ### 2.8 Outcome: any conflict → failure @@ -89,32 +89,32 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 2.11 Outcome: ≥1 PR Owns, no conflicts → pending -- ✅ `handlePrOwns` writes the changeset (`:131`), sets pending status with `target_url` (`:148-154`), and posts the diffs comment (`:156-160`). _(Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields success + no comment, which is the documented deletions behavior, not a regression.)_ +- ✅ `handlePrOwns` writes the changeset (`manifest-compare.ts:131`), sets pending status with `target_url` (`manifest-compare.ts:148-154`), and posts the diffs comment (`manifest-compare.ts:156-160`). _(Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields success + no comment, which is the documented deletions behavior, not a regression.)_ ### 2.12 Changeset schema - ✅ Written to `changesets/{pr-head-sha}.json` (`manifest-s3.ts:39-50`, `sha = prSha`). -- ✅ `_headSha` = the HEAD SHA resolved in compare step 2 (`result.headSha`, `buildChangeset:168`). -- ✅ One entry per PR-owned changed/added path with the PR's new hash (`buildChangeset:172-180`). -- ✅ One `null` entry per PR-deleted path (`:170-171`). +- ✅ `_headSha` = the HEAD SHA resolved in compare step 2 (`result.headSha`, `manifest-compare.ts:168`). +- ✅ One entry per PR-owned changed/added path with the PR's new hash (`manifest-compare.ts:172-180`). +- ✅ One `null` entry per PR-deleted path (`manifest-compare.ts:170-171`). - ✅ Main Owns excluded (loop iterates only `prOwns`). -- ✅ Contains only PR-introduced paths. Defensive guard throws if a non-deleted entry is missing its PR hash (`:174-178`). +- ✅ Contains only PR-introduced paths. Defensive guard throws if a non-deleted entry is missing its PR hash (`manifest-compare.ts:174-178`). ### 2.13 Missing ancestor manifest -- ⚠️ `requireAncestorManifest` throws with the exact required guidance ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest", `classify:137-149`). The action does fail and the message surfaces. **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation. +- ⚠️ `requireAncestorManifest` throws with the exact required guidance ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest", `manifest-compare-classify.ts:137-149`). The action does fail and the message surfaces. **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation. ### 2.14 HEAD SHA resolution -- ✅ `octokit.rest.repos.getBranch({ ...repo, branch: baseRef })` → `data.commit.sha` (`classify:151-161`) — the live branches endpoint, not a cached value. +- ✅ `octokit.rest.repos.getBranch({ ...repo, branch: baseRef })` → `data.commit.sha` (`manifest-compare-classify.ts:151-161`) — the live branches endpoint, not a cached value. ### 2.15 Ancestor SHA resolution -- ✅ `octokit.rest.repos.compareCommitsWithBasehead({ basehead: \`${headSha}...${prSha}\` })`→`merge_base_commit.sha` (`classify:163-174`). +- ✅ `octokit.rest.repos.compareCommitsWithBasehead({ basehead: \`${headSha}...${prSha}\` })`→`merge_base_commit.sha` (`manifest-compare-classify.ts:163-174`). ### 2.16 Monorepo — squash per-package manifests before comparison -- ✅ `squashPrManifest` lists `manifests/{pr-sha}/`, merges all parts, and uploads the combined `manifests/{pr-sha}.json` (`manifest-s3.ts:80-109`); duplicate keys across packages throw. No-op (returns `null`, writes nothing) for single-package PRs. `classify` then reads the squashed `manifests/{pr-sha}.json` alongside HEAD and ancestor manifests. +- ✅ `squashPrManifest` lists `manifests/{pr-sha}/`, merges all parts, and uploads the combined `manifests/{pr-sha}.json` (`manifest-s3.ts:80-109`); duplicate keys across packages throw. No-op (returns `null`, writes nothing) for single-package PRs. `classifyManifests` (`manifest-compare-classify.ts`) then reads the squashed `manifests/{pr-sha}.json` alongside HEAD and ancestor manifests. --- @@ -122,15 +122,15 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 3.1 Manifest always written for merge commit -- ⚠️ Written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists." +- ⚠️ Written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`manifest-merge.ts:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists." ### 3.2 No changeset — copy parent manifest -- ✅ `parentManifest` copied verbatim to `manifests/{merge-commit-sha}.json` (`:49-55`); returns before any base-image update or status change. +- ✅ `parentManifest` copied verbatim to `manifests/{merge-commit-sha}.json` (`manifest-merge.ts:49-55`); returns before any base-image update or status change. ### 3.3 Conflict prevention — overlapping open PR changesets -- ✅ `flagOverlappingOpenPrs` paginates **all** open PRs, loads each one's changeset, and on any shared screenshot path sets a `failure` commit status on that PR's head SHA with "Visual comparison outdated — please rebase." (`manifest-merge-flag-prs.ts:38-69`). Skips the merging PR itself (`:46`). +- ✅ `flagOverlappingOpenPrs` paginates **all** open PRs, loads each one's changeset, and on any shared screenshot path sets a `failure` commit status on that PR's head SHA with "Visual comparison outdated — please rebase." (`manifest-merge-flag-prs.ts:38-69`). Skips the merging PR itself (`manifest-merge-flag-prs.ts:46`). ### 3.4 Stale changeset — no overlapping paths → proceed @@ -146,7 +146,7 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 3.7 Overlay — null entries remove key -- ✅ `delete result[path]` for null entries (`:19-21`). +- ✅ `delete result[path]` for null entries (`manifest-merge-overlay.ts:19-21`). ### 3.8 Base image update — non-null entries @@ -154,7 +154,7 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 3.9 Base image update — null entries delete base image -- ✅ `deleteObjects` removes `base-images/path/base.png` for null entries (`:67-77`). +- ✅ `deleteObjects` removes `base-images/path/base.png` for null entries (`manifest-merge-base-images.ts:67-77`). ### 3.10 Parent SHA resolution From 1414792ed2b60682d0c414c14dc021add6ddc00d Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 12:07:45 -0500 Subject: [PATCH 16/20] update review 2 --- MANIFEST_AC_REVIEW_2.md | 261 +++++++++++++++++++++++++++++----------- 1 file changed, 192 insertions(+), 69 deletions(-) diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md index 9f393857..c0150d2d 100644 --- a/MANIFEST_AC_REVIEW_2.md +++ b/MANIFEST_AC_REVIEW_2.md @@ -14,28 +14,40 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 1.1 Normal run — differential upload -- ✅ MD5 of each `new.png` via Node `crypto` from the full-size file on disk — `hash.ts` reads the file and `createHash('md5')`; called at `manifest-generate.ts:56` before any resize. -- ✅ Each entry keyed by path relative to the screenshots root, prefixed with package path for monorepos — `manifest-generate.ts:53-55` (`manifestKey = packagePath ? \`${packagePath}/${localKey}\` : localKey`). _(Review #1 ❌ → now fixed.)_ -- ✅ Only changed images uploaded to `new-images/{commit-sha}/path/new.png` — `changedEntries` filter (`manifest-generate.ts:65-67`), key at `manifest-generate.ts:80`. -- ✅ Resize enabled → resized before upload — `:75-77`. -- ✅ Resize not enabled → full-size — `:75-77` (`resizeEnabled` false path passes the raw buffer). -- ✅ Matching-hash images not uploaded — filter excludes them. -- ✅ Nothing written to `original-new-images/` — no such write exists in `manifest-generate.ts`. _(Review #1 ❌ → now fixed.)_ -- ✅ Manifest written to `manifests/{commit-sha}.json` mapping **all** screenshots — `manifest` built from every entry (`manifest-generate.ts:57`), written at `manifest-generate.ts:89-94`. +**Given** the `workflow` input is `manifest-generate`, a HEAD manifest exists at `manifests/{head-sha}.json` in S3, and `visual-test-command` completes successfully +**When** the action runs +**Then**: + +- ✅ The MD5 hash of each `new.png` in the screenshots directory is computed using Node.js `crypto` from the full-size image on disk — _`hash.ts` reads the file and `createHash('md5')`; called at `manifest-generate.ts:56` before any resize_ +- ✅ Each entry is keyed by the screenshot directory's path relative to the screenshots root (prefixed with package path for monorepos) — _`manifest-generate.ts:53-55` (`manifestKey = packagePath ? \`${packagePath}/${localKey}\` : localKey`); Review #1 ❌ → now fixed_ +- ✅ Only images whose hash differs from the HEAD manifest are uploaded to `new-images/{commit-sha}/path/new.png` — _`changedEntries` filter (`manifest-generate.ts:65-67`), key at `manifest-generate.ts:80`_ +- ✅ If resize is enabled: changed images are resized before upload — _`manifest-generate.ts:75-77`_ +- ✅ If resize is not enabled: changed images are uploaded full-size — _`manifest-generate.ts:75-77` (`resizeEnabled` false path passes the raw buffer)_ +- ✅ Images whose hash matches the HEAD manifest are not uploaded — _filter excludes them_ +- ✅ Nothing is ever written to `original-new-images/` — _no such write exists in `manifest-generate.ts`; Review #1 ❌ → now fixed_ +- ✅ A manifest is written to `manifests/{commit-sha}.json` mapping every screenshot directory path to its MD5 hash (all screenshots, not just changed ones) — _`manifest` built from every entry (`manifest-generate.ts:57`), written at `manifest-generate.ts:89-94`_ ### 1.2 First run — no HEAD manifest exists -- ✅ Missing manifest treated as empty — `fetchHeadManifest` returns `null` on `NoSuchKey` (`manifest-generate.ts:113-118`); also `null` when `head-sha` not supplied (`manifest-generate.ts:61-63`). Filter `!headManifest || ...` ⇒ upload all. -- ✅ All images uploaded following 1.1 resize rules. -- ✅ Manifest written for all screenshots. +**Given** the `workflow` input is `manifest-generate` and no manifest exists at `manifests/{head-sha}.json` in S3 +**When** the action runs +**Then**: -### 1.3 Hashing always from full-size image +- ✅ The missing manifest is treated as an empty object (no hash comparisons are performed) — _`fetchHeadManifest` returns `null` on `NoSuchKey` (`manifest-generate.ts:113-118`); also `null` when `head-sha` not supplied (`manifest-generate.ts:61-63`). Filter `!headManifest || ...` ⇒ upload all_ +- ✅ All images are uploaded to `new-images/{commit-sha}/path/new.png` following the same resize rules as 1.1 +- ✅ A manifest is written to `manifests/{commit-sha}.json` for all screenshots -- ✅ Hash taken at `manifest-generate.ts:56` from the on-disk file; resize only ever touches the upload buffer (`manifest-generate.ts:75`), never the hash input. +### 1.3 Hashing is always from full-size image + +**Given** any run of `manifest-generate` +**When** hashes are computed +**Then** ✅ each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied — _hash taken at `manifest-generate.ts:56` from the on-disk file; resize only ever touches the upload buffer (`manifest-generate.ts:75`), never the hash input_ ### 1.4 Monorepo — per-package manifest path -- ✅ When `packagePath` is set, manifest written to `manifests/{commit-sha}/{package-path}.json` (`manifest-generate.ts:86-88`); falls back to `manifests/{commit-sha}.json` otherwise. A `package-paths` value with >1 entry is rejected with a clear `setFailed` (`manifest-generate.ts:22-28`), enforcing one package per matrix job. _(Review #1 ❌ → now fixed.)_ +**Given** the `workflow` input is `manifest-generate` and `package-paths` is non-empty +**When** the manifest is written to S3 +**Then** ✅ the manifest is uploaded to `manifests/{commit-sha}/{package-path}.json` (one file per package invocation) instead of `manifests/{commit-sha}.json` — _`manifest-generate.ts:86-88`; falls back to `manifests/{commit-sha}.json` otherwise. A `package-paths` value with >1 entry is rejected with a clear `setFailed` (`manifest-generate.ts:22-28`), enforcing one package per matrix job. Review #1 ❌ → now fixed_ --- @@ -43,78 +55,147 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 2.1 All hashes match — success -- ✅ `classifyManifests` returns `{ outcome: 'match' }` when `differingPaths` is empty (`manifest-compare-classify.ts:49-55`); `manifestCompare` sets a success status and returns (`manifest-compare.ts:62-71`) — no changeset, no comment. +**Given** the `workflow` input is `manifest-compare`, the PR manifest at `manifests/{pr-head-sha}.json` and the HEAD manifest at `manifests/{head-sha}.json` both exist, and every hash in the PR manifest matches the corresponding hash in the HEAD manifest +**When** the action runs +**Then**: + +- ✅ A success commit status is set on the PR head SHA — _`classifyManifests` returns `{ outcome: 'match' }` when `differingPaths` is empty (`manifest-compare-classify.ts:49-55`); `manifestCompare` sets success and returns (`manifest-compare.ts:62-71`)_ +- ✅ No changeset is written +- ✅ No Comparadise comment is posted + +### 2.2 PR Owns — PR introduced a visual diff (normal case) -### 2.2 PR Owns — PR introduced a visual diff (changed) +**Given** at least one hash differs between the PR manifest and the HEAD manifest, and for a given differing path the HEAD hash equals the ancestor hash (PR introduced the change) +**When** the 3-way comparison runs for that path +**Then**: -- ✅ Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are non-null (`manifest-compare-classify.ts:73-81`). -- ✅ `base.png` downloaded from `base-images/path/base.png` (`manifest-diff.ts:31,34`). -- ✅ `new.png` downloaded from `new-images/{pr-sha}/path/new.png` (`manifest-diff.ts:32`). -- ✅ `diff.png` generated via pixelmatch (`diff-png.ts`, `manifest-diff.ts:39`). -- ⚠️ `base.png` and `diff.png` uploaded to `new-images/{pr-sha}/path/{base,diff}.png` (`manifest-diff.ts:41-52`). **Caveat:** they are uploaded as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest — `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it just achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing. +_Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are non-null (`manifest-compare-classify.ts:73-81`)._ + +- ✅ `base.png` is downloaded from `base-images/path/base.png` — _`manifest-diff.ts:31,34`_ +- ✅ `new.png` is downloaded from `new-images/{pr-sha}/path/new.png` (as uploaded by `manifest-generate`) — _`manifest-diff.ts:32`_ +- ✅ A `diff.png` is generated via pixelmatch — _`diff-png.ts`, `manifest-diff.ts:39`_ +- ⚠️ `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload — _`manifest-diff.ts:41-52` uploads them as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest: `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing_ ### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) -- ✅ Classified `added` (`headHash === ancestorHash === null`, `manifest-compare-classify.ts:75-76`). -- ✅ No `base.png` download / no `diff.png` — `generateDiffs` processes only `type === 'changed'` (`manifest-diff.ts:23`). -- ✅ Only `new.png` referenced (already uploaded by generate). -- ✅ Counts toward pending status and comment — `reviewable` includes `added` (`manifest-compare.ts:122`); comment reports `addedCount` (`run.ts:437`). +**Given** a path exists in the PR manifest but does not exist in either the HEAD manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +_Classified `added` (`headHash === ancestorHash === null`, `manifest-compare-classify.ts:75-76`)._ + +- ✅ No `base.png` is downloaded — _`generateDiffs` processes only `type === 'changed'` (`manifest-diff.ts:23`)_ +- ✅ No `diff.png` is generated or uploaded — _same filter_ +- ✅ Only `new.png` is referenced (already uploaded by `manifest-generate`) +- ✅ The path is treated as a PR-owned change (contributes to pending status and Comparadise comment) — _`reviewable` includes `added` (`manifest-compare.ts:122`); comment reports `addedCount` (`run.ts:437`)_ ### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) -- ✅ Classified `deleted` (`prHash === null`, ancestor non-null, `manifest-compare-classify.ts:77-78`). -- ✅ Deletion logged as info — `manifest-compare.ts:125-127`. -- ✅ Recorded as `null` in the changeset — `manifest-compare.ts:170-171`. -- ✅ Does not contribute to pending/comment — excluded from `reviewable`; a deletions-only PR yields a **success** status and no comment (`manifest-compare.ts:133-144`). +**Given** a path exists in the ancestor manifest and the HEAD manifest with the same hash, but does not exist in the PR manifest +**When** the 3-way comparison runs for that path +**Then**: + +_Classified `deleted` (`prHash === null`, ancestor non-null, `manifest-compare-classify.ts:77-78`)._ + +- ✅ The deletion is logged as an informational message — _`manifest-compare.ts:125-127`_ +- ✅ The path is recorded as a `null` entry in the changeset — _`manifest-compare.ts:170-171`_ +- ✅ The path does not contribute to pending status or Comparadise comment — _excluded from `reviewable`; a deletions-only PR yields a **success** status and no comment (`manifest-compare.ts:133-144`)_ -### 2.5 Main Owns — main changed, PR clean +### 2.5 Main Owns — main changed, PR is clean -- ✅ `prHash === ancestorHash` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`). No diff, not in changeset (`buildChangeset` at `manifest-compare.ts:163-183` iterates only `prOwns`), no pending/failure from this path alone. +**Given** for a given differing path the PR hash equals the ancestor hash (main introduced the change, not the PR) +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ No diff is generated for that path — _`prHash === ancestorHash` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`)_ +- ✅ The path is not included in the changeset — _`buildChangeset` at `manifest-compare.ts:163-183` iterates only `prOwns`_ +- ✅ This path alone does not cause a pending or failure status ### 2.6 Main Owns — screenshot added on main since branching -- ✅ HEAD has it, PR/ancestor don't ⇒ `prHash === ancestorHash === null` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`). +**Given** a path exists in the HEAD manifest but not in the PR manifest or the ancestor manifest +**When** the 3-way comparison runs for that path +**Then**: + +- ✅ The path is treated as Main Owns (main added it, PR didn't touch it) — _HEAD has it, PR/ancestor don't ⇒ `prHash === ancestorHash === null` ⇒ `mainOwns` (`manifest-compare-classify.ts:82-84`)_ +- ✅ No diff is generated, no failure or pending status is set for this path alone ### 2.7 Conflict — all three hashes differ -- ✅ Reaches the `else` (conflict) branch only when `headHash !== ancestorHash` **and** `prHash !== ancestorHash` (`manifest-compare-classify.ts:85-87`). Because `differingPaths` is pre-filtered to `prManifest[p] !== headManifest[p]` (`manifest-compare-classify.ts:49-51`), `prHash !== headHash` is also guaranteed here — so the conflict branch correctly fires only when all three differ, and never when PR and main coincidentally made the same change. +**Given** for a given path the PR hash, HEAD hash, and ancestor hash are all different +**When** the 3-way comparison runs for that path +**Then** ✅ the path is collected as a conflict — _reaches the `else` (conflict) branch only when `headHash !== ancestorHash` **and** `prHash !== ancestorHash` (`manifest-compare-classify.ts:85-87`). Because `differingPaths` is pre-filtered to `prManifest[p] !== headManifest[p]` (`manifest-compare-classify.ts:49-51`), `prHash !== headHash` is also guaranteed here — so the conflict branch fires only when all three differ, never when PR and main coincidentally made the same change_ ### 2.8 Outcome: any conflict → failure -- ✅ `handleConflicts` sets `setFailed`, a failure commit status, and posts a conflict comment (`manifest-compare.ts:94-113`). No changeset is written (returns before `putChangeset`). Comment lists conflicting paths with a rebase instruction (`run.ts:430-431`). +**Given** at least one path was collected as a conflict +**When** the outcome is determined +**Then**: -### 2.10 Outcome: all Main Owns → success +- ✅ A failure commit status is set on the PR head SHA — _`handleConflicts` sets `setFailed` and a failure commit status (`manifest-compare.ts:94-113`)_ +- ✅ A comment is posted listing the conflicting paths with a rebase instruction — _`run.ts:430-431`_ +- ✅ No changeset is written — _`handleConflicts` returns before `putChangeset`_ -- ✅ `prOwns.length === 0` ⇒ success status (`manifest-compare.ts:78-89`). +### 2.10 Outcome: all Main Owns (no PR Owns, no conflicts) → success -### 2.11 Outcome: ≥1 PR Owns, no conflicts → pending +**Given** all differing hashes are Main Owns (no PR Owns, no conflicts) +**When** the outcome is determined +**Then** ✅ a success commit status is set on the PR head SHA — _`prOwns.length === 0` ⇒ success status (`manifest-compare.ts:78-89`)_ -- ✅ `handlePrOwns` writes the changeset (`manifest-compare.ts:131`), sets pending status with `target_url` (`manifest-compare.ts:148-154`), and posts the diffs comment (`manifest-compare.ts:156-160`). _(Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields success + no comment, which is the documented deletions behavior, not a regression.)_ +### 2.11 Outcome: at least one PR Owns, no conflicts → pending + +**Given** at least one path is PR Owns and no paths are conflicts +**When** the outcome is determined +**Then**: + +- ✅ A pending commit status is set on the PR head SHA — _`handlePrOwns` sets pending status with `target_url` (`manifest-compare.ts:148-154`)_ +- ✅ A Comparadise comment is posted — _`manifest-compare.ts:156-160`_ +- ✅ A changeset is written to `changesets/{pr-head-sha}.json` — _`manifest-compare.ts:131`_ + +_Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields success + no comment, which is the documented deletions behavior, not a regression._ ### 2.12 Changeset schema -- ✅ Written to `changesets/{pr-head-sha}.json` (`manifest-s3.ts:39-50`, `sha = prSha`). -- ✅ `_headSha` = the HEAD SHA resolved in compare step 2 (`result.headSha`, `manifest-compare.ts:168`). -- ✅ One entry per PR-owned changed/added path with the PR's new hash (`manifest-compare.ts:172-180`). -- ✅ One `null` entry per PR-deleted path (`manifest-compare.ts:170-171`). -- ✅ Main Owns excluded (loop iterates only `prOwns`). -- ✅ Contains only PR-introduced paths. Defensive guard throws if a non-deleted entry is missing its PR hash (`manifest-compare.ts:174-178`). +**Given** a changeset is written +**Then**: + +- ✅ The file is at `changesets/{pr-head-sha}.json` — _`manifest-s3.ts:39-50`, `sha = prSha`_ +- ✅ It contains `_headSha`: the HEAD SHA resolved in step 2 of the compare flow — _`result.headSha` (`manifest-compare.ts:168`)_ +- ✅ It contains one entry per PR-owned changed path with the PR's new hash as the value — _`manifest-compare.ts:172-180`_ +- ✅ It contains one entry per PR-deleted path with `null` as the value — _`manifest-compare.ts:170-171`_ +- ✅ Main Owns paths are not included — _loop iterates only `prOwns`_ +- ✅ The changeset contains only paths where the PR introduced a change — _defensive guard throws if a non-deleted entry is missing its PR hash (`manifest-compare.ts:174-178`)_ ### 2.13 Missing ancestor manifest -- ⚠️ `requireAncestorManifest` throws with the exact required guidance ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest", `manifest-compare-classify.ts:137-149`). The action does fail and the message surfaces. **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation. +**Given** the ancestor SHA is resolved but `manifests/{ancestor-sha}.json` does not exist in S3 +**When** the compare runs +**Then**: + +- ✅ The action fails — _`requireAncestorManifest` throws (`manifest-compare-classify.ts:137-149`)_ +- ⚠️ An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest — _the message text is exactly that ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest"). **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation_ ### 2.14 HEAD SHA resolution -- ✅ `octokit.rest.repos.getBranch({ ...repo, branch: baseRef })` → `data.commit.sha` (`manifest-compare-classify.ts:151-161`) — the live branches endpoint, not a cached value. +**Given** the compare mode runs +**When** the HEAD SHA is resolved +**Then** ✅ the GitHub API is called at `GET /repos/{owner}/{repo}/branches/{base.ref}` to get the latest main HEAD SHA (not a stale cached value) — _`octokit.rest.repos.getBranch({ ...repo, branch: baseRef })` → `data.commit.sha` (`manifest-compare-classify.ts:151-161`)_ ### 2.15 Ancestor SHA resolution -- ✅ `octokit.rest.repos.compareCommitsWithBasehead({ basehead: \`${headSha}...${prSha}\` })`→`merge_base_commit.sha` (`manifest-compare-classify.ts:163-174`). +**Given** the HEAD SHA and PR SHA are known +**When** the ancestor SHA is resolved +**Then** ✅ the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA — _`octokit.rest.repos.compareCommitsWithBasehead({ basehead: \`${headSha}...${prSha}\` })`→`merge_base_commit.sha` (`manifest-compare-classify.ts:163-174`)_ ### 2.16 Monorepo — squash per-package manifests before comparison -- ✅ `squashPrManifest` lists `manifests/{pr-sha}/`, merges all parts, and uploads the combined `manifests/{pr-sha}.json` (`manifest-s3.ts:80-109`); duplicate keys across packages throw. No-op (returns `null`, writes nothing) for single-package PRs. `classifyManifests` (`manifest-compare-classify.ts`) then reads the squashed `manifests/{pr-sha}.json` alongside HEAD and ancestor manifests. +**Given** the `workflow` input is `manifest-compare` and `package-paths` is non-empty +**When** the action resolves the PR manifest +**Then**: + +- ✅ All per-package manifests at `manifests/{pr-sha}/{package-path}.json` are downloaded and merged into a single manifest — _`squashPrManifest` lists `manifests/{pr-sha}/` and merges all parts; duplicate keys across packages throw (`manifest-s3.ts:80-109`)_ +- ✅ The squashed manifest is uploaded to `manifests/{pr-sha}.json` — _`manifest-s3.ts:80-109`; no-op (returns `null`, writes nothing) for single-package PRs_ +- ✅ The squashed manifest is used alongside the pre-existing `manifests/{head-sha}.json` and `manifests/{ancestor-sha}.json` for the 3-way comparison — _`classifyManifests` reads the squashed `manifests/{pr-sha}.json` alongside HEAD and ancestor manifests_ --- @@ -122,43 +203,74 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 3.1 Manifest always written for merge commit -- ⚠️ Written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`manifest-merge.ts:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists." +**Given** the `workflow` input is `manifest-merge` +**When** the action runs +**Then** ⚠️ a manifest is always written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists — _written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`manifest-merge.ts:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists."_ ### 3.2 No changeset — copy parent manifest -- ✅ `parentManifest` copied verbatim to `manifests/{merge-commit-sha}.json` (`manifest-merge.ts:49-55`); returns before any base-image update or status change. +**Given** no changeset exists at `changesets/{pr-head-sha}.json` +**When** the action runs +**Then**: + +- ✅ The parent manifest (`manifests/{parent-sha}.json`) is copied as-is to `manifests/{merge-commit-sha}.json` — _`manifest-merge.ts:49-55`_ +- ✅ No base images are updated — _returns before any base-image update_ +- ✅ No failure status is set — _returns before any status change_ ### 3.3 Conflict prevention — overlapping open PR changesets -- ✅ `flagOverlappingOpenPrs` paginates **all** open PRs, loads each one's changeset, and on any shared screenshot path sets a `failure` commit status on that PR's head SHA with "Visual comparison outdated — please rebase." (`manifest-merge-flag-prs.ts:38-69`). Skips the merging PR itself (`manifest-merge-flag-prs.ts:46`). +**Given** a changeset exists for the merging PR, and at least one other open PR has a changeset that shares at least one screenshot path key with the merging PR's changeset +**When** the merge action runs +**Then**: + +- ✅ A failure commit status is set on that other open PR's head SHA — _`flagOverlappingOpenPrs` sets a `failure` commit status on the PR's head SHA (`manifest-merge-flag-prs.ts:38-69`)_ +- ✅ The failure status message indicates visual comparison is outdated and a rebase is required — _"Visual comparison outdated — please rebase."_ +- ✅ This check is performed for every open PR that has a changeset in S3 — _`flagOverlappingOpenPrs` paginates **all** open PRs and loads each one's changeset; skips the merging PR itself (`manifest-merge-flag-prs.ts:46`). Review #1 ❌ → now fixed_ ### 3.4 Stale changeset — no overlapping paths → proceed -- ✅ When `_headSha !== parentSha`, `detectStaleConflicts` compares `manifests/{_headSha}` vs `manifests/{parent}` on changeset keys; empty result ⇒ returns and the merge proceeds (`manifest-merge.ts:64-66`, `:88`). +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, but none of the changeset's screenshot keys differ between `manifests/{changeset._headSha}.json` and `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then** ✅ the merge proceeds normally (the intervening merges didn't touch the same screenshots) — _when `_headSha !== parentSha`, `detectStaleConflicts` compares `manifests/{_headSha}` vs `manifests/{parent}` on changeset keys; empty result ⇒ returns and the merge proceeds (`manifest-merge.ts:64-66`, `:88`)_ ### 3.5 Stale changeset — overlapping paths → fail -- ✅ Non-empty conflicts ⇒ `core.setFailed` with the conflicting paths listed, then throw (`manifest-merge.ts:88-92`). +**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, and at least one changeset key has a different hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` +**When** the merge action runs +**Then**: + +- ✅ The action fails — _non-empty conflicts ⇒ `core.setFailed`, then throw (`manifest-merge.ts:88-92`)_ +- ✅ The conflicting paths are listed in the failure output — _`manifest-merge.ts:88-92`_ ### 3.6 Overlay — non-null entries update hash -- ✅ `overlayChangeset` sets `result[path] = hash` for non-null entries (`manifest-merge-overlay.ts:17-24`); parent manifest not mutated (spread copy). +**Given** a valid (non-stale) changeset exists +**When** the changeset is overlaid onto the parent manifest +**Then** ✅ for each non-null entry in the changeset, the corresponding key in the resulting manifest has the changeset's hash value — _`overlayChangeset` sets `result[path] = hash` for non-null entries (`manifest-merge-overlay.ts:17-24`); parent manifest not mutated (spread copy)_ ### 3.7 Overlay — null entries remove key -- ✅ `delete result[path]` for null entries (`manifest-merge-overlay.ts:19-21`). +**Given** a valid changeset contains a `null` entry for a path +**When** the changeset is overlaid onto the parent manifest +**Then** ✅ that path is absent from `manifests/{merge-commit-sha}.json` — _`delete result[path]` for null entries (`manifest-merge-overlay.ts:19-21`)_ ### 3.8 Base image update — non-null entries -- ✅ `applyChangesetToBaseImages` copies `new-images/{pr-sha}/path/new.png` → `base-images/path/base.png` (`manifest-merge-base-images.ts:55-66`), with a properly URI-encoded `CopySource`. +**Given** a valid changeset is applied +**When** base images are updated +**Then** ✅ for each non-null changeset entry, `new-images/{pr-sha}/path/new.png` is copied to `base-images/path/base.png` — _`applyChangesetToBaseImages` (`manifest-merge-base-images.ts:55-66`), with a properly URI-encoded `CopySource`_ ### 3.9 Base image update — null entries delete base image -- ✅ `deleteObjects` removes `base-images/path/base.png` for null entries (`manifest-merge-base-images.ts:67-77`). +**Given** a valid changeset contains a `null` entry for a path +**When** base images are updated +**Then** ✅ `base-images/path/base.png` is deleted from S3 — _`deleteObjects` removes `base-images/path/base.png` for null entries (`manifest-merge-base-images.ts:67-77`)_ ### 3.10 Parent SHA resolution -- ✅ `octokit.rest.repos.getCommit({ ref: mergeSha })` → `data.parents[0].sha`, used as the parent manifest key (`run.ts:356-368`); throws if no parent exists. +**Given** the merge commit SHA is known +**When** the parent manifest is fetched +**Then** ✅ the GitHub API is called to get `parents[0].sha` of the merge commit, and `manifests/{parents[0].sha}.json` is used as the parent manifest — _`octokit.rest.repos.getCommit({ ref: mergeSha })` → `data.parents[0].sha` (`run.ts:356-368`); throws if no parent exists_ --- @@ -166,32 +278,43 @@ Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisf ### 4.1 New modes do not affect existing modes -- ✅ `run.ts:37-50` dispatches the three manifest modes and returns early; the `pr`/`merge` code paths below are untouched. Covered by `run.test.ts`. +**Given** the `workflow` input is `pr` or `merge` +**When** the action runs +**Then** ✅ behavior is identical to before this implementation — no regressions in existing modes — _`run.ts:37-50` dispatches the three manifest modes and returns early; the `pr`/`merge` code paths below are untouched. Covered by `run.test.ts`_ ### 4.2 Manifest key format — monorepo -- ✅ Keys prefixed with the package path (`manifest-generate.ts:55`). +**Given** the action is run in a monorepo with a non-empty `package-paths` input +**When** manifest entries are keyed +**Then** ✅ each key is prefixed with the package path (e.g. `packages/ui/components/Button`, not just `components/Button`) — _`manifest-generate.ts:55`_ ### 4.3 Manifest key format — single package -- ✅ No prefix when `package-paths` unset (`packagePath = ''` ⇒ `manifestKey = localKey`). +**Given** no `package-paths` input is set +**When** manifest entries are keyed +**Then** ✅ each key is the screenshot directory's path relative to the screenshots root (no prefix) — _`packagePath = ''` ⇒ `manifestKey = localKey`_ ### 4.4 S3 key structure is exact -- ✅ Manifests (single/squashed): `manifests/{sha}.json` (`manifest-s3.ts`, `manifest-generate.ts:88`). -- ✅ Manifests (monorepo generate): `manifests/{sha}/{package-path}.json` (`manifest-generate.ts:86-87`). -- ✅ Changesets: `changesets/{sha}.json` (`manifest-s3.ts:43`). -- ✅ New images: `new-images/{sha}/path/new.png`, resized when enabled (`manifest-generate.ts:80`). -- ✅ Base images: `base-images/path/base.png`, populated by copy from already-resized `new-images/` (`manifest-merge-base-images.ts:63`). -- ✅ `original-new-images/` never written by any manifest mode (grep-confirmed; constant `ORIGINAL_NEW_IMAGES_DIRECTORY` is unused outside legacy `pr` mode). +**Given** the implementation writes or reads any manifest, changeset, or image +**Then** the following exact S3 key patterns are used: + +- ✅ Manifests (single-package, or squashed by compare for monorepo): `manifests/{commit-sha}.json` — _`manifest-s3.ts`, `manifest-generate.ts:88`_ +- ✅ Manifests (monorepo, written by generate per package): `manifests/{commit-sha}/{package-path}.json` — _`manifest-generate.ts:86-87`_ +- ✅ Changesets: `changesets/{pr-head-sha}.json` — _`manifest-s3.ts:43`_ +- ✅ New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) — _`manifest-generate.ts:80`_ +- ✅ Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) — _populated by copy from already-resized `new-images/` (`manifest-merge-base-images.ts:63`)_ +- ✅ `original-new-images/` is never written by any manifest mode — _grep-confirmed; constant `ORIGINAL_NEW_IMAGES_DIRECTORY` is unused outside legacy `pr` mode_ ### 4.5 MD5 hashing implementation -- ✅ `hash.ts` uses Node `crypto` `createHash('md5')` over the full-size file buffer. +**Given** a screenshot file is hashed +**Then** ✅ Node.js `crypto` is used to compute the MD5 hash of the full-size image file — _`hash.ts` uses Node `crypto` `createHash('md5')` over the full-size file buffer_ ### 4.6 Concurrency constraint is documented -- ✅ `docs/docs/setup/manifest-workflows.md:137-149` states the `manifest-merge` workflow must set a `concurrency` group with `cancel-in-progress: false`, with the rationale, and the example YAML includes it. +**Given** the implementation is complete +**Then** ✅ the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races — _`docs/docs/setup/manifest-workflows.md:137-149` states the requirement, the rationale, and the example YAML includes it_ --- From a4d85c701082172a252a53445c319bf847fbec57 Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 12:57:46 -0500 Subject: [PATCH 17/20] Refine manifest AC/review for stale-conflict edge and fix criterion count - Reframe AC 3.1 as "written for every successful merge run" with an explicit stale-conflict hard-failure exception, defining the precise merge-race window that can leave a merge commit without a manifest. - Clarify AC 2.2 that diff/base resize is satisfied implicitly (sources resized at rest) and that diff generation tolerates dimension mismatch. - Mark Review #2 items 2.2 / 2.13 / 3.1 satisfied with factual notes. - Fix stale criterion count 40 -> 35 in Review #2 summary and verdict. Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC.md | 17 ++++++++++++++--- MANIFEST_AC_REVIEW_2.md | 23 +++++++++++++++-------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md index 36e0deb1..14d295f0 100644 --- a/MANIFEST_AC.md +++ b/MANIFEST_AC.md @@ -68,6 +68,8 @@ This document is the authoritative acceptance criteria for the `manifest-generat - A `diff.png` is generated via pixelmatch - `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload +_Note: "resized before upload" is satisfied implicitly when the source `base-images/` and `new-images/` objects are already at resized dimensions (both are written resized at merge/generate time), so no second resize is required here. Diff generation must tolerate a base/new dimension mismatch rather than assume identical dimensions._ + ### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) **Given** a path exists in the PR manifest but does not exist in either the HEAD manifest or the ancestor manifest @@ -187,11 +189,20 @@ This document is the authoritative acceptance criteria for the `manifest-generat ## 3. `manifest-merge` mode -### 3.1 Manifest always written for merge commit +### 3.1 Manifest written for every successful merge run **Given** the `workflow` input is `manifest-merge` -**When** the action runs -**Then** a manifest is always written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists +**When** the action completes successfully (no changeset, a valid changeset, or a non-overlapping stale changeset) +**Then** a manifest is written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists + +**Exception — stale-conflict hard failure:** when a stale changeset has overlapping conflicts (3.5), the action fails _before_ writing a manifest. This is intentional — a manifest just declared conflicting must not be published, and the correct baseline for the overlapping paths is genuinely ambiguous. The resulting chain gap on `main` is prevented in practice by requiring the `Visual Regression` status check, which blocks a stale/overlapping PR from merging at all. Only one specific race can still reach this path, given two PRs (A and B) that both changed the same screenshot path: + +1. A is PR-Owns against base X and its visual changes are accepted in the Comparadise UI, flipping its required `Visual Regression` status to `success` — A is now mergeable. +2. B (also owning that path) merges; `main` advances to Y. B's merge run will overwrite A's status with `failure` via the overlap flag (3.3). +3. **The window** is the interval between B's merge commit landing on `main` and B's merge workflow actually writing that `failure` status onto A's head. Until it lands, A's head still shows the stale `success`, so the required check passes. +4. If A is merged inside that window (auto-merge firing, or a manual merge), A lands on top of Y; A's own merge run then sees `A.changeset._headSha` (X) ≠ A's merge parent (Y) with the shared path differing → stale conflict → fail before any manifest is written. + +The window is normally seconds, but because merge workflows are serialized by the required `concurrency` group (`cancel-in-progress: false`), a backlog of queued merges can delay B's flag and widen it. Once the flag lands, A's required check is red and A cannot merge until it rebases. ### 3.2 No changeset — copy parent manifest diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md index c0150d2d..19e44351 100644 --- a/MANIFEST_AC_REVIEW_2.md +++ b/MANIFEST_AC_REVIEW_2.md @@ -6,7 +6,7 @@ Source-only review of `action/src/` at the PR head SHA `02869c8`. (Local review Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisfied with a caveat worth noting), with a reason for anything not plainly satisfied. -**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 40 criteria are satisfied. Three minor caveats are noted (2.2, 2.13, 3.1) — none are behavioral failures. +**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 35 criteria are satisfied. --- @@ -74,7 +74,7 @@ _Classified `changed` when `headHash === ancestorHash` and both PR/ancestor are - ✅ `base.png` is downloaded from `base-images/path/base.png` — _`manifest-diff.ts:31,34`_ - ✅ `new.png` is downloaded from `new-images/{pr-sha}/path/new.png` (as uploaded by `manifest-generate`) — _`manifest-diff.ts:32`_ - ✅ A `diff.png` is generated via pixelmatch — _`diff-png.ts`, `manifest-diff.ts:39`_ -- ⚠️ `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload — _`manifest-diff.ts:41-52` uploads them as-is, with no explicit resize call. This still satisfies the intent because both source images are already resized at rest: `base-images/` is populated from already-resized `new-images/` during merge, and `new-images/` is resized at generate time — so the generated diff and the re-uploaded base are already at resized dimensions. The behavior is correct; it achieves "resized" implicitly rather than by re-running resize. Worth a confirming test if resize correctness here is load-bearing_ +- ✅ `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload — _`manifest-diff.ts:41-52`. Both source images are already resized at rest (`base-images/` is copied from already-resized `new-images/` at merge time; `new-images/` is resized at generate time), so the re-uploaded base and generated diff are already at resized dimensions without a second resize call. `diff-png.ts` also pads any dimension mismatch to `Math.max(width)`×`Math.max(height)` via `ensureSize` before `pixelmatch`, so it never assumes identical dimensions_ ### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) @@ -172,8 +172,8 @@ _Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields su **When** the compare runs **Then**: -- ✅ The action fails — _`requireAncestorManifest` throws (`manifest-compare-classify.ts:137-149`)_ -- ⚠️ An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest — _the message text is exactly that ("Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest"). **Caveat:** the failure is delivered by a thrown `Error` that is **not** caught at the top level — `main.ts` is just `run();` with no `.catch()`, so it surfaces as an unhandled rejection (non-zero exit + stderr) rather than via `core.setFailed`. This matches the pre-existing `pr`/`merge` error style, so it's consistent, but routing it through `core.setFailed` would produce a cleaner Actions annotation_ +- ✅ The action fails — _`requireAncestorManifest` throws (`manifest-compare-classify.ts:137-149`); `main.ts` is `run();` with no top-level `.catch()`, so the rejection exits non-zero and the Actions step fails. Matches the pre-existing `pr`/`merge` error style; routing it through `core.setFailed` would produce a cleaner annotation_ +- ✅ An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest — _exact text: "Ensure `manifest-generate` has run on the base branch, then rebase your branch onto a commit that has a manifest"_ ### 2.14 HEAD SHA resolution @@ -201,11 +201,18 @@ _Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields su ## 3. `manifest-merge` mode -### 3.1 Manifest always written for merge commit +### 3.1 Manifest written for every successful merge run **Given** the `workflow` input is `manifest-merge` -**When** the action runs -**Then** ⚠️ a manifest is always written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists — _written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`manifest-merge.ts:69`). **Caveat:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws **before** `putManifest`, so no manifest is written in that case. This is the correct behavior for a hard failure (you do not want to publish a manifest you just declared conflicting), and 3.5 explicitly requires the action to fail — so "always written" holds for every non-failing path. Flagging only because the literal wording is "regardless of whether a changeset exists."_ +**When** the action completes successfully (no changeset, a valid changeset, or a non-overlapping stale changeset) +**Then** ✅ a manifest is written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists — _written on both the no-changeset path (`manifest-merge.ts:53`) and the normal overlay path (`manifest-merge.ts:69`)._ + +**Stale-conflict hard failure:** when a stale changeset has overlapping conflicts (3.5), `assertNoStaleConflicts` throws before `putManifest` (`manifest-merge.ts:88-92`), so no manifest is written — intentional, since a manifest just declared conflicting must not be published and the correct baseline for the overlapping paths is genuinely ambiguous. The merge commit (already on `main`, since merge runs `on: pull_request: closed` + `if: merged == true`) is left without a manifest. The required `Visual Regression` status check (`manifest-workflows.md:168-170`, `cicd.md:20`) blocks the common case; only one specific race reaches this path, given two PRs (A and B) that both changed the same screenshot path: + +1. A is PR-Owns against base X and is accepted in the Comparadise UI, flipping its required `Visual Regression` status to `success` (`acceptVisualChanges.ts:69-74`) — A is now mergeable. +2. B (also owning that path) merges; `main` advances to Y. B's `manifest-merge` will overwrite A's status with `failure` via `flagOverlappingOpenPrs` (3.3, `manifest-merge-flag-prs.ts:56-62`). +3. **The window** is the interval between B's merge commit landing on `main` and B's merge workflow executing that `createCommitStatus(failure)` on A's head. Until it lands, A's head still shows the stale `success`, so the required check passes. +4. If A is merged inside that window (auto-merge, or a manual click), A lands on top of Y; A's own `manifest-merge` then sees `A.changeset._headSha` (X) ≠ A's merge parent (Y) with the shared path differing → stale conflict → fails before `putManifest`. ### 3.2 No changeset — copy parent manifest @@ -320,4 +327,4 @@ _Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields su ## Verdict -All 40 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. The three ⚠️ caveats (2.2 implicit resize, 2.13 thrown error vs `core.setFailed`, 3.1 no manifest on the stale-conflict failure path) are correctness-neutral and optional to address; none block acceptance. +All 35 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. Two items carry minor implementation notes — 2.13 (failure surfaces via a thrown error rather than `core.setFailed`) and 3.1 (no manifest written on the intentional stale-conflict hard-failure path) — both correct and non-blocking, as noted inline. From a9991b56b170e9811c58cbd21f6f8606ad98e4a4 Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 13:35:48 -0500 Subject: [PATCH 18/20] docs(manifest): add AC 4.7 (inputs derived from event); expand 2.2 dimension note Adds acceptance criterion 4.7 requiring each manifest mode to derive its SHAs/refs/PR identifiers from the triggering event rather than dedicated head-sha/base-ref/pr-sha/pr-number/merge-commit-sha inputs, and expands the 2.2 note to state that diff generation must tolerate a base/new dimension mismatch (padding before pixelmatch). Co-Authored-By: Claude Opus 4.8 (1M context) --- MANIFEST_AC.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md index 14d295f0..d57949e8 100644 --- a/MANIFEST_AC.md +++ b/MANIFEST_AC.md @@ -68,7 +68,7 @@ This document is the authoritative acceptance criteria for the `manifest-generat - A `diff.png` is generated via pixelmatch - `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload -_Note: "resized before upload" is satisfied implicitly when the source `base-images/` and `new-images/` objects are already at resized dimensions (both are written resized at merge/generate time), so no second resize is required here. Diff generation must tolerate a base/new dimension mismatch rather than assume identical dimensions._ +_Note: "resized before upload" is satisfied implicitly when the source `base-images/` and `new-images/` objects are already at resized dimensions (both are written resized at merge/generate time), so no second resize is required here. Diff generation must tolerate a base/new dimension mismatch (e.g. by padding to a common canvas before pixelmatch) rather than assume identical dimensions, since a real visual change frequently alters a screenshot's height._ ### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) @@ -312,3 +312,15 @@ The window is normally seconds, but because merge workflows are serialized by th **Given** the implementation is complete **Then** the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races + +### 4.7 Inputs are derived from the triggering event, not duplicated as overrides + +**Given** a manifest workflow mode runs on its documented trigger (`manifest-generate` and `manifest-compare` on `pull_request`; `manifest-merge` on `pull_request` with `types: [closed]`) +**When** the action resolves the SHAs, refs, and PR identifiers it needs +**Then** each value is derived from the GitHub event context rather than required as a dedicated input — the action does not define `head-sha`, `base-ref`, `pr-sha`, `pr-number`, or `merge-commit-sha` inputs: + +- `manifest-generate` resolves the differential-upload baseline from the base branch (e.g. the latest base-branch HEAD via `pull_request.base.ref`); if no baseline manifest exists it is treated as empty (all images upload). No `head-sha` input is required. +- `manifest-compare` resolves the base branch ref from `pull_request.base.ref`. No `base-ref` input is required. +- `manifest-merge` resolves the PR head SHA from `pull_request.head.sha`, the PR number from `pull_request.number`, and the merge commit SHA from `pull_request.merge_commit_sha`. No `pr-sha`, `pr-number`, or `merge-commit-sha` inputs are required. + +_Rationale: each mode runs on exactly one trigger, so an override input only duplicates a value already in the event payload. The per-commit merge core must remain parameter-driven (decoupled from the event source) so a future trigger — e.g. a `push`/merge-queue path that resolves the same values from the pushed commit range — can reuse it without these inputs._ From c247f05716130d8ce5ddae8a695a280646bba547 Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 13:45:53 -0500 Subject: [PATCH 19/20] update review 2 --- MANIFEST_AC_REVIEW_2.md | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md index 19e44351..c252c8ed 100644 --- a/MANIFEST_AC_REVIEW_2.md +++ b/MANIFEST_AC_REVIEW_2.md @@ -2,11 +2,11 @@ This is `MANIFEST_AC.md` annotated with implementation status for PR [#786](https://github.com/ExpediaGroup/comparadise/pull/786). -Source-only review of `action/src/` at the PR head SHA `02869c8`. (Local review HEAD was one commit ahead, `e8a6575`, whose only difference is an edit to `MANIFEST_AC.md` itself — the reviewed `action/src/` is byte-identical to the PR head.) The full test suite passes: **175 pass / 0 fail across 14 files** (`bunx nx test action`). +Source-only review of `action/src/` at the PR head SHA `da406b5`. Each criterion is marked ✅ (satisfied), ❌ (not satisfied), or ⚠️ (satisfied with a caveat worth noting), with a reason for anything not plainly satisfied. -**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. All 35 criteria are satisfied. +**Summary:** Every defect flagged in Review #1 (package-prefixed keys, `original-new-images/` writes, missing per-package manifest path) is now fixed. Of the 36 criteria, 35 are satisfied; the newly-added **4.7** (inputs derived from the triggering event, no override inputs) is **not** satisfied — the action still defines `head-sha`/`base-ref`/`pr-sha`/`pr-number`/`merge-commit-sha` and `manifest-generate` does not derive its baseline from the base branch. --- @@ -323,8 +323,22 @@ _Consistent with 2.4: a PR-Owns set that is **only** deletions instead yields su **Given** the implementation is complete **Then** ✅ the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races — _`docs/docs/setup/manifest-workflows.md:137-149` states the requirement, the rationale, and the example YAML includes it_ +### 4.7 Inputs are derived from the triggering event, not duplicated as overrides + +**Given** a manifest workflow mode runs on its documented trigger (`manifest-generate` and `manifest-compare` on `pull_request`; `manifest-merge` on `pull_request` with `types: [closed]`) +**When** the action resolves the SHAs, refs, and PR identifiers it needs +**Then** ❌ each value should be derived from the event context rather than required as a dedicated input, and the action should not define `head-sha`/`base-ref`/`pr-sha`/`pr-number`/`merge-commit-sha` inputs — _not satisfied. `action.yml` still defines all five inputs (`action.yml:56-68`). Per-bullet:_ + +- ❌ `manifest-generate` does **not** derive the baseline from the base branch — it reads only `getInput('head-sha')` (`manifest-generate.ts:14`) and, when absent, treats the baseline as empty and uploads everything (`manifest-generate.ts:61-62`); it never resolves `base.ref`/`getBranch`. The `head-sha` input is still required to get differential upload. +- ⚠️ `manifest-compare` does fall back to `pull_request.base.ref` (`run.ts:271`), so derivation works — but the `base-ref` input is still defined. +- ⚠️ `manifest-merge` does fall back to `pull_request.head.sha` / `.number` / `.merge_commit_sha` (`run.ts:320-327`), so derivation works — but the `pr-sha`, `pr-number`, and `merge-commit-sha` inputs are still defined. + +_Net: the criterion fails because the inputs are still declared (and `manifest-generate` lacks the auto-derive). The compare/merge wrappers already read from the payload, so satisfying 4.7 there is removing the now-redundant input declarations; for generate it additionally requires deriving the baseline from `base.ref`._ + --- ## Verdict -All 35 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. Two items carry minor implementation notes — 2.13 (failure surfaces via a thrown error rather than `core.setFailed`) and 3.1 (no manifest written on the intentional stale-conflict hard-failure path) — both correct and non-blocking, as noted inline. +35 of the 36 acceptance criteria are satisfied. The defects from Review #1 are resolved and the test suite is green. Two satisfied items carry minor implementation notes — 2.13 (failure surfaces via a thrown error rather than `core.setFailed`) and 3.1 (no manifest written on the intentional stale-conflict hard-failure path) — both correct and non-blocking. + +The one unsatisfied criterion is **4.7** (inputs derived from the triggering event): the action still defines the five override inputs, and `manifest-generate` does not auto-derive its baseline from the base branch. This is a newly-added requirement rather than a regression — the compare and merge wrappers already read from the event payload, so closing it is largely removing the redundant input declarations (plus adding `base.ref`-based baseline resolution in generate). From 4213d2e134449700baa7dc1ec3462b5e1daa3d61 Mon Sep 17 00:00:00 2001 From: dadajian Date: Wed, 24 Jun 2026 14:56:29 -0500 Subject: [PATCH 20/20] delete MANIFEST_AC.md to use the one on manifest-implementation as source of truth --- MANIFEST_AC.md | 326 ------------------------------------------------- 1 file changed, 326 deletions(-) delete mode 100644 MANIFEST_AC.md diff --git a/MANIFEST_AC.md b/MANIFEST_AC.md deleted file mode 100644 index d57949e8..00000000 --- a/MANIFEST_AC.md +++ /dev/null @@ -1,326 +0,0 @@ -# Manifest-Based Visual Comparison — Acceptance Criteria - -This document is the authoritative acceptance criteria for the `manifest-generate`, `manifest-compare`, and `manifest-merge` workflow modes described in `MANIFEST_PLAN.md`. It is intended for use during PR review to verify the implementation satisfies every behavioral requirement. - ---- - -## 1. `manifest-generate` mode - -### 1.1 Normal run — differential upload - -**Given** the `workflow` input is `manifest-generate`, a HEAD manifest exists at `manifests/{head-sha}.json` in S3, and `visual-test-command` completes successfully -**When** the action runs -**Then**: - -- The MD5 hash of each `new.png` in the screenshots directory is computed using Node.js `crypto` from the full-size image on disk -- Each entry is keyed by the screenshot directory's path relative to the screenshots root (prefixed with package path for monorepos) -- Only images whose hash differs from the HEAD manifest are uploaded to `new-images/{commit-sha}/path/new.png` -- If resize is enabled: changed images are resized before upload -- If resize is not enabled: changed images are uploaded full-size -- Images whose hash matches the HEAD manifest are not uploaded -- Nothing is ever written to `original-new-images/` -- A manifest is written to `manifests/{commit-sha}.json` mapping every screenshot directory path to its MD5 hash (all screenshots, not just changed ones) - -### 1.2 First run — no HEAD manifest exists - -**Given** the `workflow` input is `manifest-generate` and no manifest exists at `manifests/{head-sha}.json` in S3 -**When** the action runs -**Then**: - -- The missing manifest is treated as an empty object (no hash comparisons are performed) -- All images are uploaded to `new-images/{commit-sha}/path/new.png` following the same resize rules as 1.1 -- A manifest is written to `manifests/{commit-sha}.json` for all screenshots - -### 1.3 Hashing is always from full-size image - -**Given** any run of `manifest-generate` -**When** hashes are computed -**Then** each MD5 hash is computed from the full-size image file as it exists on disk after `visual-test-command` completes — before any resize is applied - -### 1.4 Monorepo — per-package manifest path - -**Given** the `workflow` input is `manifest-generate` and `package-paths` is non-empty -**When** the manifest is written to S3 -**Then** the manifest is uploaded to `manifests/{commit-sha}/{package-path}.json` (one file per package invocation) instead of `manifests/{commit-sha}.json` - ---- - -## 2. `manifest-compare` mode - -### 2.1 All hashes match — success - -**Given** the `workflow` input is `manifest-compare`, the PR manifest at `manifests/{pr-head-sha}.json` and the HEAD manifest at `manifests/{head-sha}.json` both exist, and every hash in the PR manifest matches the corresponding hash in the HEAD manifest -**When** the action runs -**Then**: - -- A success commit status is set on the PR head SHA -- No changeset is written -- No Comparadise comment is posted - -### 2.2 PR Owns — PR introduced a visual diff (normal case) - -**Given** at least one hash differs between the PR manifest and the HEAD manifest, and for a given differing path the HEAD hash equals the ancestor hash (PR introduced the change) -**When** the 3-way comparison runs for that path -**Then**: - -- `base.png` is downloaded from `base-images/path/base.png` -- `new.png` is downloaded from `new-images/{pr-sha}/path/new.png` (as uploaded by `manifest-generate`) -- A `diff.png` is generated via pixelmatch -- `base.png` and `diff.png` are uploaded to `new-images/{pr-sha}/path/base.png` and `new-images/{pr-sha}/path/diff.png`; if resize is enabled they are resized before upload - -_Note: "resized before upload" is satisfied implicitly when the source `base-images/` and `new-images/` objects are already at resized dimensions (both are written resized at merge/generate time), so no second resize is required here. Diff generation must tolerate a base/new dimension mismatch (e.g. by padding to a common canvas before pixelmatch) rather than assume identical dimensions, since a real visual change frequently alters a screenshot's height._ - -### 2.3 PR Owns — new screenshot (not in HEAD or ancestor) - -**Given** a path exists in the PR manifest but does not exist in either the HEAD manifest or the ancestor manifest -**When** the 3-way comparison runs for that path -**Then**: - -- No `base.png` is downloaded -- No `diff.png` is generated or uploaded -- Only `new.png` is referenced (already uploaded by `manifest-generate`) -- The path is treated as a PR-owned change (contributes to pending status and Comparadise comment) - -### 2.4 PR Owns — deleted screenshot (PR deleted, HEAD = ancestor) - -**Given** a path exists in the ancestor manifest and the HEAD manifest with the same hash, but does not exist in the PR manifest -**When** the 3-way comparison runs for that path -**Then**: - -- The deletion is logged as an informational message -- The path is recorded as a `null` entry in the changeset -- The path does not contribute to pending status or Comparadise comment - -### 2.5 Main Owns — main changed, PR is clean - -**Given** for a given differing path the PR hash equals the ancestor hash (main introduced the change, not the PR) -**When** the 3-way comparison runs for that path -**Then**: - -- No diff is generated for that path -- The path is not included in the changeset -- This path alone does not cause a pending or failure status - -### 2.6 Main Owns — screenshot added on main since branching - -**Given** a path exists in the HEAD manifest but not in the PR manifest or the ancestor manifest -**When** the 3-way comparison runs for that path -**Then**: - -- The path is treated as Main Owns (main added it, PR didn't touch it) -- No diff is generated, no failure or pending status is set for this path alone - -### 2.7 Conflict — all three hashes differ - -**Given** for a given path the PR hash, HEAD hash, and ancestor hash are all different -**When** the 3-way comparison runs for that path -**Then** the path is collected as a conflict - -### 2.8 Outcome: any conflict → failure - -**Given** at least one path was collected as a conflict -**When** the outcome is determined -**Then**: - -- A failure commit status is set on the PR head SHA -- A comment is posted listing the conflicting paths with a rebase instruction -- No changeset is written - -### 2.10 Outcome: all Main Owns (no PR Owns, no conflicts) → success - -**Given** all differing hashes are Main Owns (no PR Owns, no conflicts) -**When** the outcome is determined -**Then** a success commit status is set on the PR head SHA - -### 2.11 Outcome: at least one PR Owns, no conflicts → pending - -**Given** at least one path is PR Owns and no paths are conflicts -**When** the outcome is determined -**Then**: - -- A pending commit status is set on the PR head SHA -- A Comparadise comment is posted -- A changeset is written to `changesets/{pr-head-sha}.json` - -### 2.12 Changeset schema - -**Given** a changeset is written -**Then**: - -- The file is at `changesets/{pr-head-sha}.json` -- It contains `_headSha`: the HEAD SHA resolved in step 2 of the compare flow -- It contains one entry per PR-owned changed path with the PR's new hash as the value -- It contains one entry per PR-deleted path with `null` as the value -- Main Owns paths are not included -- The changeset contains only paths where the PR introduced a change - -### 2.13 Missing ancestor manifest - -**Given** the ancestor SHA is resolved but `manifests/{ancestor-sha}.json` does not exist in S3 -**When** the compare runs -**Then**: - -- The action fails -- An error message is returned explaining that the ancestor manifest was not found and instructing the user to ensure `manifest-generate` has run on the base branch and to rebase onto a commit that has a manifest - -### 2.14 HEAD SHA resolution - -**Given** the compare mode runs -**When** the HEAD SHA is resolved -**Then** the GitHub API is called at `GET /repos/{owner}/{repo}/branches/{base.ref}` to get the latest main HEAD SHA (not a stale cached value) - -### 2.15 Ancestor SHA resolution - -**Given** the HEAD SHA and PR SHA are known -**When** the ancestor SHA is resolved -**Then** the GitHub Compare API is called at `GET /repos/{owner}/{repo}/compare/{head-sha}...{pr-sha}` and `merge_base_commit.sha` is used as the ancestor SHA - -### 2.16 Monorepo — squash per-package manifests before comparison - -**Given** the `workflow` input is `manifest-compare` and `package-paths` is non-empty -**When** the action resolves the PR manifest -**Then**: - -- All per-package manifests at `manifests/{pr-sha}/{package-path}.json` are downloaded and merged into a single manifest -- The squashed manifest is uploaded to `manifests/{pr-sha}.json` -- The squashed manifest is used alongside the pre-existing `manifests/{head-sha}.json` and `manifests/{ancestor-sha}.json` for the 3-way comparison - ---- - -## 3. `manifest-merge` mode - -### 3.1 Manifest written for every successful merge run - -**Given** the `workflow` input is `manifest-merge` -**When** the action completes successfully (no changeset, a valid changeset, or a non-overlapping stale changeset) -**Then** a manifest is written to `manifests/{merge-commit-sha}.json`, regardless of whether a changeset exists - -**Exception — stale-conflict hard failure:** when a stale changeset has overlapping conflicts (3.5), the action fails _before_ writing a manifest. This is intentional — a manifest just declared conflicting must not be published, and the correct baseline for the overlapping paths is genuinely ambiguous. The resulting chain gap on `main` is prevented in practice by requiring the `Visual Regression` status check, which blocks a stale/overlapping PR from merging at all. Only one specific race can still reach this path, given two PRs (A and B) that both changed the same screenshot path: - -1. A is PR-Owns against base X and its visual changes are accepted in the Comparadise UI, flipping its required `Visual Regression` status to `success` — A is now mergeable. -2. B (also owning that path) merges; `main` advances to Y. B's merge run will overwrite A's status with `failure` via the overlap flag (3.3). -3. **The window** is the interval between B's merge commit landing on `main` and B's merge workflow actually writing that `failure` status onto A's head. Until it lands, A's head still shows the stale `success`, so the required check passes. -4. If A is merged inside that window (auto-merge firing, or a manual merge), A lands on top of Y; A's own merge run then sees `A.changeset._headSha` (X) ≠ A's merge parent (Y) with the shared path differing → stale conflict → fail before any manifest is written. - -The window is normally seconds, but because merge workflows are serialized by the required `concurrency` group (`cancel-in-progress: false`), a backlog of queued merges can delay B's flag and widen it. Once the flag lands, A's required check is red and A cannot merge until it rebases. - -### 3.2 No changeset — copy parent manifest - -**Given** no changeset exists at `changesets/{pr-head-sha}.json` -**When** the action runs -**Then**: - -- The parent manifest (`manifests/{parent-sha}.json`) is copied as-is to `manifests/{merge-commit-sha}.json` -- No base images are updated -- No failure status is set - -### 3.3 Conflict prevention — overlapping open PR changesets - -**Given** a changeset exists for the merging PR, and at least one other open PR has a changeset that shares at least one screenshot path key with the merging PR's changeset -**When** the merge action runs -**Then**: - -- A failure commit status is set on that other open PR's head SHA -- The failure status message indicates visual comparison is outdated and a rebase is required -- This check is performed for every open PR that has a changeset in S3 - -### 3.4 Stale changeset — no overlapping paths → proceed - -**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, but none of the changeset's screenshot keys differ between `manifests/{changeset._headSha}.json` and `manifests/{parents[0].sha}.json` -**When** the merge action runs -**Then** the merge proceeds normally (the intervening merges didn't touch the same screenshots) - -### 3.5 Stale changeset — overlapping paths → fail - -**Given** the changeset's `_headSha` does not match the merge commit's first parent SHA, and at least one changeset key has a different hash in `manifests/{changeset._headSha}.json` vs `manifests/{parents[0].sha}.json` -**When** the merge action runs -**Then**: - -- The action fails -- The conflicting paths are listed in the failure output - -### 3.6 Overlay — non-null entries update hash - -**Given** a valid (non-stale) changeset exists -**When** the changeset is overlaid onto the parent manifest -**Then** for each non-null entry in the changeset, the corresponding key in the resulting manifest has the changeset's hash value - -### 3.7 Overlay — null entries remove key - -**Given** a valid changeset contains a `null` entry for a path -**When** the changeset is overlaid onto the parent manifest -**Then** that path is absent from `manifests/{merge-commit-sha}.json` - -### 3.8 Base image update — non-null entries - -**Given** a valid changeset is applied -**When** base images are updated -**Then** for each non-null changeset entry, `new-images/{pr-sha}/path/new.png` is copied to `base-images/path/base.png` - -### 3.9 Base image update — null entries delete base image - -**Given** a valid changeset contains a `null` entry for a path -**When** base images are updated -**Then** `base-images/path/base.png` is deleted from S3 - -### 3.10 Parent SHA resolution - -**Given** the merge commit SHA is known -**When** the parent manifest is fetched -**Then** the GitHub API is called to get `parents[0].sha` of the merge commit, and `manifests/{parents[0].sha}.json` is used as the parent manifest - ---- - -## 4. General / Cross-cutting - -### 4.1 New modes do not affect existing modes - -**Given** the `workflow` input is `pr` or `merge` -**When** the action runs -**Then** behavior is identical to before this implementation — no regressions in existing modes - -### 4.2 Manifest key format — monorepo - -**Given** the action is run in a monorepo with a non-empty `package-paths` input -**When** manifest entries are keyed -**Then** each key is prefixed with the package path (e.g. `packages/ui/components/Button`, not just `components/Button`) - -### 4.3 Manifest key format — single package - -**Given** no `package-paths` input is set -**When** manifest entries are keyed -**Then** each key is the screenshot directory's path relative to the screenshots root (no prefix) - -### 4.4 S3 key structure is exact - -**Given** the implementation writes or reads any manifest, changeset, or image -**Then** the following exact S3 key patterns are used: - -- Manifests (single-package, or squashed by compare for monorepo): `manifests/{commit-sha}.json` -- Manifests (monorepo, written by generate per package): `manifests/{commit-sha}/{package-path}.json` -- Changesets: `changesets/{pr-head-sha}.json` -- New images: `new-images/{commit-sha}/path/new.png` (resized if resize is enabled, full-size otherwise) -- Base images: `base-images/path/base.png` (same dimensions as `new-images/` — resized if resize is enabled) -- `original-new-images/` is never written by any manifest mode - -### 4.5 MD5 hashing implementation - -**Given** a screenshot file is hashed -**Then** Node.js `crypto` is used to compute the MD5 hash of the full-size image file - -### 4.6 Concurrency constraint is documented - -**Given** the implementation is complete -**Then** the documentation (README, `action.yml` input descriptions, or equivalent) explicitly states that consumers using `manifest-merge` must configure a `concurrency` group with `cancel-in-progress: false` on their merge workflow to prevent concurrent merge races - -### 4.7 Inputs are derived from the triggering event, not duplicated as overrides - -**Given** a manifest workflow mode runs on its documented trigger (`manifest-generate` and `manifest-compare` on `pull_request`; `manifest-merge` on `pull_request` with `types: [closed]`) -**When** the action resolves the SHAs, refs, and PR identifiers it needs -**Then** each value is derived from the GitHub event context rather than required as a dedicated input — the action does not define `head-sha`, `base-ref`, `pr-sha`, `pr-number`, or `merge-commit-sha` inputs: - -- `manifest-generate` resolves the differential-upload baseline from the base branch (e.g. the latest base-branch HEAD via `pull_request.base.ref`); if no baseline manifest exists it is treated as empty (all images upload). No `head-sha` input is required. -- `manifest-compare` resolves the base branch ref from `pull_request.base.ref`. No `base-ref` input is required. -- `manifest-merge` resolves the PR head SHA from `pull_request.head.sha`, the PR number from `pull_request.number`, and the merge commit SHA from `pull_request.merge_commit_sha`. No `pr-sha`, `pr-number`, or `merge-commit-sha` inputs are required. - -_Rationale: each mode runs on exactly one trigger, so an override input only duplicates a value already in the event payload. The per-commit merge core must remain parameter-driven (decoupled from the event source) so a future trigger — e.g. a `push`/merge-queue path that resolves the same values from the pushed commit range — can reuse it without these inputs._