fix(images): lock OpenClaw production graph#6830
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds lockfile-based OpenClaw installation verification, schema 2 reviewed npm audit configuration, trusted PR audit execution, expanded base-image triggers, runtime graph staging, provenance schema 3, and regression tests for integrity, workflow, and symlink protections. ChangesReviewed npm graph verification
OpenClaw image and CI integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PRWorkflow
participant TrustedAuditAction
participant AuditScript
participant NpmRegistry
participant AuditArtifact
PRWorkflow->>TrustedAuditAction: select trusted action from base SHA
TrustedAuditAction->>AuditScript: pass target-root and report-dir
AuditScript->>NpmRegistry: verify reviewed lock metadata
AuditScript->>AuditScript: npm ci and verify installed lock
AuditScript->>AuditArtifact: write JSON audit reports
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
f2e670f to
bd2035e
Compare
|
Addressed the exact-head guardrail and trusted-audit warning in
Validation: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/pr.yaml (1)
311-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne-time bootstrap hardcodes a specific PR number permanently into the shared workflow.
The bootstrap checkout/audit steps are gated on
github.event.pull_request.number == 6830and a specific fork name. Since PR numbers are never reused, this is safe from replay, but once PR#6830merges these branches become permanent dead conditionals inpr.yamlthat will never evaluatetrueagain. Consider tracking removal in a follow-up issue/PR once the trusted base has the schema-2 audit, so this doesn't linger indefinitely.Also applies to: 341-346
🤖 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 @.github/workflows/pr.yaml around lines 311 - 326, The one-time bootstrap conditions in the workflow permanently retain dead PR-specific branches after PR `#6830` merges. Track removal of the bootstrap checkout and corresponding audit steps, identified by “Checkout pinned bootstrap reviewed npm audit” and the related trusted-reviewed-npm-audit step, in a follow-up issue or PR once the trusted base supports schema-2 audit.test/openclaw-integrity-pin-suite.ts (1)
168-170: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a fail-closed test for the
--verify-installed-lockcheckpoint.The
node()mock at Line 288 always returns success for any call containing--verify-installed-lock, with no configurable failure path. The suite addedfailOpenClawNpmCiandfailOpenClawPostinstall(Lines 168-170, 1420-1465) to prove the pipeline fails closed at those checkpoints, but the adjacent — and security-relevant —--verify-installed-lockstep (documented inagents/openclaw/dependency-review.mdas the check that "reject[s] symlinked package roots or manifests") has no analogous negative test. As written, nothing in this suite would catch a regression that made a real installed-lock-verification failure non-blocking.♻️ Suggested addition mirroring the existing pattern
tamperOpenClawLock?: boolean; failOpenClawNpmCi?: boolean; failOpenClawPostinstall?: boolean; + failOpenClawVerifyInstalledLock?: boolean; } = {},tamperOpenClawLock = false, failOpenClawNpmCi = false, failOpenClawPostinstall = false, + failOpenClawVerifyInstalledLock = false, } = options;`fail_openclaw_npm_ci=${failOpenClawNpmCi ? "1" : "0"}`, `fail_openclaw_postinstall=${failOpenClawPostinstall ? "1" : "0"}`, + `fail_openclaw_verify_installed_lock=${failOpenClawVerifyInstalledLock ? "1" : "0"}`, "node() {", - ' if printf "%s\\n" "$*" | grep -q -- "--verify-installed-lock"; then printf "node %s\\n" "$*" >> "$call_log"; return 0; fi', + ' if printf "%s\\n" "$*" | grep -q -- "--verify-installed-lock"; then printf "node %s\\n" "$*" >> "$call_log"; [ "$fail_openclaw_verify_installed_lock" = "0" ] || return 44; return 0; fi',Then add a test paired with the existing "leaves no runtime exposure" cases, asserting
postinstall-bundled-plugins.mjsand symlink/provenance creation never happen when this flag is set.Also applies to: 192-194, 285-289, 1420-1465
🤖 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/openclaw-integrity-pin-suite.ts` around lines 168 - 170, Add a configurable failure path for the --verify-installed-lock branch in the node() mock, following the existing failOpenClawNpmCi and failOpenClawPostinstall options. Add a fail-closed test alongside the existing “leaves no runtime exposure” cases that enables this failure, asserts the pipeline rejects, and verifies postinstall-bundled-plugins.mjs plus symlink/provenance creation are never invoked.
🤖 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/audit-reviewed-npm-graph.mts`:
- Around line 34-49: The path validation around resolveTargetPath and
CONFIG_PATH is only lexical and must reject symlink traversal. Canonicalize
REPO_ROOT and validate each relevant target’s existing path components before
accessing the reviewed-npm configuration, locked graph, or report-directory
cleanup, rejecting direct and ancestor symlinks that resolve outside the
checkout. Add regression coverage for both direct and ancestor symlink cases.
In `@scripts/lib/reviewed-npm-archive.mts`:
- Around line 344-353: Update the root dependency validation around
rootDependencies to also inspect root.optionalDependencies and reject any root
optional dependency entries, ensuring the reviewed lock contains only the
requested package closure under npm ci --omit=dev. Preserve the existing
requirement that root.dependencies contains exactly the requested package and
version.
In `@test/reviewed-npm-audit-workflow.test.ts`:
- Around line 29-31: Add an explicit cleanup follow-up note near BOOTSTRAP_IF
documenting that the PR-6830 fork-specific bootstrap condition must be removed
after the PR merges. Preserve the existing bootstrap condition and BOOTSTRAP_SHA
values.
---
Nitpick comments:
In @.github/workflows/pr.yaml:
- Around line 311-326: The one-time bootstrap conditions in the workflow
permanently retain dead PR-specific branches after PR `#6830` merges. Track
removal of the bootstrap checkout and corresponding audit steps, identified by
“Checkout pinned bootstrap reviewed npm audit” and the related
trusted-reviewed-npm-audit step, in a follow-up issue or PR once the trusted
base supports schema-2 audit.
In `@test/openclaw-integrity-pin-suite.ts`:
- Around line 168-170: Add a configurable failure path for the
--verify-installed-lock branch in the node() mock, following the existing
failOpenClawNpmCi and failOpenClawPostinstall options. Add a fail-closed test
alongside the existing “leaves no runtime exposure” cases that enables this
failure, asserts the pipeline rejects, and verifies
postinstall-bundled-plugins.mjs plus symlink/provenance creation are never
invoked.
🪄 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: cc1f2202-ce38-48bd-aee7-a6111d9b63c0
⛔ Files ignored due to path filters (1)
agents/openclaw/openclaw-runtime/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/actions/ci-reviewed-npm-audit/action.yaml.github/actions/resolve-sandbox-base-image/action.yaml.github/workflows/base-image.yaml.github/workflows/main.yaml.github/workflows/pr.yamlDockerfileDockerfile.baseagents/openclaw/dependency-review.mdagents/openclaw/openclaw-runtime/package.jsonci/reviewed-npm-audit.jsonci/source-shape-test-budget.jsondocs/security/openclaw-2026.6.10-dependency-review.mdscripts/audit-reviewed-npm-graph.mtsscripts/lib/reviewed-npm-archive.mtssrc/lib/sandbox/build-context.tstest/openclaw-dependency-review.test.tstest/openclaw-integrity-pin-suite.tstest/openclaw-locked-install.test.tstest/reviewed-npm-archive.test.tstest/reviewed-npm-audit-workflow.test.tstest/sandbox-build-context.test.ts
bd2035e to
be7b05e
Compare
wscurran
added
area: ci
|
✨ Thanks for the fix, @HOYALIM. Locking the OpenClaw production graph improves supply-chain integrity for both image paths. Ready for maintainer review. Related open issues: Related open issues: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
664d170 to
27d36da
Compare
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
29386a3 to
fe37cce
Compare
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
Fixed the exact-head Validation: 25 workflow trust-boundary tests passed; YAML, repository, source-shape, lint, and pre-push checks passed. Documentation receipt: |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
Upstream publication status (2026-07-25): openclaw/openclaw#113584 tracks the same two published-graph findings covered here. OpenClaw |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approved at exact head cf8c889. The OpenClaw remediation and provenance boundary are unchanged from the reviewed implementation; this head adds only verified current-main merge commits, including the shared schema registration fix. No unresolved review threads remain, and the exact-head documentation receipt is current. Required CI, reviewed npm audit, image, and E2E gates must still pass before merge.
<!-- markdownlint-disable MD041 --> ## Summary Repair the required real-OpenClaw distribution harness after the production Dockerfile began using NemoClaw's shared version extractor. The harness now maps that container-only helper path to the same repository script used by focused test fixtures, so it exercises the production patch block instead of failing before patching starts. ## Related Issue Follow-up to #6830. ## Changes - Map `/usr/local/lib/nemoclaw/extract-semver` to `scripts/extract-semver.sh` when the real-distribution harness materializes the Dockerfile patch block outside the container. - Add a regression assertion that rejects an unresolved container-only helper path. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This repairs test-harness evidence only; shipped OpenClaw patching and user-facing behavior are unchanged. - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The exact change is limited to `test/openclaw-real-patched-dist-harness.test.ts` and does not alter shipped or user-facing behavior. - Agent: Codex Desktop <!-- docs-review-head-sha: 1dde473 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — the exact registry-backed suite passed 6/6 on Node 22.23.1, and the current installer hash contract passed including `openshell.rb`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable; the change only supplies one existing helper to a focused harness. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
openclaw@2026.7.1production closure as a NemoClaw-owned lock consumed by both image pathsbrace-expansion@5.0.7andfast-uri@3.1.2resolutions with reviewed5.0.8and3.1.4resolutions before lock generationValidation
npm run build:clinpm run typecheck22.23.1: production install with lifecycle scripts disabled, installed identities verified, reviewed lifecycle completed, andOpenClaw 2026.7.1confirmedLocal Docker image build was not run because the Docker daemon is unavailable; the required image workflow provides that proof.
References #5896
Signed-off-by: Ho Lim subhoya@gmail.com
Documentation Writer Review
docs-updateddocs/security/openclaw-2026.7.1-dependency-review.mddocuments the authoritative OpenClaw lock, reviewed dependency remediations, registry/SRI and installed-graph verification, CI audit boundary, and both image paths. The documented lock digest matches the exact refreshed head.