diff --git a/MANIFEST_AC_REVIEW_1.md b/MANIFEST_AC_REVIEW_1.md new file mode 100644 index 00000000..38b90556 --- /dev/null +++ b/MANIFEST_AC_REVIEW_1.md @@ -0,0 +1,319 @@ +# 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` (`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 + +### 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_ + +### 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 + +### 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 `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."_ + +### 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` (`manifest-compare.ts`) 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`_ + +### 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 + +### 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 (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) +- ❌ `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`_ diff --git a/MANIFEST_AC_REVIEW_2.md b/MANIFEST_AC_REVIEW_2.md new file mode 100644 index 00000000..c252c8ed --- /dev/null +++ b/MANIFEST_AC_REVIEW_2.md @@ -0,0 +1,344 @@ +# 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 `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. 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. + +--- + +## 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 — _`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 + +**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` (`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 + +### 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 + +**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_ + +--- + +## 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 — _`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) + +**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` 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`. 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) + +**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) + +**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 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 — _`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 + +**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 + +**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 + +**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 — _`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`_ + +### 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 — _`prOwns.length === 0` ⇒ success status (`manifest-compare.ts:78-89`)_ + +### 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 + +**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 + +**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`); `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 + +**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 + +**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 + +**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_ + +--- + +## 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 — _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 + +**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 + +**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 + +**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 + +**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 + +**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 + +**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 + +**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 + +**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 + +**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_ + +--- + +## 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 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 + +**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 + +**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 + +**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 + +**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 + +**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 + +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). diff --git a/MANIFEST_PLAN.md b/MANIFEST_PLAN.md new file mode 100644 index 00000000..6d0799cb --- /dev/null +++ b/MANIFEST_PLAN.md @@ -0,0 +1,130 @@ +# 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/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 +``` + +## File Schemas + +### Manifest (`manifests/{commit-sha}.json`) + +```json +{ + "components/Button": "d41d8cd98f00b204e9800998ecf8427e", + "components/Modal": "7d793037a076d2e1f3eb15d3a5e4389a", + "pages/Home": "098f6bcd4621d373cade4e832627b4f6" +} +``` + +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": "a3c2f8d1b4e6a9c7d2f0e1b3a5c7d9f1", + "components/Removed": 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. `_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 + +### `manifest-generate` mode + +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`; if resize enabled, resize before upload +5. 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 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 + - 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) + - Includes: screenshot added on main since branching (in HEAD only) + - **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 + - 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) + - 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. 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: 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) +8. Overlay changeset onto parent manifest: + - Non-null entries: update hash + - Null entries: remove key +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 + +- **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 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). + 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 + +- 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