Repository navigation
fix: retry and bound the npm audit call behind check:audit - #1763
Conversation
scripts/audit-gate.mjs made exactly one npm audit call and failed the gate when that call came back without a report, with no retry and no bound on how long the call could take. One bad answer from the registry therefore reddened the required Validate repository check on every open pull request at once, with nothing in any of those diffs to fix. The npm bundled with Node 22 posts to the bulk advisory endpoint first and, on any failure, falls back to the retired quick-audit endpoint, which now answers 400. The 400 is what reaches the log while the bulk failure that caused the fallback stays hidden, so a transient fault reads as a lockfile problem. npm defaults fetch-timeout to 300000, and one hung bulk request really did spend all of it. The job running this gate is capped at fifteen minutes, so three attempts at that default would have replaced a bad diagnostic with no diagnostic at all; each attempt is now capped at sixty seconds, which keeps the whole loop cheaper than one attempt is today. Only the answers carrying no report are retried. A parsed report stays final whatever it says, so a genuinely new advisory is never retried away, and every exhausted path still exits 1 — the gate fails closed exactly as before. Closes #1762 Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
Important Held for human review by the hive's ACMM level gate. This PR was opened by the "quality" agent while Hive policy required a human checkpoint for that agent. Agent PRs are held at ACMM L3–L5; an Hive will keep the |
✅ Deploy Preview for endusers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
011a0b7
ℹ️ Sentinel notice — informational; author is trusted (hive agent), merge is not blockedHive flagged this PR (author @hivecommons-hive[bot], head
Hive did not add the |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Read the diff against the tree at the reviewed head (011a0b7), ran node --test tests/audit-gate.test.mjs (47 tests, 0 fail), and verified the surrounding claims. This looks correct to me.
What I checked:
- Fail-closed invariant holds.
runAudit()(scripts/audit-gate.mjs) returnsfailure: nullonly whenreport.vulnerabilitiesis a non-null object;main()retries only whilelast.failure !== null, so a parsed report — including one naming a new advisory — is final on the first attempt. The added test "a report naming a new advisory is never retried away" assertsattempts === 1with exit 1, which pins this. - Every exhausted path still exits 1. The three failure arms (
spawn,unparseable,no-report) each reachprocess.exit(1)inmain(), and the three diagnostics stay distinguishable, matching the pre-existing test assertions. - Retry bound is real, not just claimed. The loop runs at most
AUDIT_ATTEMPTS = 3; the "retrying stops at the attempt limit" test asserts the stub was invoked exactly 3 times, so an unbounded loop can't regress silently. --fetch-timeout=60000is on every attempt (runAudit's argv), asserted per-invocation via the stub's argv log — the flag can't be dropped without a test failure.- Scope claims check out:
check:auditisnode scripts/audit-gate.mjs(package.json:26);stryker.config.mjs:62mutatessrc/**andscripts/lib/**only, so the mutation lane is indeed untouched; the diff touches only the two files claimed, disjoint from #1761'stests/validate-utils.test.mjs.
One nit, not a defect: the body's "Only answers carrying no report are retried" is slightly broader in the code — spawn and parse failures are retried too (all three arms return report: null). The last added test states this explicitly ("a missing npm is retried too"), so the behavior is intentional and tested; just noting the prose is looser than the loop.
PR is hold-gated; a maintainer still needs to lift the hold. The change is low-risk (CI-only script, no workflow files touched) and all checks are green on this head.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Bound each audit process independently of npm request retries, preserve fail-closed handling, and cover deadline recovery and exhaustion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com>
|
Maintainer-delegated sole-maintainer disposition: the maintainer explicitly approved this existing-PR repair and delegated hold release and normal protected queue submission after the repaired head passed checks. Reviewed exact head 51da80e: the process deadline, retry/finality behavior and timeout diagnostics match the approved scope; no allowlist or protection changes. Required Validate repository and Lint repository, CodeQL, DCO, browser tests and browser coverage are green on this head. Agent inspection is not independent human approval. Releasing only this PR hold and submitting this exact head to the protected queue; no bypass. |
1 participant
Summary
Retry npm audit failures that produce no usable report, while enforcing an
actual subprocess deadline rather than treating a per-request timeout as a
whole-process guarantee.
--fetch-timeout=60000and additionally enforce a 60,000-msspawnSynctimeout withSIGKILL, so npm's retries, fallback requests orsignal handling cannot indefinitely extend the audit subprocess.
two delays, plus process launch and reporting overhead. This is not a
guarantee for the duration of the entire CI job.
partial JSON is not accepted as a completed audit.
diagnostics, advisory classification and the documented allowlist.
advisory; retries cannot discard an actual finding.
Regression evidence
The existing CLI sandbox now records the actual subprocess timeout and kill
signal and can synthesize deadline errors without adding minute-long waits
to the suite. Focused cases cover the exact 60,000-ms deadline on every
attempt, recovery after a timed-out partial report, exhausted deadline
failures and the existing one-attempt finality for real advisory reports.
Local validation: 49 focused cases; the full required unit coverage gate
passes with 3,034 tests, source 100% lines/regions and harness 100% lines /
99.77% regions. All six lint gates, the live dependency audit and the
production build pass. The live audit's existing allowlisted findings are
unchanged; no dependency or allowlist update is included.
Maintainer authorization
The maintainer explicitly approved this bounded repair to the existing PR
and delegated hold release and exact-head protected queue submission after
required checks pass under the documented sole-maintainer path. An agent
review is not independent human approval; no protection is bypassed.
Closes #1762.