Claude Code PR Review

PR: #238 • Head: bc587ba • Reviewers: stack:code-review

Summary

Hardens the committed .npmrc with supply-chain directives (ignore-scripts=true, save-exact=true, engine-strict=true, etc.), upgrades esbuild 0.10.2 → 0.19.12 (exact-pinned), and adds npm rebuild esbuild --ignore-scripts=false follow-ups to the patched CI install paths (GH test.yml build job, CircleCI dependencies job) plus the standard-version postbump chain; documents the new local-dev steps in the developer guide.

Review Table

Findings

• File: package.json:68

• Severity: Critical

• Reviewer: stack:code-review

• Issue: The standard-version postbump runs npm ci && npm rebuild esbuild … && npm run sri-update, but with ignore-scripts=true the npm ci no longer triggers the repo's prepare script (husky && npx patch-package). patches/colorjs.io+0.4.3.patch patches colorjs.io, which is bundled into axe.js (lib/core/imports/index.js:8) — so the release-built artifact silently bundles unpatched colorjs.io.

• Suggestion: Insert npm run prepare (or at minimum npm run patch) after npm ci in postbump: npm ci && npm run prepare && npm rebuild esbuild --ignore-scripts=false --foreground-scripts && npm run sri-update && git add doc/rule-descriptions.md.

• File: .github/workflows/update-generated-files.yaml:26

• Severity: High

• Reviewer: stack:code-review

• Issue: This workflow runs npm ci + npm run build and auto-commits generated files, but was not given the npm rebuild esbuild + npm run prepare follow-ups added to test.yml — same unpatched-colorjs.io problem, with the output committed back to the repo. (File not in this PR's diff.)

• Suggestion: Add the same two steps after npm ci as in test.yml:21-22.

• File: a11y-engine-core/build/scripts/build_axe.sh (umbrella repo — cross-repo)

• Severity: High

• Reviewer: stack:code-review

• Issue: The umbrella repo's engine build does npm install + npm run build inside the axe-core/ submodule with no prepare/rebuild step. Once this .npmrc lands in the submodule, every a11y-engine-core build bundles unpatched colorjs.io (loses the CSS?.supports guard affecting color-contrast paths). Cannot be fixed in this PR.

• Suggestion: Sequence a coordinated AXE-3443/AXE-3444 change to build_axe.sh (add npm run prepare && npm rebuild esbuild --ignore-scripts=false) before bumping the submodule pointer, and state the dependency explicitly in the PR description.

• File: .github/workflows/release.yml:26

• Severity: Medium

• Reviewer: stack:code-review

• Issue: The release job's correctness depends entirely on the postbump chain in package.json for prepare/rebuild. (File not in this PR's diff.)

• Suggestion: After fixing postbump, also add an explicit npm run prepare to the release workflow so artifact correctness doesn't hinge on standard-version config.

• File: .npmrc:10

• Severity: Medium

• Reviewer: stack:code-review

• Issue: engine-strict=true enforces every dependency's engines range at install time, turning previous warnings into hard install failures. CI pins Node 22 via .nvmrc, but the umbrella repo's default toolchain is Node 18.20.4 — a dev running npm install inside axe-core/ on Node 18 may now hard-fail, and build_axe.sh's nvm use becomes load-bearing.

• Suggestion: Confirm this is deliberate (the developer guide already mandates Node 22+); if so, note it in the umbrella repo's setup docs.

• File: .circleci/config.yml:207,223

• Severity: Medium

• Reviewer: stack:code-review

• Issue: npm install w3c/wcag-act-rules#main / w3c/aria-practices#main are git dependencies whose own prepare scripts are skipped under ignore-scripts=true. Both look content-only today, but the test:act/test:apg jobs were given no follow-up step. (Lines not in this PR's diff.)

• Suggestion: Confirm both CircleCI jobs pass on this branch; add a targeted --ignore-scripts=false to those installs if they break.

• File: doc/developer-guide.md:37

• Severity: Low

• Reviewer: stack:code-review

• Issue: "fetches the esbuild bundler binary" is slightly inaccurate — with esbuild ≥0.16 the platform binary arrives as an @esbuild/* optionalDependency during npm ci; the rebuild validates/links it. The documented command also omits --foreground-scripts used everywhere else.

• Suggestion: Reword to "validates and links the esbuild platform binary" and include the full flag set for consistency.

Positive notes from the reviewer: the esbuild 0.10 → 0.19 jump itself is safe (the sole invocation in build/tasks/esbuild.js uses only options stable across the 0.17 breaking changes); the CircleCI rebuild line is correctly placed before save_cache so downstream jobs inherit the rebuilt binary; CircleCI's build job already runs npm run prepare explicitly, so the patch-package gap does not apply there; submodule // [tag]: rule treated as satisfied (no JS source changed, no sensible anchor for inline tags, no sensitive info in .npmrc comments, no Deque packages).

Reviewer questions for the PR thread: (1) Has a full npm ci && npm run prepare && npm run build && npm test been run from a clean clone on this branch? (2) Was a release dry-run executed to prove the postbump chain end-to-end under ignore-scripts? (3) Is the umbrella build_axe.sh change merged/sequenced before the submodule pointer bump?

Verdict: FAIL — the release postbump chain and two other install→build paths silently lose the bundled colorjs.io patch under ignore-scripts=true; fix postbump and update-generated-files.yaml in this PR and sequence the umbrella build_axe.sh change before the pointer bump.