Skip to content

fix(ci): skip implicit Portable npm audit - #11068

Merged
cv merged 3 commits into
mainfrom
codex/npm-audit-dedup
Sep 6, 2026
Merged

fix(ci): skip implicit Portable npm audit#11068
cv merged 3 commits into
mainfrom
codex/npm-audit-dedup

Conversation

@cjagwani

@cjagwani cjagwani commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Outcome

Automated Portable rootless E2E runs no longer send an implicit npm advisory request while installing root dependencies. Manual workflow dispatches retain an explicit install-time audit; reviewed npm-audit gates and the accepted best-effort behavior for concurrent exact-input cache misses are unchanged.

Reason

Portable run 33879304029 showed both the rootless job and portable-launch job implicitly auditing 506 packages from the same checkout lock. Neither install-time result enforces NemoClaw policy or emits reviewed evidence. Main CI for the same revision separately evaluated the candidate through the reviewed production-graph audit.

These requests do not have identical audit inputs. Issue #11028 accepts concurrent exact-input cache misses and rejects global serialization solely to guarantee one request. This change preserves that tradeoff and removes a different observed source of registry work.

Related issues

Refs #11028

Changes

  • Route the rootless dependency install by event: pull requests and main pushes use --no-audit --no-fund, while workflow_dispatch uses explicit --audit --no-fund.
  • Extend the existing Portable workflow contract to protect both install commands, their mutually exclusive conditions, the exact trigger set, and main-only push scope.
  • Register that security contract in the source-shape test budget.

The shared pinned prepare-E2E action remains unchanged because changing it would affect many consumers and require a separate provenance repin.

Verification

  • npm run validate:pr — passed pre-commit, commit-message, and pre-push checks. Pinned hook source trees and release artifacts were verified locally after direct GitHub Git transport returned HTTP 503.
  • npx vitest run --project e2e-support test/e2e/support/portable-profile-rootless-runtime-workflow.test.ts — 4 tests passed.
  • npm run source-shape:check — passed.
  • npm run checks:repository — passed.
  • npm run test:projects:check — passed; 2,588 files mapped across 7 projects.
  • npm run test:changed — passed; the catalogue compiled and 45 growth tests passed. The focused test separately verified the changed workflow contract.
  • git diff --check — passed.
  • Secret review — the diff contains no secrets, API keys, or credentials.

Review notes

Issue #11028 contains the accepted scope and trust decision. This workflow change does not alter cache-writer trust, freshness checks, policy evaluation, fail-closed behavior, graph receipts, Docker audit fallback, or signature verification. Manual install-time audit remains best-effort visibility rather than a fail-closed policy decision; the reviewed production-graph audit remains authoritative for pull requests and main.

Concurrent cache misses remain best-effort by the accepted decision. The first Advisor cycle identified the manual-dispatch gap; the repair preserves its audit and passed three independent pre-publication reviews with no remaining findings.


Signed-off-by: cjagwani cjagwani@nvidia.com

Summary by CodeRabbit

  • Chores

    • Updated automated dependency installation behavior based on how the workflow is triggered.
    • Manual workflow runs now include dependency auditing, while routine pull request and push runs use a streamlined installation process.
  • Tests

    • Expanded workflow validation to cover both automated and manually triggered runs.
    • Added checks confirming that auditing occurs before catalogue compilation during manual runs.
    • Added coverage to ensure each trigger follows its intended installation and auditing behavior.

@copy-pr-bot

copy-pr-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 33aea310-d606-4dd3-9978-c5dde443af27

📥 Commits

Reviewing files that changed from the base of the PR and between ae0d528 and b4e5691.

📒 Files selected for processing (1)
  • ci/source-shape-test-budget.json

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The workflow selects npm audit behavior by trigger. Automated runs disable auditing, while manual runs enable it. Contract tests validate both paths and their ordering.

Changes

Dependency audit routing

Layer / File(s) Summary
Workflow trigger routing
.github/workflows/portable-profile-e2e.yaml
Push and pull request runs use non-auditing installation. Manual dispatch runs use auditing.
Workflow contract validation
test/e2e/support/portable-profile-rootless-runtime-workflow.test.ts, ci/source-shape-test-budget.json
Tests validate both triggers, installation paths, and audited-step ordering. The source-shape budget includes the new security exception.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b4e56

