Repository navigation
ci: remove redundant npm ci step from Security Audit workflow - #1238
Conversation
The ./.github/actions/setup-node composite action already runs `npm ci --include=dev`, and .npmrc sets include=dev, so the bare `npm ci` reinstalled an identical tree and doubled the job's slowest step. The job still runs the blocking `npm run audit` gate followed by the non-blocking dev-inclusive pass.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M3 (anthropic) — primary route · pr-reviewer-action v3.4.1
Recommendation: Approve
This is a tight, correct cleanup that removes a redundant install from a CI workflow. The removal is safe and is well-supported by the code in the diff and the surrounding context.
Change-by-change findings
1. .github/workflows/security-audit.yaml — drop the bare npm ci step (lines 15-16 of new file):
The diff also adds a 2-line explanatory comment at lines 15-16 referencing PR 1174 and explaining why the step is gone. Verified end-to-end:
- The composite action
/.github/actions/setup-node(action.yml:17) runsnpm ci --include=devandnpx prisma generate. The job therefore still installs dependencies and still generates the Prisma client before any audit step. .npmrcsetsinclude=dev, so the removed barenpm ci(had it stayed) would have reified the same dev-inclusive tree the composite action already installed. The installed tree is unchanged.- The blocking gate
npm run audit(line 21) maps topackage.json"audit", which isnpm audit --omit=dev --include=prod --audit-level=high .... The explicit--include=prodoverrides the.npmrcinclude=devprecedence (per PR 1166), so the prod-only audit gate behavior is preserved exactly. - The non-blocking informational pass
npm audit --include=dev ...(lines 26-27) still runs withcontinue-on-error: true, so dev-only advisories remain visible without turning main red. - Final post-change step order: checkout → setup-node →
npm run audit(blocking) →npm audit --include=dev(non-blocking). Matches the PR body's claim.
Sources
.github/workflows/security-audit.yaml(post-change): the diff.github/actions/setup-node/action.yml— confirmed the composite runsnpm ci --include=dev+npx prisma generatepackage.json"scripts.audit"— confirmed--omit=dev --include=prod --audit-level=highpackage.json"//".auditcomment +package-audit.test.ts— regression guard for the audit-scoping invariant (PR 1166); comment notes that CI installs pass--include=devexplicitly via the composite, which is exactly the model this PR relies onSECURITY-ACCEPTED-RISKS.md(line 57, 74) — independently documents the same audit scoping and the non-blocking dev-inclusive passDockerfile(lines 22-25) — analogous install scoping commentary references PR 1166, consistent with the audit script's behavior
Claim falsification summary
- Claim 1 (composite action runs
npm ci --include=dev; removed barenpm ciwas redundant): held.action.yml:17showsrun: npm ci --include=dev. A repo-wide grep fornpm cishows the only other workflow occurrences are comments (pr-smoke.yaml:80 explains why it deliberately does not use the composite; the security-audit.yaml comments are explanatory). No other workflow carries a redundant install. - Claim 2 (audit scoping is independent of the install step because
scripts.auditpasses explicit--include=prod): held.package.json"audit" isnpm audit --omit=dev --include=prod --audit-level=high .... The--include=prodcancels the.npmrcinclude=devper documented npm precedence, and this is regression-guarded bypackage-audit.test.ts. - Claim 3 (four invariants — install still runs, audited tree unchanged, blocking-then-non-blocking order preserved, install-once rule in other workflows): held 4/4.
- Claim 4 (PR-stated invariants) and Claim 5 (local validation receipts): receipts only — author claims
yamlparsed OK andactionlint 1.7.12clean. The PR's CI run on this commit shows theWorkflow lintcheck as success, which is the canonical actionlint gate for the repo (.github/workflows/ci.yaml:24-31runs actionlint 1.7.12).
Standards Compliance
AGENTS.md"Lint/typecheck blocks CI" is satisfied for the change surface: the file touched is YAML, the repo's lint check isnpm run lint(which does not lint YAML), and the dedicated workflow-lint gate (Workflow lintjob inci.yaml, actionlint 1.7.12) is the authoritative check for workflow files. The PR's CI run showsWorkflow lintas success, so this obligation is satisfied.- No secrets are logged, echoed, or persisted by this change.
- The change follows the install-once convention already used by
ci.yaml(every job uses./.github/actions/setup-nodeand does not follow up with a separate install).
Linked Issue Fit
Issue PR 1174 asks to drop the redundant npm ci from the Security Audit workflow, and explicitly states the expected file is .github/workflows/security-audit.yaml. The PR matches exactly: that file is the only file changed, and the redundant step is removed. The issue also asks to confirm the job still ends with npm run audit (blocking) followed by the non-blocking dev-inclusive pass; the post-change workflow preserves both.
Tool Harness Findings
The harness attempted 11 read-only calls. One — a read_file on .npmrc — returned a sensitive-file block marker (.npmrc is in the repo's sensitive-file list and was redacted, not exposed). The substantive .npmrc content (include=dev and the precedence comment) is corroborated independently by:
package.json"//".audit(describes the precedence)package-audit.test.ts(regression guard mentions.npmrcinclude=dev)Dockerfile:22-25(parallel install-scoping comment)SECURITY-ACCEPTED-RISKS.md:73,74(resolves the "invalid omit config" incident and references the same precedence)
The redacted fetch therefore does not leave any invariant unverified.
Unknowns / Needs Verification
None blocking. The two operational notes in the PR body — that the Workflow lint job (actionlint) is the right gate for this change, and that the operator should delete two junk repo labels (type/chrome] and body) — are not within the review surface and need no review-time decision.
Closes #1174
What
Drop the standalone
- run: npm cistep from the Security Audit workflow. The./.github/actions/setup-nodecomposite action already runsnpm ci --include=dev(.github/actions/setup-node/action.yml:17), so the barenpm cireinstalled an identical tree and doubled the job's slowest step.Verified against the current code (not memory):
.npmrcsetsinclude=dev, so the removed barenpm ciresolved the same tree as the composite action'snpm ci --include=dev— no change to the audited tree.scripts.auditpasses an explicit--include=prodto override the.npmrcinclude=devprecedence (CI: Security Audit failing on the default branch (npm audit) #1166), so the blocking gate behavior is unchanged.Invariants this change must keep, and how each is kept
./.github/actions/setup-node), which runsnpm ci --include=devandnpx prisma generate; it is untouched..npmrcinclude=devmade the removed barenpm ciinstall the same dev-inclusive tree; the blocking pass still overrides scope via--include=prodinscripts.audit, and the informational pass still passes--include=devexplicitly.npm run auditfollowed by the non-blocking dev-inclusive pass. Verified by parsing the post-change workflow: step order is checkout → setup-node →npm run audit→npm audit --include=dev(continue-on-error: true).npm cinow shows it only in the composite action itself and explanatory comments (pr-smoke.yaml:80). No other workflow carried a redundant install.Local validation (receipts)
yaml(npmyaml@2) parse of.github/workflows/security-audit.yamlin/workspace: OK; parsedjobs.audit.stepsorder confirmed as above.actionlint 1.7.12(exact version CI pins in.github/workflows/ci.yaml) on the changed file and on the full repo: no findings.npm run lint/npm run typechecknot run: the change is workflow YAML only; no TS/source files touched, and CI's actionlint job is the check that covers this file.Remaining external verification: the Security Audit workflow run on this PR (CI), observed by the operator.
Note from the issue thread (not code, not addressed here): two junk repo-level labels (
type/chrome]andbody) from a tooling mislabel remain in the repo and need an operator to delete them; no MCP label-delete capability was available in this run.