docs: add implementation plan for manifest-based visual comparison#773
docs: add implementation plan for manifest-based visual comparison#773danadajian wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| 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` |
There was a problem hiding this comment.
When resolving the latest main HEAD, that commit's manifest-merge job might still be running. What do you think about adding a retry with exponential backoff, 3-minute max timeout? Fail with a clear error if it's still missing.
There was a problem hiding this comment.
Each manifest-merge job will be executed serially, as we will instruct consumers to use concurrency in their merge workflows such that only one job can run at a time. I noted this here in the plan but I'll make it more clear
| - 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`. |
There was a problem hiding this comment.
If manifest-merge fails and never writes its manifest, every subsequent PR's manifest-compare is now blocked. It resolves HEAD, can't find the manifest, retries for 3 minutes, and fails. This is not self-healing.
Option 1: Instead of requiring the exact HEAD SHA's manifest, walk back through recent commits on the base branch until a manifest is found. This keeps PRs unblocked while making the gap visible in logs. The fallback manifest might be one commit stale, but for our purposes how likely is that to matter? Those changes already merged successfully.
Option 2: If we want a stronger guarantee, does requesting a missing SHA’s manifest kick off a new manifest-merge job for HEAD~1 SHA and HEAD~2?
There was a problem hiding this comment.
To be honest, I would rather fail loudly than silently if there is a missing manifest. If we implement either Option 1 or 2 we invite silent failures to infect the system.
For this first iteration, the only way manifest-merge can fail is if there is a GitHub outage or an AWS outage. If either occurs, we have bigger problems and I'd rather halt the system entirely than build bandaids that obfuscate the problem
erikkrietsch
force-pushed
the
manifest-plan
branch
from
11d5faf to
52510fb
Compare
erikkrietsch
force-pushed
the
manifest-plan
branch
from
d8b6bcf to
f4a0e70
Compare
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…three modes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
📝 Description
🔗 Related Issues