Automated Portable runs now skip npm audit while manual dispatches retain audit visibility, with contract coverage for both paths. No current merge-blocking risk remains.

Suggested reviewers: ericksoa, cv, apurvvkumaria

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary CI change: skipping implicit Portable npm audits for automated runs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/npm-audit-dedup

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

@github-code-quality

github-code-quality Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit b4e5691 in the codex/npm-audit-dedu... branch remains at 96%, unchanged from commit 0e5f186 in the main branch.


Updated September 06, 2026 16:14 UTC

@cjagwani

cjagwani commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Candidate evidence disposition (ae0d528)

The second automated-review cycle and gate-true CI have completed for the unchanged candidate.

Advisor findings

All nine specialist jobs succeeded and all nine reports were collected. Architecture, Customer value, Delivery, and Migration raised one shared objection: the old install-time audit covered development dependencies, so they treated its removal as the loss of an enforcing full-lock audit.

That premise does not match the observed behavior:

  • The pre-change rootless job audited 506 packages, reported nine vulnerabilities (including six high), and continued through compilation and the live E2E. The install-time report did not reject the graph.
  • A successful manual-dispatch job likewise reported the same nine vulnerabilities and completed successfully.
  • Merged PR #11035 established the same suppression pattern for full-development installs while retaining the reviewed production-graph audit as the policy authority.
  • The first Advisor cycle specifically requested retaining audit visibility for workflow_dispatch; ae0d528 does exactly that. Requiring a new reviewed full-development graph, per-job evidence handoff, or all-trigger reviewed-audit integration would add a new security mechanism outside this narrow deduplication slice.

Disposition: the shared objection is false-premised and would expand scope, so the candidate remains unchanged. The Reduction specialist's suggestion to combine the two complementary static steps is stylistic; keeping the commands explicit makes trigger behavior directly reviewable and testable.

CI disposition

Gate-true CI passed the reviewed audit, static checks, compilation, installer integration, build/typecheck, plugin tests, all 12 CLI shards, the CLI aggregate, and final checks. Security and code-quality scanning also passed.

The candidate rootless job proves the changed install ran as npm ci --ignore-scripts --no-audit --no-fund and emitted no root-install audit summary. It later failed in phase 7 when the Hermes dashboard did not become ready. That signature exactly matches the recorded base job and the first candidate job: image staging passed, the 60-attempt dashboard wait expired, and cleanup passed.

Disposition: the rootless failure is inherited and outside this diff. The checked-in retry policy does not authorize rerunning unchanged work, so no rerun or waiver is claimed. A maintainer can decide its merge-time disposition if supplemental CI failures require an exception.

No candidate-owned CI or automated-review finding remains. Substantive CodeRabbit review is the remaining automated evaluation and will be triggered by moving the PR out of draft.

@cjagwani
cjagwani marked this pull request as ready for review September 4, 2026 15:52
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions bug-fix PR fixes a bug or regression labels Sep 4, 2026

@rsliter rsliter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at ae0d5281adc51043e1200819249fdc062f402f09.

The accepted #11028 scope permits removing redundant advisory requests while the reviewed production-graph audit remains the enforcing gate. The prior install-time audit was best-effort visibility: base evidence shows it reported vulnerabilities and continued, so suppressing it on automated Portable runs does not remove a fail-closed control. Manual dispatch retains that visibility. The trigger split and ordering are covered by the workflow contract test.

Required checks pass. The supplemental rootless job failed at the unchanged Hermes dashboard readiness phase after the image build, matching the base run; cleanup passed. I re-evaluated the PR Review Advisor objections and CodeRabbit feedback. They do not identify a PR-attributable blocker.

@cv
cv enabled auto-merge (squash) September 6, 2026 16:14
@cv
cv merged commit 30271df into main Sep 6, 2026
74 of 75 checks passed
@cv
cv deleted the codex/npm-audit-dedup branch September 6, 2026 16:16
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit b4e5691. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

All previous runs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants