docs(npmrc): CAUTION for include=dev precedence over --omit=dev (fix landed via #1166/#1172)

[itsmiso-ai]

Originally this PR fixed #1162 (npm run audit auditing the dev tree because .npmrc include=dev overrides --omit=dev, exposing the dev-only, unfixable braces * advisory GHSA-vfj7-8cjw-p6xm) via --include=optional.

While open, main landed the equivalent fix via #1166 (merged #1172): --include=prod in scripts.audit, guard test package-audit.test.ts, plus a non-blocking dev-inclusive informational audit step. Issue #1162 closed completed with that merge.

I merged origin/main and resolved conflicts in favor of main for package.json and SECURITY-ACCEPTED-RISKS.md. Remaining delta: one added CAUTION comment line in .npmrc (which main lacks), documenting the include=dev precedence footgun at its source and pointing at the landed --include=prod fix (#1166). Comment-only; include=dev remains the sole config line.

Invariants kept:

1. npm run audit gates only prod+optional; dev-only advisories do not fail the gate - main's --omit=dev --include=prod script, untouched here; verified locally: found 0 vulnerabilities.
2. npm ci installs devDeps despite a global omit=dev (chore: fix npmrc dev dependency config #428) - include=dev unchanged; only a # comment added.
3. .npmrc stays valid, no invalid-config noise (chore: fix npmrc dev dependency config #428) - no omit= key introduced.
4. Comment matches landed implementation - cites --include=prod, package.json "//".audit, SECURITY-ACCEPTED-RISKS.md, CI: Security Audit failing on the default branch (npm audit) #1166; independent review confirmed all other files byte-exact with main.

Other code paths enforcing the same rule (unchanged, consistent): package-audit.test.ts (6/6 pass on this tree; full suite 3789 passed); security-audit.yaml blocking + informational steps; .github/actions/setup-node (npm ci --include=dev); Dockerfile prod-deps (npm ci --omit=dev, never copies .npmrc).

[itsmiso-ai]

Heads-up: your commit 7a6ad35 was cherry-picked into #1168 (as c506e1e) so that PR can show green CI independently — the npm audit check there was red on the same pre-existing braces root cause and blocked its review-readiness. Whichever PR merges second carries an empty delta; close this one as merged-via-#1168 if that ordering suits.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: glm-5.3-flash (openai) — routed smart (risk match: linked_priority_p1) · pr-reviewer-action v3.2.0

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Findings (1 minor, 1 info)

Severity	Location	Finding
Minor		The PR body's Fix section and invariants describe the superseded `--include=optional` approach (commit 7a6ad35, cherry-picked into PR 1168), but the fix that actually landed via PR 1166/1172 uses `--include=prod` in scripts.audit — the body no longer matches the change it documents, though the diff'
Info	.npmrc:4	The added CAUTION comment is accurate and consistent with the landed state: scripts.audit does pass an explicit --include=prod (package.json line 12), the "//".audit rationale exists, SECURITY-ACCEPTED-RISKS.md documents the scoping, and package-audit.test.ts regression-guards the flag pair.

Recommendation: Approve

This PR is a one-line documentation comment in .npmrc recording why include=dev is kept despite its interaction with --omit=dev, now that the actual audit-gate fix has landed separately (per the repository history: commit f683575, "fix(ci): scope the npm audit gate to production dependencies", landed via PR 1166/1172; an earlier --include=optional attempt, commit 7a6ad35, was cherry-picked into PR 1168 and superseded). The comment is factually accurate against the landed state, CI is fully green on the head commit (including the blocking npm audit check), and the linked priority/p1 issue is resolved. The only real problem is that the PR body still describes the superseded --include=optional approach — a description staleness, not a code defect.

Change-by-change findings

.npmrc (+1 line: CAUTION comment above include=dev)

• The comment states: include=dev overrides --omit=dev (npm config precedence), which is why scripts.audit passes an explicit --include=prod, with pointers to the "//".audit note in package.json, SECURITY-ACCEPTED-RISKS.md, and the fix PR. Every element checks out:
  • package.json scripts.audit (line 12) is npm audit --omit=dev --include=prod --audit-level=high ... — the comment's flag name matches the landed code.
  • package.json "//".audit documents exactly this rationale, including that the CLI include value replaces the .npmrc value so --omit=dev takes effect.
  • SECURITY-ACCEPTED-RISKS.md carries both the accepted-risk row for the dev-only braces advisory (GHSA-vfj7-8cjw-p6xm, all versions, no patched release, dev lint chain only) and a resolution-history row explaining the .npmrc include=dev vs --omit=dev interaction and the --include=prod fix.
  • package-audit.test.ts regression-guards the mechanism: it asserts the script contains --omit=dev, --include=prod, --audit-level=high, the exact adjacent pair --omit=dev --include=prod, that the "//".audit rationale exists, and that the dev-inclusive audit step in the workflow stays continue-on-error: true.
  • .github/workflows/security-audit.yaml matches: a blocking npm run audit gate plus a non-blocking dev-inclusive npm audit --include=dev visibility step, with a comment explaining the same precedence issue.
• include=dev itself is unchanged, preserving its stated purpose (clean-checkout npm ci installs devDeps); the composite setup-node action additionally passes --include=dev explicitly, so the install path is robust even independent of .npmrc.
• The Dockerfile is unaffected: its deps/prod-deps stages copy only package.json/package-lock.json, so .npmrc never reaches the image build and npm ci --omit=dev in prod-deps behaves as before.

PR body accuracy (the one finding): the body's "Fix" section and invariants 1–2 say scripts.audit passes --include=optional and cite a sharp verification command using that flag. That describes the superseded commit 7a6ad35. The landed fix uses --include=prod. The substantive invariants still hold under the landed flags (see claim tally below), and the PR title itself says the fix landed via PR 1166/1172 — but the body text is stale and should not be treated as the mechanism of record. The diff's comment is the correct record.

Claims to falsify — tally

• Claim 1 (.npmrc sets include=dev intentionally so npm ci installs devDeps): held, 1/1 — the diff retains include=dev; the setup-node action passes --include=dev explicitly; package-audit.test.ts comments confirm the intent.
• Claim 2 (scripts.audit passes --include=optional; CLI include replaces the project-level list): violated as written, 1 of 1 items — the landed script passes --include=prod, not --include=optional (package.json line 12). The underlying mechanism claim (CLI include replaces, rather than unions with, the project-level include) is corroborated by the "//".audit note and the green audit gate, but the flag name in the claim is stale. Reported as the minor docs finding.
• Claim 3 (audit gates only prod + optional; dev-only advisories must not fail CI): held in substance — under the landed --omit=dev --include=prod pair the dev tree is excluded (the blocking npm audit check succeeded on head despite the unpatched dev-only braces advisory being present in the dev tree — a strong counterexample-absence signal), and the dev-inclusive audit step is explicitly non-blocking. The claim's cited mechanism (--include=optional) is stale, same as Claim 2.
• Claim 4 (.npmrc include=dev left unchanged; only a CAUTION comment added): held, 1/1 — the diff is exactly one added comment line; no config value changed.
• Claim 5 (no invalid-config warning noise; .npmrc not reverted to the old empty omit= form): held on corpus evidence — .npmrc was not reverted; SECURITY-ACCEPTED-RISKS.md's resolution history corroborates that the empty omit= form was previously fixed (to omit=dev, later include=dev). The sub-claim that precedence was "verified empirically" on npm 11.19 could not be re-run in this review, but nothing in the corpus contradicts it.

Items the claims missed: I checked the sibling consumers the body lists and they all check out — the setup-node composite action (npm ci --include=dev), the security-audit workflow (blocking npm run audit + non-blocking dev-inclusive step), and the Dockerfile (no .npmrc copied; --omit=dev unaffected). A repo-wide grep for include=dev|omit=dev|include=prod|include=optional|npm audit found no consumer outside these files plus the regression test and the risk-register doc. No unlisted counterexample found.

Linked Issue Fit

The linked issue (priority/p1, type/bug, now status/done) reported the Security Audit workflow failing repeatedly on main because npm audit --omit=dev reported 5 high findings in the dev-only eslint-config-next → fast-glob → micromatch → braces chain. This PR closes that issue by documenting the fix's rationale at the point of future confusion (.npmrc), which is where a well-meaning revert would re-break the gate. The fix itself landed in the merged PR 1166/1172 commit already present in the base; the head commit is a merge of main into the branch, and the remaining delta is exactly the comment. Fit is correct: the issue's root cause, the failing command line in the log, and the documented fix all align.

Standards Compliance (AGENTS.md)

• No secrets committed: .npmrc contains only comments and include=dev (the harness redaction markers on grep hits over this file are review-harness insertions over a config file path, not repository secrets).
• Lint/typecheck must pass and block CI: CI results for head show Lint, Typecheck, Tests, Coverage, Build, Docker Build (both variants), Database migrations/integration, smoke, and npm audit all successful.
• Documentation conventions: the comment follows the repo's established pattern of cross-referencing rationale ("//".audit in package.json, SECURITY-ACCEPTED-RISKS.md rows) rather than leaving tribal knowledge.

Tool Harness Findings

15 tool calls across 6 rounds: direct reads of package.json, SECURITY-ACCEPTED-RISKS.md, package-audit.test.ts, security-audit.yaml, the setup-node action, Dockerfile, and Dockerfile.test.ts all succeeded and are cited above. A repo-wide grep confirmed the consumer inventory. Attempts to read .npmrc directly and its git history were blocked as a sensitive file, so .npmrc content is taken from the PR diff itself (which shows the full file: three comment lines, the new CAUTION line, and include=dev). All external verification attempts (gh_api for the fix PRs and npm's config source, repo_contents) failed with "platform transport not configured" / preflight errors, so upstream npm documentation could not be fetched; the npm include/omit semantics are therefore grounded in the repo's own empirically-validated notes and the green CI gate rather than an upstream doc.

Unknowns or Needs Verification

• Optional-dependency scope under --include=prod: three repo artifacts (the "//".audit note, SECURITY-ACCEPTED-RISKS.md, and the package-audit.test.ts header comment) assert that production and optional dependencies remain audited with --omit=dev --include=prod, but the only cited empirical verification covers a production package, and no test asserts optional inclusion at runtime. Upstream npm docs could not be fetched (transport unavailable). This behavior landed in the already-merged fix PR, not in this diff, and the green head CI is consistent with it — but if anyone wants belt-and-braces certainty that sharp and the Next.js platform binaries stay inside the audit gate, a one-off npm ls sharp --omit=dev --include=prod (or a test asserting it) would close the gap.
• PR body staleness: the body's Fix/invariant text referencing --include=optional should ideally be corrected or annotated so the description matches the landed mechanism; this does not affect the diff's correctness.
• The PR thread comment suggesting this PR is an "empty delta" versus PR 1168 is untrusted discussion and also factually off for the current head: the diff versus base is the one-line .npmrc comment, which is real, reviewable content.

Sources

• Repository files read directly: package.json (scripts.audit line 12; "//".audit line 67), SECURITY-ACCEPTED-RISKS.md, package-audit.test.ts, .github/workflows/security-audit.yaml, .github/actions/setup-node/action.yml, Dockerfile, Dockerfile.test.ts
• PR diff for .npmrc (full post-image visible in the hunk)
• Repository history: f683575 (fix via PR 1166/1172), 7a6ad35 (superseded --include=optional attempt), d6d4fb5 (original .npmrc dev-deps config)
• CI status API results for head commit be550e1 (all checks success, including the blocking npm audit gate)
• Linked issue body (failing-run log establishing the dev-tree audit root cause)

Unaddressed required checks

The classifier marked these checks as required for this PR's risk profile, but the review does not resolve or disposition them:

• treat as high priority — verify correctness carefully

Requirement trace

1 of 3 requirement(s) not fully traced to enforcement and a test:

• req-1987056d15a3 — unverifiable (no valid enforcement location)

Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.

[itsmiso-ai]

Update on the cherry-pick note above: the underlying fix for #1162 landed on main via #1166 (merged in #1172) with --include=prod plus a guard test, so I merged origin/main into this branch and resolved the conflicts in favor of main. This PR's remaining delta is now only a CAUTION comment line in .npmrc documenting the include=dev precedence footgun, aligned with the landed --include=prod fix. No cherry-pick of 7a6ad35 into #1168 is needed anymore for CI (the audit gate on main is already green), and this PR keeps its own single-branch history (merge commit be550e1, no force-push). PR retitled/re-described accordingly; all checks green on be550e1.

[joryirving]

Re-reviewed current head be550e11. Approve.

The branch has converged to a one-line documentation-only delta in .npmrc, and the comment is accurate against the landed fix on main:

• scripts.audit is npm audit --omit=dev --include=prod ...;
• the package.json audit rationale documents the same precedence trap;
• SECURITY-ACCEPTED-RISKS.md records #1166 as the resolved mechanism and the regression guard;
• include=dev itself remains unchanged, preserving clean-checkout devDependency installs.

The earlier automated-review complaint about a stale --include=optional PR body no longer applies: the current PR description has been rewritten to describe the landed --include=prod implementation and this residual comment-only delta.

Current-head Security Audit, CI, image build, PR Smoke, and AI PR Review are all green. I found no remaining blocker or nit worth holding this for.