ci(security): retry incomplete npm audits

[apurvvkumaria]

Summary

The reviewed npm audit gate currently fails immediately when the registry returns an incomplete JSON response. This change retries only incomplete or timed-out scans within a fixed 138-second budget, continues to evaluate complete vulnerability reports immediately, and still fails closed when the retry budget is exhausted.

Changes

• Bound each npm audit --omit=dev --json attempt to 45 seconds and retry incomplete scans after fixed 1-second and 2-second delays.
• Preserve immediate policy evaluation for every structurally complete audit report, including reports containing blocking vulnerabilities.
• Fail closed after three incomplete attempts and retain the final report and failure provenance.
• Sanitize transient retry warnings to stable reason codes so registry diagnostics, credentials, and control text cannot enter CI logs.
• Add focused regressions for incomplete responses, scanner timeouts, retry exhaustion, blocking vulnerability reports, non-timeout process errors, and diagnostic redaction.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: This changes an internal trusted CI audit retry boundary without changing supported commands, configuration, APIs, policy, or user workflows.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: A nine-category security review passed exact current-main head 293ed408a. The final refresh adds only byte-identical upstream changes from merged PRs fix(security): update sandbox dependency fixes #7563 and fix(ci): build conflict trees from main #7578; the three PR-authored audit files are unchanged. Complete vulnerability reports still evaluate immediately, retry exhaustion remains fail-closed, and both transient and terminal diagnostics are redacted with negative regression coverage.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: Exact head 293ed408a changes only the internal reviewed-npm-audit helper and its tests relative to current main. The merge-only changes from fix(security): update sandbox dependency fixes #7563 and fix(ci): build conflict trees from main #7578 match main byte-for-byte. No user-facing CLI, configuration, API, or supported workflow contract changes. No comments were added; the new test titles precisely describe retry/backoff, timeout, non-timeout error, complete-report, and exhausted-budget behavior. No documentation or Fern build input changed.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit: Not applicable
• Station profile/scenario: Not applicable
• Result: Not applicable
• Supporting evidence: Not applicable

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — exact refreshed head: npx vitest run --project integration test/reviewed-npm-audit.test.ts test/reviewed-npm-audit-workflow.test.ts: 39 passed; the normal pre-push CLI TypeScript and package/tag version checks passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

[coderabbitai]

📝 Walkthrough

Walkthrough

The npm audit helper now retries incomplete or timed-out executions, classifies failures, sanitizes retry warnings, and returns parsed reports or terminal errors. Reviewed audit execution uses the final result, while unit and workflow tests cover the new behavior.

Changes

NPM audit retry handling

Layer / File(s)	Summary
Retry runner and audit result contract scripts/lib/reviewed-npm-audit.mts	Adds attempt timeouts, retry classification, bounded delays, warning sanitization, report parsing, and terminal failure results through runNpmAuditWithRetry.
Reviewed audit integration scripts/lib/reviewed-npm-audit.mts	Routes reviewed audits through the retry helper, persists final stdout, and uses the resulting report or failure in downstream processing.
Retry behavior validation test/reviewed-npm-audit.test.ts, test/reviewed-npm-audit-workflow.test.ts	Tests incomplete reports, timeouts, blocking reports, spawn errors, exhausted retries, warning redaction, provenance output, and configured timeout usage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: bug-fix

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding retries for incomplete npm audit scans.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/reviewed-npm-audit-bounded-retry

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 293ed40 in the codex/reviewed-npm-a... branch remains at 96%, unchanged from commit 1c86061 in the main branch.

---

_{Updated July 26, 2026 08:47 UTC}

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized E2E selections match; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: None

1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Document the source and removal condition for audit retries

• Location: scripts/lib/reviewed-npm-audit.mts:319
• Category: correctness
• Problem: The new retry path treats every incomplete npm audit result as transient, but it does not identify the originating npm condition, explain why it cannot be fixed at that source, or define when this workaround can be removed.
• Impact: The retry policy can remain after its source condition changes and can mask a persistent audit-service failure for the additional retry interval.
• Recommendation: Add a concise code comment or linked tracked record that names the observed source condition, why this wrapper must handle it, and the condition for removing the retry path.
• Verification: Read the retry branch and its linked source record. Confirm it identifies the incomplete-report condition and a removal condition.
• Test coverage: Existing retry tests cover wrapper behavior. Add or retain a regression fixture that represents the named originating incomplete-report condition and verifies bounded fail-closed handling.
• Evidence: `runNpmAuditWithRetry` catches all report-validation errors and retries before returning a generic terminal failure. The supplied correctness context has no linked issue or maintainer decision that documents the originating condition or removal boundary. The changed tests demonstrate empty, incomplete, invalid, and timeout result handling but do not identify a source condition.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

test/reviewed-npm-audit-workflow.test.ts (1)

132-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Source-text assertions on the helper file, not observable behavior.

This reads reviewed-npm-audit.mts as text and asserts on literal substrings ("const NPM_AUDIT_ATTEMPT_TIMEOUT_MS = 45_000", "timeout: NPM_AUDIT_ATTEMPT_TIMEOUT_MS"). This locks the test to source formatting/wording rather than proving the configured timeout is actually wired into the spawnSync call — a harmless rename, reformat, or refactor (e.g. extracting the constant into a shared config module) would fail this test without any behavior change, while a genuine bug (e.g. passing a different constant to timeout) that happens to preserve these substrings would pass it.

Consider asserting behavior instead, e.g. mocking spawnSync/child_process and asserting the timeout option value actually passed by runReviewedNpmAudit, or invoking runNpmAuditWithRetry with a fake run and confirming the configured constant governs retry timing.

Based on path instructions for **/*.test.{ts,js,mts,mjs,cts,cjs}: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/reviewed-npm-audit-workflow.test.ts` around lines 132 - 147, Replace the
source-text assertions for NPM audit timeout wiring in the reviewed-npm-audit
test with an observable behavior test. Exercise runReviewedNpmAudit or
runNpmAuditWithRetry using a controlled fake spawnSync/run implementation, then
verify the timeout option passed to the audit attempt is 45_000 and retry
behavior remains correct. Remove the helper file substring checks while
preserving the existing action and configuration assertions.

Source: Path instructions

test/reviewed-npm-audit.test.ts (1)

315-338: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

This test's fixture never exercises the redaction gap in the terminal failure path.

Using empty summary/detail here means the assertion on audit.failure?.message never proves that sensitive stderr/report.error content is redacted once the retry budget is exhausted — unlike the "bounded backoff" test, which does inject secrets but only into retry warnings (a different, already-sanitized path). See the related comment on scripts/lib/reviewed-npm-audit.mts (Lines 337-344) for the underlying gap this test doesn't catch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/reviewed-npm-audit.test.ts` around lines 315 - 338, Update the “fails
closed after the scan-incomplete retry budget is exhausted” test fixture to
include representative sensitive values in stderr and the report error
summary/detail, then assert that audit.failure?.message contains the structural
failure context while excluding every injected secret. Preserve the existing
three-attempt and backoff assertions, and target the terminal failure message
produced by runNpmAuditWithRetry rather than retry warnings.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/lib/reviewed-npm-audit.mts`:
- Around line 337-344: Update the terminal failure construction in
runNpmAuditWithRetry so it uses the same sanitized stable reason code as the
per-attempt warn() calls instead of embedding lastFailure?.message. Preserve the
retry count and fail-closed behavior, and ensure the resulting failure message
used for both throwing and provenance cannot expose raw stderr or report error
contents.

---

Nitpick comments:
In `@test/reviewed-npm-audit-workflow.test.ts`:
- Around line 132-147: Replace the source-text assertions for NPM audit timeout
wiring in the reviewed-npm-audit test with an observable behavior test. Exercise
runReviewedNpmAudit or runNpmAuditWithRetry using a controlled fake
spawnSync/run implementation, then verify the timeout option passed to the audit
attempt is 45_000 and retry behavior remains correct. Remove the helper file
substring checks while preserving the existing action and configuration
assertions.

In `@test/reviewed-npm-audit.test.ts`:
- Around line 315-338: Update the “fails closed after the scan-incomplete retry
budget is exhausted” test fixture to include representative sensitive values in
stderr and the report error summary/detail, then assert that
audit.failure?.message contains the structural failure context while excluding
every injected secret. Preserve the existing three-attempt and backoff
assertions, and target the terminal failure message produced by
runNpmAuditWithRetry rather than retry warnings.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b2789591-f867-4870-a2a9-ee3ec52674cc

📥 Commits

Reviewing files that changed from the base of the PR and between d525e15 and 154fe5f.

📒 Files selected for processing (3)

• scripts/lib/reviewed-npm-audit.mts
• test/reviewed-npm-audit-workflow.test.ts
• test/reviewed-npm-audit.test.ts

[cv]

The new head fixes the test-body growth guard, and the exact focused suite passes 39/39. One security blocker remains before this trusted CI change can be approved:

runNpmAuditWithRetry constructs the terminal failure with lastFailure?.message. That message can contain raw final-attempt stderr or report.error text (including authenticated registry URLs, newlines, and ANSI/control sequences), and is then emitted by the CLI and stored in provenance. The per-attempt warnings are sanitized, but retry exhaustion reopens the same diagnostic leak. Build the terminal failure from a stable sanitized reason code (plus attempt count), never raw scanner text, and add a final-attempt fixture with injected credentials/control text that proves both the returned failure/provenance-facing message and console path exclude them.

The timeout source-shape assertion and the advisor note about documenting the upstream/removal boundary are nonblocking maintainability follow-ups. After the security fix, refresh the exact-head sensitive/docs receipts and rerun normal gates. The current reviewed-npm-audit failure is still the old trusted base helper receiving an incomplete registry response; wait for registry recovery rather than waiving that required gate.

[apurvvkumaria]

Addressed the requested terminal redaction in signed commit feb812c77. Retry exhaustion now reports only the attempt count and stable sanitized reason code; it no longer embeds raw scanner stderr or report.error. Regression coverage injects secrets into stderr and report summary/detail, verifies the returned failure excludes them, and verifies the provenance/throw path excludes ECONNREFUSED and registry detail. Focused audit tests pass 39/39, formatting and hooks pass, the docs receipt is refreshed to the exact head, and all three commits are GitHub Verified. Fresh exact-head CI and automated review are running.