Skip to content

fix(ci): retry the npm audit, so an unreachable registry is not read as a finding - #827

Merged
wshallwshall merged 2 commits into
mainfrom
fix/npm-audit-retry
Sep 4, 2026
Merged

fix(ci): retry the npm audit, so an unreachable registry is not read as a finding#827
wshallwshall merged 2 commits into
mainfrom
fix/npm-audit-retry

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

Why

npm audit exits non-zero both when the advisory database reports a vulnerability and when it cannot be reached. The bare npm audit --package-lock-only this replaces made the two indistinguishable, so a red on the required npm-audit (ide dependency vulnerabilities) context said nothing about the dependencies. That is a compensating control resting on a false premise.

Measured 2026-09-04. Of 34 failures of that required context in one night, 32 were transport errors (503 Service Unavailable, or network timeout at https://registry.npmjs.org/-/npm/v1/security/advisories/bulk) and 2 were real findings on other jobs. Each false one blocked a merge or evicted a merge-queue entry; one pull request was evicted from the queue seven times on this alone, and three more were pushed to UNMERGEABLE by it while this was being written.

What changed

The step retries five times with backoff, and decides "verdict or transport failure" on the content of the audit output (.metadata.vulnerabilities) rather than on exit status, which cannot separate them.

This is a strengthening, not a relaxation

The gate stays fail-closed, and that is the property the test pins:

  • the only exit 0 sits inside the branch where npm audit itself succeeded;
  • exhausting the retries exits 1, with a message saying no verdict was obtained;
  • nothing added here can turn a real advisory green.

The one assumption I cannot verify without a networked runner is that npm audit --json emits .metadata.vulnerabilities on a verdict and omits it on a transport error. It is arranged so that being wrong about it is safe: if the discriminator never matches, a real finding is treated as "no verdict", the retries exhaust, and the job fails. The assumption can cost a false red; it cannot produce a false green.

Verification

tests/test_npm_audit_retry.py grades the fail-closed property, not a signature. Three arms, disjoint reds:

arm change result
A unmutated 5 pass
B fail-open exit 0 added after the retry loop 2 tests fail (the exit 0 count, and the tail check)
C exactly one exit 0, but guarded by if [ -f package-lock.json ] instead only the enclosing-block check fails

Arm C matters: it keeps the count unchanged, so it confirms the enclosing-if walk works independently rather than riding on the count assertion.

An earlier version of that test searched the whole preface for if npm audit, which every later line trivially satisfies. It passed arm B and so proved nothing. It was rewritten to walk backwards to the genuinely enclosing block, tracking fi.

Also adds the tests/tooling_manifest.txt line the partition test requires.

Checks run locally

ruff format --check, ruff check, tests/test_npm_audit_retry.py, tests/test_tooling_partition.py — all green. The workflow's own effect is only observable on a hosted runner, so the Security leg here should be read before merging.

…as a finding

`npm audit` exits non-zero BOTH when the advisory database reports a vulnerability
AND when it cannot be reached, so the bare `npm audit --package-lock-only` this
replaces made the two indistinguishable. A red on the required
`npm-audit (ide dependency vulnerabilities)` context therefore said nothing about the
dependencies -- a compensating control resting on a false premise.

Measured 2026-09-04: of 34 failures of that context in one night, 32 were
`503 Service Unavailable` or `network timeout at .../advisories/bulk`, and 2 were real
findings (pip-audit, gitleaks). Each false one blocked a merge or evicted a merge-queue
entry; one pull request was evicted seven times on this alone.

The step now retries five times with backoff and decides "verdict or transport failure"
on the CONTENT of the output (`.metadata.vulnerabilities`), which exit status cannot
separate.

STILL FAIL-CLOSED, which is the point. The only `exit 0` sits inside the branch where
`npm audit` itself succeeded, and exhausting the retries exits 1. Nothing here can turn
a real advisory green. If the discriminator assumption about npm's JSON is wrong, the
cost is a false RED, never a false green.

tests/test_npm_audit_retry.py pins the fail-closed property rather than a signature.
Verified with three arms and disjoint reds: unmutated passes; a fail-open added after
the loop trips two tests; and a variant keeping exactly one `exit 0` but guarding it
with a different condition trips only the enclosing-block check, so that logic is
confirmed independently of the count.
@wshallwshall wshallwshall added reviewed A reviewer has read this. Removed automatically when new commits arrive. and removed reviewed A reviewer has read this. Removed automatically when new commits arrive. labels Sep 4, 2026
@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 4, 2026
tests/test_security_posture.py::test_required_jobs_have_no_neutered_steps failed on
both harness legs: my diagnostic `head -c 400 audit.err || true` sits inside a REQUIRED
job, and that guard forbids discarding an exit code anywhere in one.

The guard is right and is not being worked around. Its own remediation -- confine a
failure-tolerant command to its own step -- is unavailable here because the line is
inside the retry loop, so the `|| true` is simply removed instead. It was never needed:
`2> audit.err` on the line above creates the file unconditionally, so `head` cannot fail.

No behaviour change. The step still retries, still decides on `.metadata.vulnerabilities`
rather than exit status, and its only `exit 0` still sits inside the branch where
`npm audit` itself succeeded.

Verified: the guard that caught this now passes, tests/test_npm_audit_retry.py still
passes all five, and the whole of tests/test_security_posture.py is green (26 tests).
@github-actions github-actions Bot removed the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall wshallwshall added the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall
wshallwshall added this pull request to the merge queue Sep 4, 2026
Merged via the queue into main with commit c950346 Sep 4, 2026
44 of 45 checks passed
@wshallwshall
wshallwshall deleted the fix/npm-audit-retry branch September 4, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying. reviewed A reviewer has read this. Removed automatically when new commits arrive.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant