From db5dbea12e2b95974af1d4f74d064910e1a69950 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 6 Aug 2026 13:34:54 +1000 Subject: [PATCH 1/2] ci: resolve fixtures pin from the PR branch in update-snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/update-snapshots.yml | 86 +++++++++++++++++++++++--- CHANGELOG.md | 1 + 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index 2c1891a..2465a03 100644 --- a/.github/workflows/update-snapshots.yml +++ b/.github/workflows/update-snapshots.yml @@ -3,8 +3,12 @@ on: issue_comment: types: [created] -# Snapshots are generated against quantecon-book-theme-fixtures at this -# pinned SHA. Keep FIXTURES_SHA in sync with .github/workflows/ci.yml. +# Snapshots are generated against quantecon-book-theme-fixtures. The values +# 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. env: FIXTURES_REPO: QuantEcon/quantecon-book-theme-fixtures FIXTURES_SHA: d8ffc17c753ecf45fa25c6062827e1aa9de201b3 @@ -39,11 +43,29 @@ jobs: ref: ${{ steps.pr.outputs.ref }} token: ${{ secrets.GITHUB_TOKEN }} + # Must run after the PR branch is checked out. On `issue_comment` events + # the workflow definition comes from the default branch, so the top-level + # env pins main's fixtures. A PR that bumps the pin (because it needs a + # fixtures change to exercise a new feature) would otherwise regenerate + # against the OLD fixtures, produce byte-identical snapshots, and commit + # nothing — leaving the PR's visual job failing with no way to fix it. + - name: Resolve fixtures pin from the PR branch + id: pin + run: | + CI=.github/workflows/ci.yml + SHA=$(grep -oE '^[[:space:]]*FIXTURES_SHA:[[:space:]]*[0-9a-f]{40}' "$CI" \ + | head -1 | grep -oE '[0-9a-f]{40}' || true) + REPO=$(grep -oE '^[[:space:]]*FIXTURES_REPO:[[:space:]]*[^[:space:]]+' "$CI" \ + | head -1 | awk '{print $2}' || true) + echo "sha=${SHA:-$FIXTURES_SHA}" >> "$GITHUB_OUTPUT" + echo "repo=${REPO:-$FIXTURES_REPO}" >> "$GITHUB_OUTPUT" + echo "Fixtures: ${REPO:-$FIXTURES_REPO}@${SHA:-$FIXTURES_SHA}" + - name: Checkout fixtures uses: actions/checkout@v7 with: - repository: ${{ env.FIXTURES_REPO }} - ref: ${{ env.FIXTURES_SHA }} + repository: ${{ steps.pin.outputs.repo }} + ref: ${{ steps.pin.outputs.sha }} path: fixtures - name: Setup Python @@ -79,15 +101,20 @@ jobs: SITE_PATH: fixtures/_build/html - name: Commit and Push Updated Snapshots + id: commit run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" git add tests/visual/__snapshots__/ if git diff --staged --quiet; then echo "No snapshot changes to commit" + echo "committed=false" >> "$GITHUB_OUTPUT" else + CHANGED=$(git diff --staged --name-only | wc -l) git commit -m "UPDATE: Visual regression snapshots (new)" git push + echo "committed=true" >> "$GITHUB_OUTPUT" + echo "changed=$CHANGED" >> "$GITHUB_OUTPUT" fi - name: Add reaction to comment @@ -103,13 +130,20 @@ jobs: - name: Comment on PR uses: actions/github-script@v9 + 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.'; await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, - body: '✅ Missing visual snapshots have been added and committed to this PR.' + body }); # /update-snapshots — regenerates ALL snapshots (use when styling changes legitimately) @@ -142,11 +176,29 @@ jobs: ref: ${{ steps.pr.outputs.ref }} token: ${{ secrets.GITHUB_TOKEN }} + # Must run after the PR branch is checked out. On `issue_comment` events + # the workflow definition comes from the default branch, so the top-level + # env pins main's fixtures. A PR that bumps the pin (because it needs a + # fixtures change to exercise a new feature) would otherwise regenerate + # against the OLD fixtures, produce byte-identical snapshots, and commit + # nothing — leaving the PR's visual job failing with no way to fix it. + - name: Resolve fixtures pin from the PR branch + id: pin + run: | + CI=.github/workflows/ci.yml + SHA=$(grep -oE '^[[:space:]]*FIXTURES_SHA:[[:space:]]*[0-9a-f]{40}' "$CI" \ + | head -1 | grep -oE '[0-9a-f]{40}' || true) + REPO=$(grep -oE '^[[:space:]]*FIXTURES_REPO:[[:space:]]*[^[:space:]]+' "$CI" \ + | head -1 | awk '{print $2}' || true) + echo "sha=${SHA:-$FIXTURES_SHA}" >> "$GITHUB_OUTPUT" + echo "repo=${REPO:-$FIXTURES_REPO}" >> "$GITHUB_OUTPUT" + echo "Fixtures: ${REPO:-$FIXTURES_REPO}@${SHA:-$FIXTURES_SHA}" + - name: Checkout fixtures uses: actions/checkout@v7 with: - repository: ${{ env.FIXTURES_REPO }} - ref: ${{ env.FIXTURES_SHA }} + repository: ${{ steps.pin.outputs.repo }} + ref: ${{ steps.pin.outputs.sha }} path: fixtures - name: Setup Python @@ -190,16 +242,20 @@ jobs: retention-days: 30 - name: Commit and Push Updated Snapshots + id: commit run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" git add tests/visual/__snapshots__/ if git diff --staged --quiet; then echo "No snapshot changes to commit" + echo "committed=false" >> "$GITHUB_OUTPUT" else CHANGED=$(git diff --staged --name-only | wc -l) git commit -m "UPDATE: Regenerate all visual snapshots ($CHANGED files)" git push + echo "committed=true" >> "$GITHUB_OUTPUT" + echo "changed=$CHANGED" >> "$GITHUB_OUTPUT" fi - name: Add reaction to comment @@ -215,13 +271,25 @@ jobs: - name: Comment on PR with summary uses: actions/github-script@v9 + env: + COMMITTED: ${{ steps.commit.outputs.committed }} + CHANGED: ${{ steps.commit.outputs.changed }} + FIXTURES: ${{ steps.pin.outputs.repo }}@${{ steps.pin.outputs.sha }} with: script: | const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; + const committed = process.env.COMMITTED === 'true'; + const body = committed + ? `✅ Regenerated visual snapshots and committed ${process.env.CHANGED} changed file(s) to this PR.\n\n` + + `Built against fixtures \`${process.env.FIXTURES}\`.\n\n` + + `📦 [Download snapshot-update-diff artifact](${runUrl}#artifacts) to review before/after images.\n\n` + + `⚠️ The bot's commit uses \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines.` + : `ℹ️ Snapshots were regenerated but came out identical to the existing baselines, so **nothing was committed** and this PR is unchanged.\n\n` + + `Built against fixtures \`${process.env.FIXTURES}\`. If the visual job is still failing, check that this is the fixtures commit you expect — the pin is read from \`.github/workflows/ci.yml\` on this branch.\n\n` + + `📦 [Run log](${runUrl})`; await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, - body: '✅ All visual snapshots have been regenerated and committed to this PR.\n\n' + - `📦 [Download snapshot-update-diff artifact](${runUrl}#artifacts) to review before/after images.` + body }); diff --git a/CHANGELOG.md b/CHANGELOG.md index cbf38cf..f0f097b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### CI +- **`/update-snapshots` now reads the fixtures pin from the PR branch** — GitHub always runs the *default branch's* copy of a workflow on `issue_comment` events, so `update-snapshots.yml` was resolving `FIXTURES_SHA` from main rather than from the PR under test. A PR that bumps the pin (because it needs a fixtures change to exercise a new feature) therefore regenerated against the *old* fixtures, produced byte-identical baselines, committed nothing — and still posted "✅ regenerated and committed", leaving the PR's visual job failing with no way to fix it. Both jobs now resolve the pin out of the checked-out branch's `ci.yml`, falling back to the workflow-level value. The summary comments also report what actually happened (committed vs. unchanged, which fixtures commit was used) instead of unconditionally claiming success. - **CI Node bumped 20 → 24** — Node 20 reached end-of-life in April 2026, and the grouped npm updates in #400 raised engine floors (`sass-loader` 17 requires Node ≥22.11). Node 24 is the current active LTS (supported to April 2028). Applies to `ci.yml`, `docs.yml`, and `update-snapshots.yml`; `.nvmrc` and the contributor docs (`CONTRIBUTING.md`, `docs/developer/setup.md`) move to Node 24 in step so local dev matches CI. - **Stabilized flaky mobile-chrome visual tests on math-heavy pages** — after the Playwright 1.57→1.60 (Chromium) bump in #400, `math`/`proofs`/`cross-references` full-page screenshots intermittently rendered ~60px shorter than baseline, failing Playwright's dimension check before pixel tolerances apply. `waitForReady` now waits for `document.fonts.ready` and polls until the document height holds steady for 750ms, instead of a fixed 500ms sleep after MathJax typesetting. - **Cleared open Dependabot security alerts via `npm audit fix`** — `webpack-dev-server` 5.2.2→5.2.4 (GHSA-79cf-xcqc-c78w), `shell-quote` 1.8.1→1.8.4 (GHSA-w7jw-789q-3m8p, critical), `qs` 6.14.2→6.15.2 (GHSA-q8mj-m7cp-5q26), `ws` 8.18.0→8.21.0 (GHSA-58qx-3vcg-4xpx). All transitive dev/build-time only; built theme assets unchanged. The remaining `uuid` alert (GHSA-w5hq-g745-h8pq) was dismissed as not exploitable — `sockjs` only calls `uuid.v4()`, the advisory affects `v3`/`v5`/`v6` with a caller-provided `buf`. From e641151aef892de72ba166cc7a901ff5112ce2a0 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 6 Aug 2026 13:43:08 +1000 Subject: [PATCH 2/2] ci: gate update-snapshots on author association, address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/update-snapshots.yml | 24 ++++++++++++++++++------ CHANGELOG.md | 3 ++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index 2465a03..3252bfe 100644 --- a/.github/workflows/update-snapshots.yml +++ b/.github/workflows/update-snapshots.yml @@ -16,9 +16,15 @@ env: jobs: # /update-new-snapshots — only creates MISSING snapshots (safe for adding new tests) update-new-snapshots: + # The author_association check is a privilege gate, not a convenience: these + # jobs check out the PR branch and execute its code (`pip install .` runs the + # PR's build backend) with a `contents: write` token. Without it, anyone able + # to comment on a PR could trigger that. if: | github.event.issue.pull_request && - contains(github.event.comment.body, '/update-new-snapshots') + contains(github.event.comment.body, '/update-new-snapshots') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), + github.event.comment.author_association) runs-on: ubuntu-latest permissions: contents: write @@ -110,7 +116,7 @@ jobs: echo "No snapshot changes to commit" echo "committed=false" >> "$GITHUB_OUTPUT" else - CHANGED=$(git diff --staged --name-only | wc -l) + CHANGED=$(git diff --staged --name-only | wc -l | tr -d "[:space:]") git commit -m "UPDATE: Visual regression snapshots (new)" git push echo "committed=true" >> "$GITHUB_OUTPUT" @@ -133,12 +139,15 @@ jobs: env: COMMITTED: ${{ steps.commit.outputs.committed }} CHANGED: ${{ steps.commit.outputs.changed }} + FIXTURES: ${{ steps.pin.outputs.repo }}@${{ steps.pin.outputs.sha }} with: script: | const committed = process.env.COMMITTED === 'true'; + const built = `\n\nBuilt against fixtures \`${process.env.FIXTURES}\`.`; 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.'; + ? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.${built}\n\n` + + `⚠️ The bot's commit uses \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines.` + : `ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.${built}`; await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, @@ -148,10 +157,13 @@ jobs: # /update-snapshots — regenerates ALL snapshots (use when styling changes legitimately) update-all-snapshots: + # See the privilege-gate note on update-new-snapshots above. if: | github.event.issue.pull_request && contains(github.event.comment.body, '/update-snapshots') && - !contains(github.event.comment.body, '/update-new-snapshots') + !contains(github.event.comment.body, '/update-new-snapshots') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), + github.event.comment.author_association) runs-on: ubuntu-latest permissions: contents: write @@ -251,7 +263,7 @@ jobs: echo "No snapshot changes to commit" echo "committed=false" >> "$GITHUB_OUTPUT" else - CHANGED=$(git diff --staged --name-only | wc -l) + CHANGED=$(git diff --staged --name-only | wc -l | tr -d "[:space:]") git commit -m "UPDATE: Regenerate all visual snapshots ($CHANGED files)" git push echo "committed=true" >> "$GITHUB_OUTPUT" diff --git a/CHANGELOG.md b/CHANGELOG.md index f0f097b..b08f8a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### CI -- **`/update-snapshots` now reads the fixtures pin from the PR branch** — GitHub always runs the *default branch's* copy of a workflow on `issue_comment` events, so `update-snapshots.yml` was resolving `FIXTURES_SHA` from main rather than from the PR under test. A PR that bumps the pin (because it needs a fixtures change to exercise a new feature) therefore regenerated against the *old* fixtures, produced byte-identical baselines, committed nothing — and still posted "✅ regenerated and committed", leaving the PR's visual job failing with no way to fix it. Both jobs now resolve the pin out of the checked-out branch's `ci.yml`, falling back to the workflow-level value. The summary comments also report what actually happened (committed vs. unchanged, which fixtures commit was used) instead of unconditionally claiming success. +- **`/update-snapshots` now reads the fixtures pin from the PR branch** — GitHub always runs the *default branch's* copy of a workflow on `issue_comment` events, so `update-snapshots.yml` was resolving `FIXTURES_SHA` from main rather than from the PR under test. A PR that bumps the pin (because it needs a fixtures change to exercise a new feature) therefore regenerated against the *old* fixtures, produced byte-identical baselines, committed nothing — and still posted "✅ regenerated and committed", leaving the PR's visual job failing with no way to fix it. Both jobs now resolve the pin out of the checked-out branch's `ci.yml`, falling back to the workflow-level value. Both summary comments also report what actually happened (committed vs. unchanged, and which fixtures commit was built against) instead of unconditionally claiming success, and note that the bot's `GITHUB_TOKEN` commit does not itself trigger CI. +- **`/update-snapshots` is now restricted to trusted actors** — both jobs check out the PR branch and execute its code (`pip install .` runs the PR's build backend) with a `contents: write` token, but had no author-association gate, so anyone able to comment on a PR could trigger that. Both now require `OWNER`, `MEMBER`, or `COLLABORATOR`. - **CI Node bumped 20 → 24** — Node 20 reached end-of-life in April 2026, and the grouped npm updates in #400 raised engine floors (`sass-loader` 17 requires Node ≥22.11). Node 24 is the current active LTS (supported to April 2028). Applies to `ci.yml`, `docs.yml`, and `update-snapshots.yml`; `.nvmrc` and the contributor docs (`CONTRIBUTING.md`, `docs/developer/setup.md`) move to Node 24 in step so local dev matches CI. - **Stabilized flaky mobile-chrome visual tests on math-heavy pages** — after the Playwright 1.57→1.60 (Chromium) bump in #400, `math`/`proofs`/`cross-references` full-page screenshots intermittently rendered ~60px shorter than baseline, failing Playwright's dimension check before pixel tolerances apply. `waitForReady` now waits for `document.fonts.ready` and polls until the document height holds steady for 750ms, instead of a fixed 500ms sleep after MathJax typesetting. - **Cleared open Dependabot security alerts via `npm audit fix`** — `webpack-dev-server` 5.2.2→5.2.4 (GHSA-79cf-xcqc-c78w), `shell-quote` 1.8.1→1.8.4 (GHSA-w7jw-789q-3m8p, critical), `qs` 6.14.2→6.15.2 (GHSA-q8mj-m7cp-5q26), `ws` 8.18.0→8.21.0 (GHSA-58qx-3vcg-4xpx). All transitive dev/build-time only; built theme assets unchanged. The remaining `uuid` alert (GHSA-w5hq-g745-h8pq) was dismissed as not exploitable — `sockjs` only calls `uuid.v4()`, the advisory affects `v3`/`v5`/`v6` with a caller-provided `buf`.