ci: resolve fixtures pin from the PR branch in update-snapshots - #424
Merged
Conversation
On `issue_comment` events GitHub always runs the workflow definition from the default branch, so `FIXTURES_SHA` in the top-level `env:` was main's pin, not the pin of the PR under test. The failure mode this produces is silent and self-defeating. A PR that bumps the fixtures pin — which is exactly what a PR adding a theme option must do, since the fixtures site has to set that option to exercise it — gets its snapshots regenerated against the OLD fixtures. Those come out identical to the existing baselines, so nothing is committed, while the PR's own visual job (which does use the PR's pin) keeps failing. Re-running the command can never help. Observed on #404, where the bot reported success and changed nothing. Both jobs now read the pin out of the checked-out branch's ci.yml, falling back to the workflow-level value if it can't be found. The summary comments report what actually happened — committed vs. unchanged, and which fixtures commit was built against — rather than unconditionally claiming success, and the committed case notes that a GITHUB_TOKEN commit does not itself trigger CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #424 +/- ##
=======================================
Coverage ? 47.75%
=======================================
Files ? 2
Lines ? 423
Branches ? 0
=======================================
Hits ? 202
Misses ? 221
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a CI correctness bug in the /update-snapshots GitHub Actions workflow: because issue_comment workflows always run from the default branch, the workflow-level FIXTURES_SHA fallback was silently using main’s fixtures pin instead of the PR branch’s pin, making snapshot regeneration ineffective for PRs that bump fixtures.
Changes:
- Re-resolve
FIXTURES_SHA/FIXTURES_REPOafter checking out the PR branch by extracting the values from the PR branch’s.github/workflows/ci.yml, with fallback to workflowenv. - Improve PR summary comments to reflect whether anything was actually committed (and include fixtures provenance for
/update-snapshots). - Document the behavior change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
CHANGELOG.md |
Documents the CI fix for /update-snapshots fixtures pin resolution and improved reporting. |
.github/workflows/update-snapshots.yml |
Resolves fixtures pin from the checked-out PR branch and improves snapshot commit/reporting behavior. |
Suppressed comments (2)
.github/workflows/update-snapshots.yml:142
- Same security concern for the
/update-snapshotsjob: it’s runnable by any commenter and then executes PR-branch code with write permissions. Add anauthor_associationgate (or equivalent) to the job-levelif:so only trusted users can trigger it.
const committed = process.env.COMMITTED === 'true';
const body = committed
? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.`
: 'ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.';
await github.rest.issues.createComment({
.github/workflows/update-snapshots.yml:255
- Same
wc -lwhitespace issue here:CHANGEDwill likely have leading spaces, which then shows up in the commit message and PR summary. Trim whitespace when computing the count.
CHANGED=$(git diff --staged --name-only | wc -l)
git commit -m "UPDATE: Regenerate all visual snapshots ($CHANGED files)"
Comment on lines
+7
to
+11
| # below are only a FALLBACK: on `issue_comment` events GitHub always runs the | ||
| # copy of this workflow that is on the default branch, so these would pin the | ||
| # fixtures main uses rather than the ones the PR under test uses. Each job | ||
| # re-reads the pin out of the checked-out PR branch's ci.yml instead — see the | ||
| # "Resolve fixtures pin" step. |
Contributor
Author
There was a problem hiding this comment.
Agreed, and fixed in e641151 — both jobs now require OWNER, MEMBER, or COLLABORATOR. This is pre-existing rather than introduced here (I'd noted it in the PR description as a follow-up), but you're right that it's worth closing while the file is open. The exposure is real: the jobs check out the PR branch and pip install . runs that branch's build backend, all under a contents: write token.
Comment on lines
+133
to
+141
| env: | ||
| COMMITTED: ${{ steps.commit.outputs.committed }} | ||
| CHANGED: ${{ steps.commit.outputs.changed }} | ||
| with: | ||
| script: | | ||
| const committed = process.env.COMMITTED === 'true'; | ||
| const body = committed | ||
| ? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.` | ||
| : 'ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.'; |
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in e641151 by adding the pin to the /update-new-snapshots comment rather than weakening the wording, since knowing which fixtures commit produced a baseline is useful in both jobs. The CHANGELOG entry now says "both summary comments" explicitly.
| echo "No snapshot changes to commit" | ||
| echo "committed=false" >> "$GITHUB_OUTPUT" | ||
| else | ||
| CHANGED=$(git diff --staged --name-only | wc -l) |
Contributor
Author
There was a problem hiding this comment.
Applied in e641151 (| tr -d "[:space:]"), though the premise needs one correction: GNU coreutils wc -l does not pad when reading from a pipe, and these jobs run on ubuntu-latest, so the value was already clean in practice. BSD wc does pad — I checked locally and got [ 3] — so the trim is worth having for robustness, and the count is interpolated into both a commit message and a PR comment where a stray space would show. Same fix applied at both sites.
Contributor
🎭 Visual Regression Test Results
Details
Skipped testsdesktop-chrome › theme.spec.ts › Visual Regression Tests › prob-matrix - full page screenshot |
Address Copilot review on #424. Add an author_association gate (OWNER/MEMBER/COLLABORATOR) to both jobs. They check out the PR branch and execute its code — `pip install .` runs the PR's build backend — with a `contents: write` token, and had no gate at all, so any user able to comment on a PR could trigger that. Pre-existing, but worth closing while the file is open. Report the resolved fixtures pin in the /update-new-snapshots summary too. Only the /update-snapshots job did, which made the CHANGELOG's claim that the summary comments name the fixtures commit true of just one of the two. Trim `wc -l` when computing the changed-file count. GNU coreutils does not pad for piped stdin so this never bit on ubuntu-latest, but BSD wc does, and the value is interpolated into a commit message and a PR comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 6, 2026
mmcky
added a commit
that referenced
this pull request
Aug 6, 2026
…425) * ci: verify regenerated snapshots and let the bot's commit trigger CI Addresses #358. Two problems, one symptom — regenerated baselines reaching a PR with nothing having checked them and no CI run to notice. Verify before committing. The workflow wrote new baselines and pushed them with nothing establishing they were reproducible. Both jobs now re-run the suite without --update-snapshots against the same built site. Passing shows the render is deterministic; failing means the images just written already don't reproduce, which is non-deterministic rendering rather than a stale baseline, and is worth knowing before the commit lands rather than after it reaches main. Make the push able to trigger CI. Commits pushed with GITHUB_TOKEN never raise push/pull_request events, so the baselines landed while the PR's visual check still showed its previous failure and the author had to push an empty commit by hand. Both checkouts now use `secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN`, so setting that secret makes CI re-run automatically while leaving the workflow working unchanged when it is absent. The summary comments report which path was taken and what, if anything, the author still needs to do. Note that #358's first stated cause is stale: the commit messages no longer carry `[skip ci]`. The GITHUB_TOKEN event-suppression rule is the whole of it. Docs cover the new secret, the verification step, and correct two points that #424 changed — ci.yml is now the source of truth for the fixtures pin, and the pin is the one thing read from the PR branch rather than the default branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop the verify run wiping the snapshot diff artifact Address Copilot review on #425, plus two defects found reviewing the result. Report the verify outcome in every case, not only when something was committed. The verify step has no `if:`, so it always runs; suppressing its result when nothing was committed hid the most useful signal of all — for /update-new-snapshots, a failure with nothing committed means the PR has real visual regressions. Give the verify run its own output directory. Playwright clears its output dir as the first task of every run, so the verify run was deleting the regeneration run's images out of test-results/ — precisely the content the snapshot-update-diff artifact exists to carry. The artifact would have uploaded nothing while the comment still linked to it, with only a buried upload-artifact warning as signal. Verified by repro: a sentinel file in test-results/ does not survive a plain `playwright test`, and does survive `playwright test --output=test-results-verify`. The upload now covers both directories, so a verify failure stays diagnosable. Stop asserting a cause the workflow cannot know. /update-new-snapshots runs --update-snapshots=missing, which creates only absent baselines and leaves existing-but-differing ones failing, while the verify run covers the whole suite. So `committed && !verified` there does not imply the new snapshots are at fault — it is reachable through the repo's own documented fixtures-bump procedure, where a bump seeds one new baseline correctly and perturbs unrelated existing pages. That job's message now gives both possibilities; the regenerate-all job, where every baseline was rewritten, still states the specific cause because there it is the only one available. Docs and CHANGELOG follow, and no longer imply the verification gates the commit. It reports; the commit happens either way, which is deliberate — the images stay available to inspect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
update-snapshots.ymlis triggered byissue_comment, and GitHub always runs the default branch's copy of a workflow for that event. So theFIXTURES_SHAin its top-levelenv:was always main's pin — never the pin of the PR under test.This produces a silent, self-defeating failure. A PR that bumps the fixtures pin — which is exactly what a PR adding a theme option has to do, since the fixtures site must set that option to exercise it — has its snapshots regenerated against the old fixtures. Those come out byte-identical to the existing baselines, so nothing gets committed, while the PR's own
visualjob (which does use the PR's pin, becausepull_requestevents run the PR's workflow file) keeps failing. Re-running/update-snapshotscan never fix it.It happened on #404. Run 27820556433 logged
FIXTURES_SHA: d8ffc17c…(main's pin) while the PR branch pins32763e43…, ended withNo snapshot changes to commit— and still posted "✅ All visual snapshots have been regenerated and committed to this PR." Nothing was committed; that PR has been stuck at 92 failing visual tests since 19 June.The fix
Both jobs now resolve the pin from the checked-out branch's
ci.ymlafter the PR checkout, falling back to the workflow-level value if it can't be found. This adds no trust surface — the workflow already checks out and executes PR-branch code (pip install ., then Playwright).Verified the extraction against the real files: main's
ci.ymlresolves tod8ffc17c…, #404's resolves to32763e43…, and a file with no match falls back to the workflow default.The summary comments also now report what actually happened — committed vs. unchanged, and which fixtures commit was built against — instead of unconditionally claiming success. The committed branch additionally notes that a
GITHUB_TOKENcommit does not itself trigger workflows, so an empty commit is needed to re-run CI against the new baselines.Why not just bump the pin on main instead
That was the first idea, and it doesn't work. The fixtures commit adds
html_theme_options.announcement, an option main's theme doesn't support yet; Sphinx warns on unknown theme options and the fixtures build runsjb build . --warningiserror, so bumping the pin standalone would break main's CI. The pin and the feature have to land together — which is precisely why the bot needs to read the PR's pin.Note
Unrelated to this change, but found while in the file:
update-snapshots.ymlhas no author-association gate. Any user who can comment on a PR can trigger a job that checks out PR-branch code and runs it with acontents: writetoken. Worth a follow-up.🤖 Generated with Claude Code