Repository navigation
fix(ci): publish base images after root dependency changes - #11273
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe base-image workflow now triggers for root ChangesBase image publication
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Root package manifest changes now trigger base-image publication, with coverage for both package.json and package-lock.json. The change is ready to merge with no identified current-head risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit efd8d76 in the TypeScript / code-coverage/cliThe overall line coverage in commit efd8d76 in the Show a line coverage summary of the most impacted files.
Updated |
sandl99
left a comment
There was a problem hiding this comment.
Independent review of efd8d768676feeac545b6ab4140590e16cfacf6e against de7f565dd062b6f5affe12218ad825c97efee042: PASS. No blocking or advisory code findings.
The two added root package paths repair the missing publication trigger. The E2E selector reads the same workflow path list, so future root dependency changes also advance its required publication revision. This preserves the existing push-only evidence boundary. The main advance inspected during this review to 87d6dfae852ede67a5acb63e2beae17020a754b6 does not change either reviewed file or the selector.
Validation:
- Ran the focused
base-image-publication.test.tssuite in an isolated worktree with Vitest 4.1.9: all 62 tests passed. - Ran the added regression with the original base workflow substituted: it failed because the root package paths were absent. Restored the candidate file byte-for-byte afterward.
- Reviewed the complete two-file diff, the publication selector, the source-graph audit consumer, and existing review feedback. GitHub marks the candidate commit Verified.
- CodeRabbit's docstring coverage warning concerns a test callback and does not identify a behavior or security defect.
Security categories:
| Category | Verdict | Evidence |
|---|---|---|
| Secrets and credentials | PASS | No new credentials, secret expressions, or credential recipients. |
| Input validation and sanitization | PASS | Adds two fixed paths accepted by the existing parser and matcher. |
| Authentication and authorization | PASS | Repository, branch, event, and permission restrictions remain intact. |
| Dependencies and third-party libraries | PASS | Adds no dependency; publication now follows changes to its audited root graph. |
| Error handling and logging | PASS | Failed audits and failed publishers continue to block consumers. |
| Cryptography and data protection | PASS | SHA, run-attempt, artifact, and digest validation remain intact. |
| Configuration and security headers | PASS | Only the two push paths change; job configuration and permissions are unchanged. |
| Security testing | PASS | 62 tests passed, including provenance rejection; the new regression distinguishes the base. |
| System security | PASS | Audit-before-publish ordering, immutable cohort validation, and first-parent selection remain intact. |
Files reviewed: .github/workflows/base-image.yaml and test/e2e/support/base-image-publication.test.ts; adjacent consumers: tools/e2e/base-image-publication.mts and scripts/audit-reviewed-npm-graph.mts.
At review time, CI and automated evaluation were still completing. This approval records the code review and does not waive the remaining merge requirements.
Outcome
Publish base and managed images after the root package manifest or lockfile changes. Downstream PR gates can then use publication evidence built from the dependency graph on
main.Reason
PR #11264 updated the root dependency graph, but the base-image workflow did not run because its
push.pathslist omitted the root package files. As a result, PR #11098 still sees the earlier failed publication instead of a new audited publication frommain.Related issues
Unblocks #11098.
Relates to #11264.
Changes
package.jsonandpackage-lock.jsonto the base-image workflow's exact push paths.pushruns frommain.Verification
npx vitest run --project e2e-support test/e2e/support/base-image-publication.test.ts --reporter=dot— passed, 62 tests.npm run format:check— passed.npm run test:titles:check— passed.npm run build:cli— passed.npm --prefix nemoclaw run build— passed.npm run validate:pr— passed.git diff --checkand manual diff review — passed; the diff contains no secrets, API keys, or credentials.Review notes
This changes the sensitive workflow path
.github/workflows/base-image.yamlinNVIDIA/NemoClawat commitefd8d768676feeac545b6ab4140590e16cfacf6e. Author self-review and the focused workflow contract test found no change to permissions, credentials, job code, or publication trust boundaries. No independent pre-publication review is verified; this draft awaits independent review.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests