diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index 2c1891a..3252bfe 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 @@ -12,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 @@ -39,11 +49,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 +107,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 | tr -d "[:space:]") 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,21 +136,34 @@ jobs: - name: Comment on PR 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 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.${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, 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) 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 @@ -142,11 +188,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 +254,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) + 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" + echo "changed=$CHANGED" >> "$GITHUB_OUTPUT" fi - name: Add reaction to comment @@ -215,13 +283,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..b08f8a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +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. 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`.