Skip to content

fix(onboard): harden BuildKit prebuild validation#6265

Merged
cv merged 4 commits into
mainfrom
codex/harden-buildkit-e2e-budget
Jul 4, 2026
Merged

fix(onboard): harden BuildKit prebuild validation#6265
cv merged 4 commits into
mainfrom
codex/harden-buildkit-e2e-budget

Conversation

@cv

@cv cv commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

This follow-up to #6166 validates the staged context before invoking host BuildKit and moves the hard #6002 cold-path acceptance assertions into the existing full-e2e lifecycle. It removes the redundant second onboarding run while preserving a distinct merge-failing signal alongside the advisory warm-system scorecard budget.

Related Issue

Follow-up to #6166 and #6002.

Changes

  • Resolve and validate BuildKit contexts as private direct os.tmpdir()/nemoclaw-build-* staging directories with a no-follow regular Dockerfile, falling back to the gateway builder when validation fails.
  • Carry generated/custom provenance into the handoff so user-supplied --from Dockerfiles remain on the OpenShell gateway-builder trust boundary.
  • Cover custom provenance, outside-temp, wrong-prefix, writable-directory, symlink, non-regular-file, path-escape, inspection-error logging, environment sanitization, and compatibility-heartbeat behavior.
  • Measure BuildKit success, fallback absence, output silence, and the first real agent response during the job's first full-e2e onboarding instead of running a second warm-cache onboarding test.
  • Remove the redundant Vitest invocation from e2e.yaml and document the hard 180-second cold-path contract separately from the advisory 390-second warm-system scorecard budget.

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:
  • 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: Reviewed every production context stager against the new contract; explicit provenance keeps custom Dockerfiles off host BuildKit, focused negative tests cover each trust-boundary case, and the unavoidable post-validation Docker pathname reopen is mitigated by private mkdtemp staging.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • 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: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added optional cold-onboarding performance measurement to the live end-to-end flow, including trace-based evidence output.
  • Bug Fixes
    • Hardened local prebuild handling for NemoClaw-generated staged build contexts and tightened eligibility/--from validation behavior.
    • Improved sandbox build-context metadata propagation and reused Docker-driver gateway detection.
  • Tests
    • Expanded/refocused live end-to-end coverage and performance assertions; added trace/probe parsing fixtures; improved test isolation and cleanup.
    • Updated e2e workflow and release-gate checks to target the correct live Vitest test.
  • Documentation
    • Clarified --from build-context routing vs local BuildKit behavior on local Docker-driver gateways.

@cv cv added the area: security Security controls, permissions, secrets, or hardening label Jul 4, 2026
@cv cv self-assigned this Jul 4, 2026
@coderabbitai

coderabbitai Bot commented Jul 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: 1130e8e9-f30f-4a77-9120-b667259bbc8a

📥 Commits

Reviewing files that changed from the base of the PR and between c51290a and 39774f2.

📒 Files selected for processing (13)
  • src/lib/actions/sandbox/rebuild-custom-image-preflight.test.ts
  • src/lib/actions/sandbox/rebuild-gpu-opt-out.test.ts
  • src/lib/actions/sandbox/rebuild-managed-image-preflight.test.ts
  • src/lib/onboard.ts
  • src/lib/onboard/build-context-stage.test.ts
  • src/lib/onboard/build-context-stage.ts
  • src/lib/onboard/prepared-dcode-rebuild.test.ts
  • src/lib/onboard/sandbox-create-launch.test.ts
  • src/lib/onboard/sandbox-prebuild.test.ts
  • src/lib/onboard/sandbox-prebuild.ts
  • src/lib/sandbox/build-context.ts
  • test/onboard-prepared-build-context.test.ts
  • test/onboard-prepared-gateway-handoff.test.ts
✅ Files skipped from review due to trivial changes (1)
  • src/lib/onboard/build-context-stage.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/lib/onboard/sandbox-create-launch.test.ts
  • src/lib/sandbox/build-context.ts
  • src/lib/onboard.ts
  • src/lib/onboard/sandbox-prebuild.test.ts

📝 Walkthrough

Walkthrough

This PR moves cold-onboard performance checks into full-e2e.test.ts, adds trace and payload helpers, hardens staged build-context validation, propagates build-context origin metadata, and updates related tests, workflow wiring, and docs.

Changes

Onboard performance and staged build-context hardening

Layer / File(s) Summary
Onboard trace helpers and support tests
test/e2e/fixtures/onboard-performance.ts, test/e2e/support/onboard-performance.test.ts, test/e2e/live/agent-turn-latency-helpers.ts
Adds helpers that parse onboard trace artifacts into a millisecond window and compute the maximum silence gap, with tests covering valid, invalid, and payload-extraction cases.
Cold onboarding measurement in full-e2e
test/e2e/live/full-e2e.test.ts, .github/workflows/e2e.yaml, test/e2e-release-gate-workflow.test.ts, test/e2e/README.md
Adds cold-onboarding capture, trace reading, silence measurement, installer signal parsing, and budget assertions to the live full-e2e test, and retargets the workflow and docs to run it.
Heartbeat reporter compatibility test
src/lib/onboard/machine/live-flow-slice.test.ts
Adds timer cleanup and a new test that keeps a pending gateway phase visible through the default heartbeat reporter.
Shared build-context prefix
src/lib/sandbox/build-context.ts, src/lib/agent/base-image.ts, src/lib/onboard/build-context-stage.ts
Exports a shared sandbox build-context prefix and uses it for temporary directories in the sandbox build context, agent sandbox, and staged build-context staging paths.
Trusted staged build-context prebuild
src/lib/onboard/sandbox-prebuild.ts, src/lib/onboard.ts
Extends prebuild input with build-context origin metadata, validates generated staged build contexts and Dockerfiles with filesystem trust checks, and uses the trusted paths for the local BuildKit invocation.
Prebuild and launch tests
src/lib/onboard/sandbox-prebuild.test.ts, src/lib/onboard/sandbox-create-launch.test.ts, src/lib/actions/sandbox/rebuild-*.test.ts, src/lib/onboard/build-context-stage.test.ts, src/lib/onboard/prepared-dcode-rebuild.test.ts, test/onboard-prepared-*.test.ts
Updates the sandbox create-launch and prebuild tests to generate temporary build contexts, cover staged-context edge cases, and assert the new fallback and BuildKit behaviors, plus the new origin field across related rebuild and handoff fixtures.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Suggested labels: bug-fix

Suggested reviewers: ericksoa, cjagwani

🚥 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 matches the main change: onboarding BuildKit prebuild validation was hardened.
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.
✨ 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/harden-buildkit-e2e-budget

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

@github-code-quality

github-code-quality Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the branch is 96%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 39774f2 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the branch is 69%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 39774f2 +/-
src/lib/shields...nsition-lock.ts 87%
src/lib/actions...all/run-plan.ts 81%
src/lib/state/o...oard-session.ts 78%
src/lib/state/sandbox.ts 74%
src/lib/onboard/preflight.ts 71%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/shields/index.ts 68%
src/lib/actions...licy-channel.ts 60%
src/lib/policy/index.ts 60%
src/lib/onboard.ts 22%

Updated July 04, 2026 05:16 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-3: TOCTOU race in resolveTrustedStagedBuildContext allows symlink swap; then add or justify PRA-T1.
Open items: 4 required · 7 warnings · 7 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 8 still apply · 5 new items found

Action checklist

  • PRA-3 Fix: TOCTOU race in resolveTrustedStagedBuildContext allows symlink swap in src/lib/onboard/sandbox-prebuild.ts:62
  • PRA-4 Fix: O_NONBLOCK flag has no effect on regular file open and may mask errors in src/lib/onboard/sandbox-prebuild.ts:71
  • PRA-5 Fix: resolveTrustedStagedBuildContext has 7 silent validation failure paths without debug logging in src/lib/onboard/sandbox-prebuild.ts:51
  • PRA-6 Fix: MEASURE_COLD_ONBOARD env-derived flag is always true in this test target in test/e2e/live/full-e2e.test.ts:41
  • PRA-1 Resolve or justify: Source-of-truth review needed: resolveTrustedStagedBuildContext silent validation returns (7 paths)
  • PRA-2 Resolve or justify: Source-of-truth review needed: MEASURE_COLD_ONBOARD env-derived flag in full-e2e.test.ts
  • PRA-7 Resolve or justify: Seven validation failure paths return null without logging specific reason in src/lib/onboard/sandbox-prebuild.ts:55
  • PRA-8 Resolve or justify: Caller does not distinguish validation failures from exceptions in src/lib/onboard/sandbox-prebuild.ts:105
  • PRA-9 Resolve or justify: MEASURE_COLD_ONBOARD derived from env but only used in full-e2e target in test/e2e/live/full-e2e.test.ts:41
  • PRA-10 Resolve or justify: Duplicate test fixture createTrustedBuildContext across two test files in src/lib/onboard/sandbox-create-launch.test.ts:21
  • PRA-11 Resolve or justify: readOnboardTraceWindow throws generic error for missing/malformed trace in test/e2e/fixtures/onboard-performance.ts:63
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Duplicate test fixture createTrustedBuildContext across two test files
  • PRA-T7 Add or justify test follow-up: readOnboardTraceWindow throws generic error for missing/malformed trace
  • PRA-T8 Add or justify test follow-up: Missing errno-specific tests for EACCES, ENOSPC, EIO in trust validation
  • PRA-12 In-scope improvement: Missing errno-specific tests for EACCES, ENOSPC, EIO in trust validation in src/lib/onboard/sandbox-prebuild.test.ts:272
  • PRA-13 In-scope improvement: Verify all original onboard-progress-budget assertions preserved in assertColdOnboardPerformance in test/e2e/live/full-e2e.test.ts:180
  • PRA-14 In-scope improvement: Source-of-truth comment missing for resolveTrustedStagedBuildContext silent validation returns in src/lib/onboard/sandbox-prebuild.ts:51
  • PRA-15 In-scope improvement: Source-of-truth comment missing for MEASURE_COLD_ONBOARD env-derived flag in test/e2e/live/full-e2e.test.ts:41
  • PRA-16 In-scope improvement: Trust validation tests could be parameterized to reduce monolith growth in src/lib/onboard/sandbox-prebuild.test.ts:1
  • PRA-17 In-scope improvement: Documentation Note about gateway builder for --from could cross-reference build-context staging in docs/reference/commands.mdx:400
  • PRA-18 In-scope improvement: Workflow removed duplicate onboard-progress-budget.test.ts run; validation confirmed in .github/workflows/e2e.yaml:2704

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Required security src/lib/onboard/sandbox-prebuild.ts:62 Remove the realpathSync(dockerfile) call on line 62. Open the Dockerfile directly with fs.openSync(dockerfile, O_RDONLY | O_NOFOLLOW) and use the returned fd for fstatSync verification. This makes the symlink check atomic with the open.
PRA-4 Required security src/lib/onboard/sandbox-prebuild.ts:71 Remove O_NONBLOCK entirely. Use only O_RDONLY | O_NOFOLLOW.
PRA-5 Required architecture src/lib/onboard/sandbox-prebuild.ts:51 Add debug-level log for each validation failure with a distinct reason code before each return null. Document source-of-truth rationale in code comment.
PRA-6 Required architecture test/e2e/live/full-e2e.test.ts:41 Replace with const MEASURE_COLD_ONBOARD = true; since this test only runs in the full-e2e target. Add comment: 'This test only runs as full-e2e target; cold measurement is unconditional here.'
PRA-7 Resolve/justify security src/lib/onboard/sandbox-prebuild.ts:55 Add debug-level log for each validation failure with a distinct reason code before each return null. Reason codes: 'parent-not-temp-root', 'wrong-prefix', 'not-directory', 'writable-directory', 'dockerfile-escapes-context', 'dockerfile-not-regular-file', 'no-follow-unsupported'.
PRA-8 Resolve/justify security src/lib/onboard/sandbox-prebuild.ts:105 In the catch block, log at debug level with 'exception' tag. In the null-check block, log at debug level with 'validation' tag and include the specific reason if available. Consider returning a discriminated union { kind: 'validation', reason } | { kind: 'exception', error } instead of null.
PRA-9 Resolve/justify correctness test/e2e/live/full-e2e.test.ts:41 Replace with const MEASURE_COLD_ONBOARD = true; and add explanatory comment.
PRA-10 Resolve/justify tests src/lib/onboard/sandbox-create-launch.test.ts:21 Extract to shared fixture: test/e2e/fixtures/trusted-build-context.ts exporting createTrustedBuildContext(prefix?: string). Both test files import and use it.
PRA-11 Resolve/justify tests test/e2e/fixtures/onboard-performance.ts:63 Enhance error messages to include: expected trace format (OTLP with resource_spans/scope_spans/spans), expected span name (nemoclaw.onboard), expected status (OK). Example: 'Trace file missing resource_spans — expected OTLP format with resource_spans[].scope_spans[].spans[] containing span name "nemoclaw.onboard" with status OK. Installer may have failed to write trace.'
PRA-12 Improvement tests src/lib/onboard/sandbox-prebuild.test.ts:272 Add three tests: 'logs filesystem permission error (EACCES) distinctly before falling back', 'logs disk full error (ENOSPC) distinctly before falling back', 'logs I/O error (EIO) distinctly before falling back'. Mock fs.openSync to throw each errno.
PRA-13 Improvement tests test/e2e/live/full-e2e.test.ts:180 Audit original test (deleted in this PR) vs current assertColdOnboardPerformance. Document mapping in code comment. If any missing, add back.
PRA-14 Improvement architecture src/lib/onboard/sandbox-prebuild.ts:51 Add code comment at function start explaining: what invalid state is handled (untrusted build context), where that state is created (build context staging), why source cannot be fixed (defense-in-depth against TOCTOU), what regression test proves it (trust validation tests), when workaround can be removed (when OpenShell uses BuildKit for local-driver path, #6258).
PRA-15 Improvement architecture test/e2e/live/full-e2e.test.ts:41 Add comment explaining why env-derived flag is used (shared test file, only full-e2e target measures cold). This satisfies PRA-2.
PRA-16 Improvement tests src/lib/onboard/sandbox-prebuild.test.ts:1 Review test structure for parameterization opportunities. The 11 new trust validation tests follow a similar pattern and could use it.each with a matrix of { name, mutateContext, expectedLog }.
PRA-17 Improvement correctness docs/reference/commands.mdx:400 Add cross-reference to the build-context staging docs for users who want to understand the trust boundary.
PRA-18 Improvement workflow .github/workflows/e2e.yaml:2704 No action needed. Confirm workflow test validates the consolidation.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-3 Required — TOCTOU race in resolveTrustedStagedBuildContext allows symlink swap

  • Location: src/lib/onboard/sandbox-prebuild.ts:62
  • Category: security
  • Problem: Lines 62-73: fs.realpathSync(dockerfile) resolves the path, then fs.openSync(dockerfile, O_RDONLY | O_NOFOLLOW | O_NONBLOCK) opens it. An attacker with write access to the staged build context directory can replace the Dockerfile with a symlink between these two calls. The subsequent fstatSync verifies the opened file's inode, but the file descriptor was already obtained after the symlink swap.
  • Impact: Local attacker who can write to the build context temp directory could cause NemoClaw to build a different Dockerfile than validated, potentially injecting malicious build instructions that execute in the host Docker daemon context during BuildKit build.
  • Required action: Remove the realpathSync(dockerfile) call on line 62. Open the Dockerfile directly with fs.openSync(dockerfile, O_RDONLY | O_NOFOLLOW) and use the returned fd for fstatSync verification. This makes the symlink check atomic with the open.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts lines 51-85: resolveTrustedStagedBuildContext should open with O_NOFOLLOW first, then fstat the fd. No separate realpathSync on the dockerfile path before open.
  • Missing regression test: Add test that creates a staged build context, then replaces Dockerfile with symlink to external file between validation and build; verify build is rejected or external file not used.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts lines 51-85: resolveTrustedStagedBuildContext should open with O_NOFOLLOW first, then fstat the fd. No separate realpathSync on the dockerfile path before open.
  • Evidence: sandbox-prebuild.ts:62 does realpathSync(dockerfile); line 73 does openSync(dockerfile, O_RDONLY | O_NOFOLLOW | O_NONBLOCK); line 75 fstatSync(descriptor) verifies isFile() but fd already opened

PRA-4 Required — O_NONBLOCK flag has no effect on regular file open and may mask errors

  • Location: src/lib/onboard/sandbox-prebuild.ts:71
  • Category: security
  • Problem: Line 71: const nonBlocking = typeof fs.constants.O_NONBLOCK === 'number' ? fs.constants.O_NONBLOCK : 0; Line 73 includes nonBlocking in openSync flags. O_NONBLOCK only affects FIFOs, sockets, and device files per POSIX. On regular files it is ignored, but on some platforms may cause open to succeed where it should fail (e.g., mandatory locking).
  • Impact: Unnecessary flag adds complexity and potential for platform-specific behavior differences. No security benefit for regular Dockerfile validation.
  • Required action: Remove O_NONBLOCK entirely. Use only O_RDONLY | O_NOFOLLOW.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts line 71-73: nonBlocking constant and its use in openSync flags should be removed.
  • Missing regression test: Test that openSync with O_RDONLY | O_NOFOLLOW (no O_NONBLOCK) still rejects symlinked Dockerfiles and accepts regular files.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts line 71-73: nonBlocking constant and its use in openSync flags should be removed.
  • Evidence: sandbox-prebuild.ts:71 defines nonBlocking from O_NONBLOCK; line 73 includes it in openSync flags

PRA-5 Required — resolveTrustedStagedBuildContext has 7 silent validation failure paths without debug logging

  • Location: src/lib/onboard/sandbox-prebuild.ts:51
  • Category: architecture
  • Problem: Function returns null silently for 7 validation failures: parent-not-temp-root, wrong-prefix, not-directory, writable-directory, dockerfile-escapes-context, dockerfile-not-regular-file, no-follow-unsupported. This is a localized workaround for trust validation failures. The invalid state handled: untrusted build context. The source boundary is the build context staging in build-context-stage.ts and createAgentSandbox. The source cannot be fixed in this PR because the staging code is trusted but the validation is defense-in-depth against TOCTOU and attacker-controlled tmp.
  • Impact: Operators cannot distinguish why local BuildKit build was skipped; debugging trust failures requires code inspection.
  • Required action: Add debug-level log for each validation failure with a distinct reason code before each return null. Document source-of-truth rationale in code comment.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts lines 51-85: each early return null should have a preceding log statement with a reason code.
  • Missing regression test: Existing trust validation tests in sandbox-prebuild.test.ts cover each failure mode; ensure they assert the debug log output.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts lines 51-85: each early return null should have a preceding log statement with a reason code.
  • Evidence: Lines 55, 58, 61, 64, 67, 70, 79 each return null without logging the specific reason

PRA-6 Required — MEASURE_COLD_ONBOARD env-derived flag is always true in this test target

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: architecture
  • Problem: const MEASURE_COLD_ONBOARD = process.env.E2E_TARGET_ID === 'full-e2e'; but this test only runs in the full-e2e target where it's always true. This is a localized workaround for sharing the test file across targets. The invalid state handled: cold measurement only needed for full-e2e target. The source boundary is the CI workflow that sets E2E_TARGET_ID. The source cannot be fixed in this PR because the test file is shared.
  • Impact: Misleading code suggests conditional behavior where there is none; future maintainers may think other targets use this flag.
  • Required action: Replace with const MEASURE_COLD_ONBOARD = true; since this test only runs in the full-e2e target. Add comment: 'This test only runs as full-e2e target; cold measurement is unconditional here.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check full-e2e.test.ts line 41: should be a constant true with explanatory comment.
  • Missing regression test: N/A - this is a constant simplification, no new test needed.
  • Done when: The required change is committed and verification passes: Check full-e2e.test.ts line 41: should be a constant true with explanatory comment.
  • Evidence: Line 41: const MEASURE_COLD_ONBOARD = process.env.E2E_TARGET_ID === 'full-e2e'; line 136 uses it but test only runs in full-e2e job
Review findings by urgency: 4 required fixes, 7 items to resolve/justify, 7 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: resolveTrustedStagedBuildContext silent validation returns (7 paths)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: 11 new trust validation tests in sandbox-prebuild.test.ts cover each failure mode
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Function at sandbox-prebuild.ts:51 lacks source-of-truth comment; PRA-1, PRA-12 track this

PRA-2 Resolve/justify — Source-of-truth review needed: MEASURE_COLD_ONBOARD env-derived flag in full-e2e.test.ts

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Test only runs in full-e2e job
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 41 uses process.env but test only runs in full-e2e job; PRA-2, PRA-13 track this

PRA-7 Resolve/justify — Seven validation failure paths return null without logging specific reason

  • Location: src/lib/onboard/sandbox-prebuild.ts:55
  • Category: security
  • Problem: Each validation check that returns null (lines 55, 58, 61, 64, 67, 70, 79) logs nothing. Caller (prebuildSandboxImageIfEligible) logs generic 'failed trust validation' message but loses the specific reason.
  • Impact: No visibility into which trust check failed; operators see only generic fallback message.
  • Recommended action: Add debug-level log for each validation failure with a distinct reason code before each return null. Reason codes: 'parent-not-temp-root', 'wrong-prefix', 'not-directory', 'writable-directory', 'dockerfile-escapes-context', 'dockerfile-not-regular-file', 'no-follow-unsupported'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check sandbox-prebuild.ts lines 55-80: each validation check that returns null should log the specific reason.
  • Missing regression test: Extend existing trust validation tests to assert debug log output contains the expected reason code for each failure mode.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check sandbox-prebuild.ts lines 55-80: each validation check that returns null should log the specific reason.
  • Evidence: Lines 55, 58, 61, 64, 67, 70, 79 each return null without logging

PRA-8 Resolve/justify — Caller does not distinguish validation failures from exceptions

  • Location: src/lib/onboard/sandbox-prebuild.ts:105
  • Category: security
  • Problem: prebuildSandboxImageIfEligible catches all exceptions from resolveTrustedStagedBuildContext but does not distinguish from validation failures (null returns). Both paths log generic 'could not be inspected' or 'failed trust validation' messages.
  • Impact: Cannot differentiate filesystem errors (EMFILE, EACCES) from trust validation failures in logs; debugging requires code inspection.
  • Recommended action: In the catch block, log at debug level with 'exception' tag. In the null-check block, log at debug level with 'validation' tag and include the specific reason if available. Consider returning a discriminated union { kind: 'validation', reason } | { kind: 'exception', error } instead of null.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check sandbox-prebuild.ts lines 100-115: catch block and null check should have distinct log tags.
  • Missing regression test: Add test that mocks fs.openSync to throw EMFILE and verify 'exception' tag in log; add test for each validation failure and verify 'validation' tag with reason code.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check sandbox-prebuild.ts lines 100-115: catch block and null check should have distinct log tags.
  • Evidence: Lines 100-115: try/catch around resolveTrustedStagedBuildContext; both catch and null check log generic messages

PRA-9 Resolve/justify — MEASURE_COLD_ONBOARD derived from env but only used in full-e2e target

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: correctness
  • Problem: Same as PRA-2 but categorized as correctness: the env-derived flag is always true in this test context.
  • Impact: Dead code path confusion; misleading conditional.
  • Recommended action: Replace with const MEASURE_COLD_ONBOARD = true; and add explanatory comment.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check full-e2e.test.ts line 41: should be const MEASURE_COLD_ONBOARD = true;
  • Missing regression test: N/A
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check full-e2e.test.ts line 41: should be const MEASURE_COLD_ONBOARD = true;.
  • Evidence: Line 41 uses process.env but test only runs in full-e2e job

PRA-10 Resolve/justify — Duplicate test fixture createTrustedBuildContext across two test files

  • Location: src/lib/onboard/sandbox-create-launch.test.ts:21
  • Category: tests
  • Problem: createTrustedBuildContext in sandbox-create-launch.test.ts and createBuildContext in sandbox-prebuild.test.ts both create a temp directory with SANDBOX_BUILD_CONTEXT_PREFIX and write a Dockerfile.
  • Impact: Maintenance burden; divergence risk if fixture logic changes.
  • Recommended action: Extract to shared fixture: test/e2e/fixtures/trusted-build-context.ts exporting createTrustedBuildContext(prefix?: string). Both test files import and use it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check both test files: they should import from shared fixture instead of defining their own.
  • Missing regression test: N/A - refactoring only
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check both test files: they should import from shared fixture instead of defining their own.
  • Evidence: sandbox-create-launch.test.ts:21 defines createTrustedBuildContext; sandbox-prebuild.test.ts:27 defines createBuildContext with identical logic

PRA-11 Resolve/justify — readOnboardTraceWindow throws generic error for missing/malformed trace

  • Location: test/e2e/fixtures/onboard-performance.ts:63
  • Category: tests
  • Problem: Error messages: 'trace artifact is missing resource_spans' or 'trace artifact must contain exactly one onboard root span' lack actionable guidance for operators.
  • Impact: Operators cannot diagnose why trace file is invalid; must inspect code to understand expected format.
  • Recommended action: Enhance error messages to include: expected trace format (OTLP with resource_spans/scope_spans/spans), expected span name (nemoclaw.onboard), expected status (OK). Example: 'Trace file missing resource_spans — expected OTLP format with resource_spans[].scope_spans[].spans[] containing span name "nemoclaw.onboard" with status OK. Installer may have failed to write trace.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check onboard-performance.ts lines 30-70: error messages should be more descriptive.
  • Missing regression test: Add test that passes malformed trace artifacts and verifies error messages contain actionable guidance.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check onboard-performance.ts lines 30-70: error messages should be more descriptive.
  • Evidence: Lines 32, 44, 50, 56, 60 throw generic errors without expected format guidance

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-12 Improvement — Missing errno-specific tests for EACCES, ENOSPC, EIO in trust validation

  • Location: src/lib/onboard/sandbox-prebuild.test.ts:272
  • Category: tests
  • Problem: New tests cover validation logic failures but not specific errno codes from fs operations.
  • Impact: Filesystem permission, disk full, and I/O errors during trust validation are not tested for distinct logging behavior.
  • Suggested action: Add three tests: 'logs filesystem permission error (EACCES) distinctly before falling back', 'logs disk full error (ENOSPC) distinctly before falling back', 'logs I/O error (EIO) distinctly before falling back'. Mock fs.openSync to throw each errno.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.test.ts: should have tests mocking fs.openSync to throw EACCES, ENOSPC, EIO and asserting log output.
  • Missing regression test: Add the three errno-specific tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: No tests mock fs.openSync with errno codes; existing test at line 187 mocks EMFILE only

PRA-13 Improvement — Verify all original onboard-progress-budget assertions preserved in assertColdOnboardPerformance

  • Location: test/e2e/live/full-e2e.test.ts:180
  • Category: tests
  • Problem: Original onboard-progress-budget.test.ts was deleted in this PR. Need to verify all its assertions are preserved in assertColdOnboardPerformance.
  • Impact: Potential regression in cold-path budget guarantees if assertions were dropped.
  • Suggested action: Audit original test (deleted in this PR) vs current assertColdOnboardPerformance. Document mapping in code comment. If any missing, add back.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare deleted onboard-progress-budget.test.ts (from git history) with current assertColdOnboardPerformance function in full-e2e.test.ts.
  • Missing regression test: Ensure all original budget assertions have corresponding checks in assertColdOnboardPerformance.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard-progress-budget.test.ts deleted; assertColdOnboardPerformance at line 180 replaces it

PRA-14 Improvement — Source-of-truth comment missing for resolveTrustedStagedBuildContext silent validation returns

  • Location: src/lib/onboard/sandbox-prebuild.ts:51
  • Category: architecture
  • Problem: Function is a localized workaround but lacks documentation of: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed.
  • Impact: Future maintainers may not understand the defense-in-depth rationale or removal criteria.
  • Suggested action: Add code comment at function start explaining: what invalid state is handled (untrusted build context), where that state is created (build context staging), why source cannot be fixed (defense-in-depth against TOCTOU), what regression test proves it (trust validation tests), when workaround can be removed (when OpenShell uses BuildKit for local-driver path, refactor(onboard): extract sandbox creation orchestration from the entrypoint #6258).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.ts line 51: function should have source-of-truth comment block.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function at line 51 has no source-of-truth comment block

PRA-15 Improvement — Source-of-truth comment missing for MEASURE_COLD_ONBOARD env-derived flag

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: architecture
  • Problem: Env-derived flag is a localized workaround but lacks documentation of why it's used (shared test file, only full-e2e target measures cold).
  • Impact: Future maintainers may not understand the sharing constraint or removal criteria.
  • Suggested action: Add comment explaining why env-derived flag is used (shared test file, only full-e2e target measures cold). This satisfies PRA-2.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check full-e2e.test.ts line 41: should have explanatory comment.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 41 has no comment explaining the workaround rationale

PRA-16 Improvement — Trust validation tests could be parameterized to reduce monolith growth

  • Location: src/lib/onboard/sandbox-prebuild.test.ts:1
  • Category: tests
  • Problem: Test file grew from 166 to 385 lines (+219). The 11 new trust validation tests follow a similar pattern and could use a test matrix.
  • Impact: Monolith growth makes test file harder to maintain; parameterization would reduce duplication.
  • Suggested action: Review test structure for parameterization opportunities. The 11 new trust validation tests follow a similar pattern and could use it.each with a matrix of { name, mutateContext, expectedLog }.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.test.ts: look for repeated test patterns that could be consolidated with it.each.
  • Missing regression test: N/A - code quality improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 63-270: 11 tests with nearly identical structure (create context, mutate, call prebuildSandboxImageIfEligible, assert fallback)

PRA-17 Improvement — Documentation Note about gateway builder for --from could cross-reference build-context staging

  • Location: docs/reference/commands.mdx:400
  • Category: correctness
  • Problem: Note added about OpenShell gateway builder for custom --from Dockerfiles. Good user-facing change but could also clarify that local BuildKit prebuild is reserved for NemoClaw-generated contexts only and cross-reference build-context staging docs.
  • Impact: Users may not understand the trust boundary rationale or where to learn more.
  • Suggested action: Add cross-reference to the build-context staging docs for users who want to understand the trust boundary.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check commands.mdx and commands-nemohermes.mdx around line 400: Note block should be present with cross-reference.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Note block at line 400 mentions gateway builder but no cross-reference

PRA-18 Improvement — Workflow removed duplicate onboard-progress-budget.test.ts run; validation confirmed

  • Location: .github/workflows/e2e.yaml:2704
  • Category: workflow
  • Problem: Workflow removed duplicate onboard-progress-budget.test.ts run from full-e2e job. Confirmed by test/e2e-release-gate-workflow.test.ts validation.
  • Impact: Reduced CI time; no functional change.
  • Suggested action: No action needed. Confirm workflow test validates the consolidation.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/e2e-release-gate-workflow.test.ts lines 25-28: should assert full-e2e job no longer runs onboard-progress-budget.test.ts.
  • Missing regression test: N/A
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: e2e.yaml diff shows removal; test/e2e-release-gate-workflow.test.ts validates
Simplification opportunities: 4 possible cuts, net -183 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-4 stdlib (src/lib/onboard/sandbox-prebuild.ts:71): const nonBlocking = typeof fs.constants.O_NONBLOCK === 'number' ? fs.constants.O_NONBLOCK : 0; and its use in openSync flags
    • Replacement: Use only fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW
    • Net: -3 lines
    • Safety boundary: Must keep O_NOFOLLOW for atomic symlink check; O_RDONLY for read-only open
  • PRA-6 shrink (test/e2e/live/full-e2e.test.ts:41): const MEASURE_COLD_ONBOARD = process.env.E2E_TARGET_ID === 'full-e2e';
    • Replacement: const MEASURE_COLD_ONBOARD = true; // This test only runs as full-e2e target; cold measurement is unconditional here
    • Net: 0 lines
    • Safety boundary: Must not affect other test targets; this file is only run in full-e2e job
  • PRA-10 delete (src/lib/onboard/sandbox-create-launch.test.ts:21): function createTrustedBuildContext() { ... } in sandbox-create-launch.test.ts (lines 21-26) and function createBuildContext() in sandbox-prebuild.test.ts (lines 27-35)
    • Replacement: import { createTrustedBuildContext } from '../../test/e2e/fixtures/trusted-build-context'
    • Net: -30 lines
    • Safety boundary: Shared fixture must use SANDBOX_BUILD_CONTEXT_PREFIX and create Dockerfile identically
  • PRA-16 shrink (src/lib/onboard/sandbox-prebuild.test.ts:1): 11 individual it() blocks for trust validation (lines 63-270)
    • Replacement: it.each matrix with { name, mutateFn, expectedLogContains } and single test body
    • Net: -150 lines
    • Safety boundary: Each test case must still assert the exact fallback behavior and log output for its specific failure mode
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — resolveTrustedStagedBuildContext logs parent-not-temp-root validation failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, docs/deployment/install-openclaw-plugins.mdx, docs/manage-sandboxes/install-plugins-hermes.mdx, docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/agent/base-image.ts, src/lib/onboard.ts, src/lib/onboard/build-context-stage.ts.
  • PRA-T2 Runtime validation — resolveTrustedStagedBuildContext logs wrong-prefix validation failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, docs/deployment/install-openclaw-plugins.mdx, docs/manage-sandboxes/install-plugins-hermes.mdx, docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/agent/base-image.ts, src/lib/onboard.ts, src/lib/onboard/build-context-stage.ts.
  • PRA-T3 Runtime validation — resolveTrustedStagedBuildContext logs not-directory validation failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, docs/deployment/install-openclaw-plugins.mdx, docs/manage-sandboxes/install-plugins-hermes.mdx, docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/agent/base-image.ts, src/lib/onboard.ts, src/lib/onboard/build-context-stage.ts.
  • PRA-T4 Runtime validation — resolveTrustedStagedBuildContext logs writable-directory validation failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, docs/deployment/install-openclaw-plugins.mdx, docs/manage-sandboxes/install-plugins-hermes.mdx, docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/agent/base-image.ts, src/lib/onboard.ts, src/lib/onboard/build-context-stage.ts.
  • PRA-T5 Runtime validation — resolveTrustedStagedBuildContext logs dockerfile-escapes-context validation failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, docs/deployment/install-openclaw-plugins.mdx, docs/manage-sandboxes/install-plugins-hermes.mdx, docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/agent/base-image.ts, src/lib/onboard.ts, src/lib/onboard/build-context-stage.ts.
  • PRA-T6 Duplicate test fixture createTrustedBuildContext across two test files — Extract to shared fixture: test/e2e/fixtures/trusted-build-context.ts exporting createTrustedBuildContext(prefix?: string). Both test files import and use it.
  • PRA-T7 readOnboardTraceWindow throws generic error for missing/malformed trace — Enhance error messages to include: expected trace format (OTLP with resource_spans/scope_spans/spans), expected span name (nemoclaw.onboard), expected status (OK). Example: 'Trace file missing resource_spans — expected OTLP format with resource_spans[].scope_spans[].spans[] containing span name "nemoclaw.onboard" with status OK. Installer may have failed to write trace.'
  • PRA-T8 Missing errno-specific tests for EACCES, ENOSPC, EIO in trust validation — Add three tests: 'logs filesystem permission error (EACCES) distinctly before falling back', 'logs disk full error (ENOSPC) distinctly before falling back', 'logs I/O error (EIO) distinctly before falling back'. Mock fs.openSync to throw each errno.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: resolveTrustedStagedBuildContext silent validation returns (7 paths)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: 11 new trust validation tests in sandbox-prebuild.test.ts cover each failure mode
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Function at sandbox-prebuild.ts:51 lacks source-of-truth comment; PRA-1, PRA-12 track this

PRA-2 Resolve/justify — Source-of-truth review needed: MEASURE_COLD_ONBOARD env-derived flag in full-e2e.test.ts

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Test only runs in full-e2e job
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 41 uses process.env but test only runs in full-e2e job; PRA-2, PRA-13 track this

PRA-3 Required — TOCTOU race in resolveTrustedStagedBuildContext allows symlink swap

  • Location: src/lib/onboard/sandbox-prebuild.ts:62
  • Category: security
  • Problem: Lines 62-73: fs.realpathSync(dockerfile) resolves the path, then fs.openSync(dockerfile, O_RDONLY | O_NOFOLLOW | O_NONBLOCK) opens it. An attacker with write access to the staged build context directory can replace the Dockerfile with a symlink between these two calls. The subsequent fstatSync verifies the opened file's inode, but the file descriptor was already obtained after the symlink swap.
  • Impact: Local attacker who can write to the build context temp directory could cause NemoClaw to build a different Dockerfile than validated, potentially injecting malicious build instructions that execute in the host Docker daemon context during BuildKit build.
  • Required action: Remove the realpathSync(dockerfile) call on line 62. Open the Dockerfile directly with fs.openSync(dockerfile, O_RDONLY | O_NOFOLLOW) and use the returned fd for fstatSync verification. This makes the symlink check atomic with the open.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts lines 51-85: resolveTrustedStagedBuildContext should open with O_NOFOLLOW first, then fstat the fd. No separate realpathSync on the dockerfile path before open.
  • Missing regression test: Add test that creates a staged build context, then replaces Dockerfile with symlink to external file between validation and build; verify build is rejected or external file not used.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts lines 51-85: resolveTrustedStagedBuildContext should open with O_NOFOLLOW first, then fstat the fd. No separate realpathSync on the dockerfile path before open.
  • Evidence: sandbox-prebuild.ts:62 does realpathSync(dockerfile); line 73 does openSync(dockerfile, O_RDONLY | O_NOFOLLOW | O_NONBLOCK); line 75 fstatSync(descriptor) verifies isFile() but fd already opened

PRA-4 Required — O_NONBLOCK flag has no effect on regular file open and may mask errors

  • Location: src/lib/onboard/sandbox-prebuild.ts:71
  • Category: security
  • Problem: Line 71: const nonBlocking = typeof fs.constants.O_NONBLOCK === 'number' ? fs.constants.O_NONBLOCK : 0; Line 73 includes nonBlocking in openSync flags. O_NONBLOCK only affects FIFOs, sockets, and device files per POSIX. On regular files it is ignored, but on some platforms may cause open to succeed where it should fail (e.g., mandatory locking).
  • Impact: Unnecessary flag adds complexity and potential for platform-specific behavior differences. No security benefit for regular Dockerfile validation.
  • Required action: Remove O_NONBLOCK entirely. Use only O_RDONLY | O_NOFOLLOW.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts line 71-73: nonBlocking constant and its use in openSync flags should be removed.
  • Missing regression test: Test that openSync with O_RDONLY | O_NOFOLLOW (no O_NONBLOCK) still rejects symlinked Dockerfiles and accepts regular files.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts line 71-73: nonBlocking constant and its use in openSync flags should be removed.
  • Evidence: sandbox-prebuild.ts:71 defines nonBlocking from O_NONBLOCK; line 73 includes it in openSync flags

PRA-5 Required — resolveTrustedStagedBuildContext has 7 silent validation failure paths without debug logging

  • Location: src/lib/onboard/sandbox-prebuild.ts:51
  • Category: architecture
  • Problem: Function returns null silently for 7 validation failures: parent-not-temp-root, wrong-prefix, not-directory, writable-directory, dockerfile-escapes-context, dockerfile-not-regular-file, no-follow-unsupported. This is a localized workaround for trust validation failures. The invalid state handled: untrusted build context. The source boundary is the build context staging in build-context-stage.ts and createAgentSandbox. The source cannot be fixed in this PR because the staging code is trusted but the validation is defense-in-depth against TOCTOU and attacker-controlled tmp.
  • Impact: Operators cannot distinguish why local BuildKit build was skipped; debugging trust failures requires code inspection.
  • Required action: Add debug-level log for each validation failure with a distinct reason code before each return null. Document source-of-truth rationale in code comment.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check sandbox-prebuild.ts lines 51-85: each early return null should have a preceding log statement with a reason code.
  • Missing regression test: Existing trust validation tests in sandbox-prebuild.test.ts cover each failure mode; ensure they assert the debug log output.
  • Done when: The required change is committed and verification passes: Check sandbox-prebuild.ts lines 51-85: each early return null should have a preceding log statement with a reason code.
  • Evidence: Lines 55, 58, 61, 64, 67, 70, 79 each return null without logging the specific reason

PRA-6 Required — MEASURE_COLD_ONBOARD env-derived flag is always true in this test target

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: architecture
  • Problem: const MEASURE_COLD_ONBOARD = process.env.E2E_TARGET_ID === 'full-e2e'; but this test only runs in the full-e2e target where it's always true. This is a localized workaround for sharing the test file across targets. The invalid state handled: cold measurement only needed for full-e2e target. The source boundary is the CI workflow that sets E2E_TARGET_ID. The source cannot be fixed in this PR because the test file is shared.
  • Impact: Misleading code suggests conditional behavior where there is none; future maintainers may think other targets use this flag.
  • Required action: Replace with const MEASURE_COLD_ONBOARD = true; since this test only runs in the full-e2e target. Add comment: 'This test only runs as full-e2e target; cold measurement is unconditional here.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check full-e2e.test.ts line 41: should be a constant true with explanatory comment.
  • Missing regression test: N/A - this is a constant simplification, no new test needed.
  • Done when: The required change is committed and verification passes: Check full-e2e.test.ts line 41: should be a constant true with explanatory comment.
  • Evidence: Line 41: const MEASURE_COLD_ONBOARD = process.env.E2E_TARGET_ID === 'full-e2e'; line 136 uses it but test only runs in full-e2e job

PRA-7 Resolve/justify — Seven validation failure paths return null without logging specific reason

  • Location: src/lib/onboard/sandbox-prebuild.ts:55
  • Category: security
  • Problem: Each validation check that returns null (lines 55, 58, 61, 64, 67, 70, 79) logs nothing. Caller (prebuildSandboxImageIfEligible) logs generic 'failed trust validation' message but loses the specific reason.
  • Impact: No visibility into which trust check failed; operators see only generic fallback message.
  • Recommended action: Add debug-level log for each validation failure with a distinct reason code before each return null. Reason codes: 'parent-not-temp-root', 'wrong-prefix', 'not-directory', 'writable-directory', 'dockerfile-escapes-context', 'dockerfile-not-regular-file', 'no-follow-unsupported'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check sandbox-prebuild.ts lines 55-80: each validation check that returns null should log the specific reason.
  • Missing regression test: Extend existing trust validation tests to assert debug log output contains the expected reason code for each failure mode.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check sandbox-prebuild.ts lines 55-80: each validation check that returns null should log the specific reason.
  • Evidence: Lines 55, 58, 61, 64, 67, 70, 79 each return null without logging

PRA-8 Resolve/justify — Caller does not distinguish validation failures from exceptions

  • Location: src/lib/onboard/sandbox-prebuild.ts:105
  • Category: security
  • Problem: prebuildSandboxImageIfEligible catches all exceptions from resolveTrustedStagedBuildContext but does not distinguish from validation failures (null returns). Both paths log generic 'could not be inspected' or 'failed trust validation' messages.
  • Impact: Cannot differentiate filesystem errors (EMFILE, EACCES) from trust validation failures in logs; debugging requires code inspection.
  • Recommended action: In the catch block, log at debug level with 'exception' tag. In the null-check block, log at debug level with 'validation' tag and include the specific reason if available. Consider returning a discriminated union { kind: 'validation', reason } | { kind: 'exception', error } instead of null.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check sandbox-prebuild.ts lines 100-115: catch block and null check should have distinct log tags.
  • Missing regression test: Add test that mocks fs.openSync to throw EMFILE and verify 'exception' tag in log; add test for each validation failure and verify 'validation' tag with reason code.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check sandbox-prebuild.ts lines 100-115: catch block and null check should have distinct log tags.
  • Evidence: Lines 100-115: try/catch around resolveTrustedStagedBuildContext; both catch and null check log generic messages

PRA-9 Resolve/justify — MEASURE_COLD_ONBOARD derived from env but only used in full-e2e target

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: correctness
  • Problem: Same as PRA-2 but categorized as correctness: the env-derived flag is always true in this test context.
  • Impact: Dead code path confusion; misleading conditional.
  • Recommended action: Replace with const MEASURE_COLD_ONBOARD = true; and add explanatory comment.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check full-e2e.test.ts line 41: should be const MEASURE_COLD_ONBOARD = true;
  • Missing regression test: N/A
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check full-e2e.test.ts line 41: should be const MEASURE_COLD_ONBOARD = true;.
  • Evidence: Line 41 uses process.env but test only runs in full-e2e job

PRA-10 Resolve/justify — Duplicate test fixture createTrustedBuildContext across two test files

  • Location: src/lib/onboard/sandbox-create-launch.test.ts:21
  • Category: tests
  • Problem: createTrustedBuildContext in sandbox-create-launch.test.ts and createBuildContext in sandbox-prebuild.test.ts both create a temp directory with SANDBOX_BUILD_CONTEXT_PREFIX and write a Dockerfile.
  • Impact: Maintenance burden; divergence risk if fixture logic changes.
  • Recommended action: Extract to shared fixture: test/e2e/fixtures/trusted-build-context.ts exporting createTrustedBuildContext(prefix?: string). Both test files import and use it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check both test files: they should import from shared fixture instead of defining their own.
  • Missing regression test: N/A - refactoring only
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check both test files: they should import from shared fixture instead of defining their own.
  • Evidence: sandbox-create-launch.test.ts:21 defines createTrustedBuildContext; sandbox-prebuild.test.ts:27 defines createBuildContext with identical logic

PRA-11 Resolve/justify — readOnboardTraceWindow throws generic error for missing/malformed trace

  • Location: test/e2e/fixtures/onboard-performance.ts:63
  • Category: tests
  • Problem: Error messages: 'trace artifact is missing resource_spans' or 'trace artifact must contain exactly one onboard root span' lack actionable guidance for operators.
  • Impact: Operators cannot diagnose why trace file is invalid; must inspect code to understand expected format.
  • Recommended action: Enhance error messages to include: expected trace format (OTLP with resource_spans/scope_spans/spans), expected span name (nemoclaw.onboard), expected status (OK). Example: 'Trace file missing resource_spans — expected OTLP format with resource_spans[].scope_spans[].spans[] containing span name "nemoclaw.onboard" with status OK. Installer may have failed to write trace.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check onboard-performance.ts lines 30-70: error messages should be more descriptive.
  • Missing regression test: Add test that passes malformed trace artifacts and verifies error messages contain actionable guidance.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check onboard-performance.ts lines 30-70: error messages should be more descriptive.
  • Evidence: Lines 32, 44, 50, 56, 60 throw generic errors without expected format guidance

PRA-12 Improvement — Missing errno-specific tests for EACCES, ENOSPC, EIO in trust validation

  • Location: src/lib/onboard/sandbox-prebuild.test.ts:272
  • Category: tests
  • Problem: New tests cover validation logic failures but not specific errno codes from fs operations.
  • Impact: Filesystem permission, disk full, and I/O errors during trust validation are not tested for distinct logging behavior.
  • Suggested action: Add three tests: 'logs filesystem permission error (EACCES) distinctly before falling back', 'logs disk full error (ENOSPC) distinctly before falling back', 'logs I/O error (EIO) distinctly before falling back'. Mock fs.openSync to throw each errno.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.test.ts: should have tests mocking fs.openSync to throw EACCES, ENOSPC, EIO and asserting log output.
  • Missing regression test: Add the three errno-specific tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: No tests mock fs.openSync with errno codes; existing test at line 187 mocks EMFILE only

PRA-13 Improvement — Verify all original onboard-progress-budget assertions preserved in assertColdOnboardPerformance

  • Location: test/e2e/live/full-e2e.test.ts:180
  • Category: tests
  • Problem: Original onboard-progress-budget.test.ts was deleted in this PR. Need to verify all its assertions are preserved in assertColdOnboardPerformance.
  • Impact: Potential regression in cold-path budget guarantees if assertions were dropped.
  • Suggested action: Audit original test (deleted in this PR) vs current assertColdOnboardPerformance. Document mapping in code comment. If any missing, add back.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare deleted onboard-progress-budget.test.ts (from git history) with current assertColdOnboardPerformance function in full-e2e.test.ts.
  • Missing regression test: Ensure all original budget assertions have corresponding checks in assertColdOnboardPerformance.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard-progress-budget.test.ts deleted; assertColdOnboardPerformance at line 180 replaces it

PRA-14 Improvement — Source-of-truth comment missing for resolveTrustedStagedBuildContext silent validation returns

  • Location: src/lib/onboard/sandbox-prebuild.ts:51
  • Category: architecture
  • Problem: Function is a localized workaround but lacks documentation of: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed.
  • Impact: Future maintainers may not understand the defense-in-depth rationale or removal criteria.
  • Suggested action: Add code comment at function start explaining: what invalid state is handled (untrusted build context), where that state is created (build context staging), why source cannot be fixed (defense-in-depth against TOCTOU), what regression test proves it (trust validation tests), when workaround can be removed (when OpenShell uses BuildKit for local-driver path, refactor(onboard): extract sandbox creation orchestration from the entrypoint #6258).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.ts line 51: function should have source-of-truth comment block.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function at line 51 has no source-of-truth comment block

PRA-15 Improvement — Source-of-truth comment missing for MEASURE_COLD_ONBOARD env-derived flag

  • Location: test/e2e/live/full-e2e.test.ts:41
  • Category: architecture
  • Problem: Env-derived flag is a localized workaround but lacks documentation of why it's used (shared test file, only full-e2e target measures cold).
  • Impact: Future maintainers may not understand the sharing constraint or removal criteria.
  • Suggested action: Add comment explaining why env-derived flag is used (shared test file, only full-e2e target measures cold). This satisfies PRA-2.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check full-e2e.test.ts line 41: should have explanatory comment.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 41 has no comment explaining the workaround rationale

PRA-16 Improvement — Trust validation tests could be parameterized to reduce monolith growth

  • Location: src/lib/onboard/sandbox-prebuild.test.ts:1
  • Category: tests
  • Problem: Test file grew from 166 to 385 lines (+219). The 11 new trust validation tests follow a similar pattern and could use a test matrix.
  • Impact: Monolith growth makes test file harder to maintain; parameterization would reduce duplication.
  • Suggested action: Review test structure for parameterization opportunities. The 11 new trust validation tests follow a similar pattern and could use it.each with a matrix of { name, mutateContext, expectedLog }.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check sandbox-prebuild.test.ts: look for repeated test patterns that could be consolidated with it.each.
  • Missing regression test: N/A - code quality improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 63-270: 11 tests with nearly identical structure (create context, mutate, call prebuildSandboxImageIfEligible, assert fallback)

PRA-17 Improvement — Documentation Note about gateway builder for --from could cross-reference build-context staging

  • Location: docs/reference/commands.mdx:400
  • Category: correctness
  • Problem: Note added about OpenShell gateway builder for custom --from Dockerfiles. Good user-facing change but could also clarify that local BuildKit prebuild is reserved for NemoClaw-generated contexts only and cross-reference build-context staging docs.
  • Impact: Users may not understand the trust boundary rationale or where to learn more.
  • Suggested action: Add cross-reference to the build-context staging docs for users who want to understand the trust boundary.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check commands.mdx and commands-nemohermes.mdx around line 400: Note block should be present with cross-reference.
  • Missing regression test: N/A - documentation only
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Note block at line 400 mentions gateway builder but no cross-reference

PRA-18 Improvement — Workflow removed duplicate onboard-progress-budget.test.ts run; validation confirmed

  • Location: .github/workflows/e2e.yaml:2704
  • Category: workflow
  • Problem: Workflow removed duplicate onboard-progress-budget.test.ts run from full-e2e job. Confirmed by test/e2e-release-gate-workflow.test.ts validation.
  • Impact: Reduced CI time; no functional change.
  • Suggested action: No action needed. Confirm workflow test validates the consolidation.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/e2e-release-gate-workflow.test.ts lines 25-28: should assert full-e2e job no longer runs onboard-progress-budget.test.ts.
  • Missing regression test: N/A
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: e2e.yaml diff shows removal; test/e2e-release-gate-workflow.test.ts validates

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: full-e2e, cloud-onboard, sandbox-rebuild, rebuild-hermes
Optional E2E: onboard-resume, onboard-repair, rebuild-openclaw

Dispatch hint: full-e2e,cloud-onboard,sandbox-rebuild,rebuild-hermes

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • full-e2e (high): Required because the PR changes onboarding build-context/prebuild behavior and the full-e2e workflow/performance contract. This job validates a real hosted-inference onboarding flow, requires generated contexts to use local BuildKit without gateway fallback, and exercises the first assistant turn.
  • cloud-onboard (high): Required because onboarding behavior, trace/performance plumbing, and the unified E2E workflow changed. The repository E2E guidance explicitly calls for cloud-onboard when onboard behavior, trace timing, scorecard analysis, or workflow wiring changes.
  • sandbox-rebuild (high): Required because build-context origin and prebuild routing affect sandbox lifecycle and image replacement paths used during rebuild/recreate operations.
  • rebuild-hermes (high): Required because src/lib/agent/base-image.ts and shared generated-context staging changes affect agent-specific sandbox image creation; Hermes rebuild is the existing live coverage for that agent-specific generated image path.

Optional E2E

  • onboard-resume (medium): Useful adjacent confidence because resumed onboarding can continue into the sandbox creation phase that now consumes build-context origin, although this PR does not directly change live machine resume state handling.
  • onboard-repair (high): Useful adjacent confidence for repaired resume sessions reaching sandbox creation after the build-context/prebuild routing change.
  • rebuild-openclaw (high): Useful additional coverage for OpenClaw-specific rebuild behavior after shared build context and prebuild provenance changes.

New E2E recommendations

  • custom Dockerfile gateway-build path (high): This PR intentionally routes user-supplied --from contexts away from host-side local BuildKit and back through the OpenShell gateway builder. Existing workflow jobs do not appear to provide a dedicated dispatch target that onboards a custom Dockerfile on a Docker-driver gateway and asserts both the 'Local BuildKit build skipped' notice and successful sandbox creation through the gateway builder.
    • Suggested test: Add a workflow-dispatched live E2E target such as custom-from-gateway-build that runs nemoclaw onboard --from <custom Dockerfile> on the local Docker-driver gateway, verifies the skip notice, confirms the gateway-builder image is used, and performs a basic sandbox readiness/assistant smoke check.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: full-e2e,cloud-onboard,sandbox-rebuild,rebuild-hermes

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: full-e2e, security-posture, onboard-resume, onboard-repair
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=full-e2e
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=security-posture
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • full-e2e: Focused free-standing E2E job wired for changed live test test/e2e/live/full-e2e.test.ts.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=full-e2e
  • security-posture: Focused free-standing E2E job wired for changed live test test/e2e/live/full-e2e.test.ts.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=security-posture
  • onboard-resume: Onboarding create-path changes in src/lib/onboard.ts, build-context staging, sandbox prebuild, and sandbox build-context provenance can affect resumed onboarding sessions. The onboarding resume rule requires the onboard-resume live job for these session/state-transition surfaces.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • onboard-repair: The same onboarding build-context/prebuild handoff changes can affect repair/backstop execution from persisted onboarding sessions, so onboard-repair is required alongside onboard-resume under the resume/repair policy.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair

Optional E2E targets

  • None.

Relevant changed files

  • .github/workflows/e2e.yaml
  • src/lib/agent/base-image.ts
  • src/lib/onboard.ts
  • src/lib/onboard/build-context-stage.ts
  • src/lib/onboard/sandbox-prebuild.ts
  • src/lib/sandbox/build-context.ts
  • test/e2e/README.md
  • test/e2e/fixtures/onboard-performance.ts
  • test/e2e/live/agent-turn-latency-helpers.ts
  • test/e2e/live/full-e2e.test.ts
  • test/e2e/live/onboard-progress-budget.test.ts
  • test/e2e/support/onboard-performance.test.ts

Comment thread src/lib/onboard/sandbox-prebuild.ts Fixed
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 3 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Acceptance clause
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Local Docker-driver full-e2e cold onboarding writes onboard-progress-budget.json with usedBuildKitPrebuild true, buildKitFallback false, no classic Docker build steps, and maxSilenceSecs within the 60-second budget.. Static and unit coverage is strong for the trust-boundary logic, but the changed behavior depends on Docker, OpenShell gateway creation, installer/onboard timing, and shell/system boundaries that unit tests can only mock.
  • PRA-T2 Runtime validation — Custom --from onboarding on a local Docker-driver gateway logs the Local BuildKit build skipped notice and continues through the OpenShell gateway builder without invoking the host Docker prebuild path.. Static and unit coverage is strong for the trust-boundary logic, but the changed behavior depends on Docker, OpenShell gateway creation, installer/onboard timing, and shell/system boundaries that unit tests can only mock.
  • PRA-T3 Acceptance clause — No linked issue acceptance clauses were available in the deterministic GitHub context. — add test evidence or identify existing coverage. The validation context reported linkedIssues: []; PR body references to perf(onboard): build sandbox image with BuildKit + no-silent-progress heartbeats (#6002) #6166 and [All Platforms][Sandbox][GitHub Issue #6002] Sandbox creation takes 8–10 minutes on Brev — significantly exceeds acceptable time budget for developer onboarding #6002 were treated only as untrusted scope hints. Diff/test evidence was reviewed against those hints but no literal linked-issue clauses or comments were available to map.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (4)
test/e2e/fixtures/onboard-performance.ts (1)

83-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Minor: Math.max(...) spread on unbounded array.

boundaries.slice(1).map(...) is spread into Math.max, which can throw RangeError if the events array is very large (many thousands of output lines). Unlikely in practice for this test, but reduce avoids the risk entirely.

♻️ Optional defensive refactor
-  const boundaries = [startedAtMs, ...outputTimes, finishedAtMs];
-  return Math.max(...boundaries.slice(1).map((atMs, index) => atMs - boundaries[index]));
+  const boundaries = [startedAtMs, ...outputTimes, finishedAtMs];
+  return boundaries
+    .slice(1)
+    .reduce((max, atMs, index) => Math.max(max, atMs - boundaries[index]), -Infinity);
🤖 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/e2e/fixtures/onboard-performance.ts` around lines 83 - 89, The duration
calculation in the helper that builds `outputTimes` and `boundaries` uses a
spread into `Math.max`, which can fail on very large event sets. Refactor the
return logic to compute the maximum gap with a `reduce`-style pass over
`boundaries` (or equivalent iterative logic) inside this fixture helper so it no
longer depends on spreading the whole array, while preserving the same
`startedAtMs`/`finishedAtMs` behavior.
src/lib/onboard/sandbox-prebuild.ts (3)

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

"nemoclaw-build-" prefix is a magic string duplicated across files.

This literal also appears in sandbox-prebuild.test.ts and sandbox-create-launch.test.ts (and presumably the staging code that creates these directories). Extracting it to a shared exported constant would prevent silent validation breakage if the prefix ever changes in only one location.

🤖 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 `@src/lib/onboard/sandbox-prebuild.ts` at line 58, The build-directory prefix
check in sandbox-prebuild currently hardcodes the "nemoclaw-build-" string, and
the same literal is duplicated in related sandbox tests. Extract this prefix
into a shared exported constant and update the validation in sandbox-prebuild
plus the corresponding references in sandbox-prebuild.test and
sandbox-create-launch.test (and any creator code) to use that constant so the
prefix stays consistent across the codebase.

141-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Silent fallback: no log when the staged context is rejected as untrusted.

log is declared at line 163, after both early-return checks (lines 151-157 and 158-161). This means the --from mismatch and untrusted-context paths return silently, giving no signal for why local BuildKit prebuild was skipped — unlike the later catch/non-zero-status paths, which do log a reason. This makes the failure mode this PR is specifically hardening against harder to diagnose in the field.

♻️ Move `log` earlier and add a diagnostic message
   const fromIndex = createArgs.indexOf("--from");
   const fromDockerfile = createArgs[fromIndex + 1];
+  const log = input.log ?? console.log;
   if (
     fromIndex < 0 ||
     !fromDockerfile ||
     path.resolve(fromDockerfile) !== path.resolve(input.buildCtx, "Dockerfile")
   ) {
+    log("  Local BuildKit build skipped: --from argument does not match the staged Dockerfile.");
     return { createArgs, imageRef: null };
   }
   const trustedContext = resolveTrustedStagedBuildContext(input.buildCtx);
   if (!trustedContext) {
+    log("  Local BuildKit build skipped: staged build context failed trust validation.");
     return { createArgs, imageRef: null };
   }
-
-  const log = input.log ?? console.log;
   const imageRef = sandboxLocalImageRef(input.sandboxName, input.buildId);
🤖 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 `@src/lib/onboard/sandbox-prebuild.ts` around lines 141 - 161, The early-return
paths in prebuildSandboxImageIfEligible are skipping logging, so BuildKit
prebuild can be silently disabled when the --from check fails or
resolveTrustedStagedBuildContext rejects the staged context. Move the log setup
earlier in prebuildSandboxImageIfEligible and emit a diagnostic message before
returning null imageRef for these cases, using the existing log variable and the
trusted-context / fromDockerfile checks to report why prebuild was skipped.

51-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Solid TOCTOU-aware validation; consider two hardening follow-ups.

The realpath → lstat → O_NOFOLLOW-open → dev/ino comparison correctly defends against symlink swaps between the initial checks and the open. Two residual gaps worth tracking:

  1. The validated file descriptor is closed in finally (line 82), and the actual docker build -f ... subprocess later reopens the path by string (lines 184-185). This reintroduces a narrow TOCTOU window between validation and use, since docker build can't consume a pre-opened fd. This is an inherent limitation of shelling out to the Docker CLI rather than a fixable bug here.
  2. There's no check that the resolved directory (or its ancestors) isn't group/world-writable, which matters on shared/multi-tenant hosts where other local users could pre-stage a directory before the intended context's mkdtemp output.

Neither blocks this PR, but worth a follow-up if the threat model includes shared/multi-user build hosts.

🛡️ Optional permission-bit hardening
     if (
       path.dirname(resolvedBuildCtx) !== temporaryRoot ||
       !path.basename(resolvedBuildCtx).startsWith("nemoclaw-build-") ||
       !fs.statSync(resolvedBuildCtx).isDirectory()
     ) {
       return null;
     }
+    const dirStat = fs.statSync(resolvedBuildCtx);
+    if ((dirStat.mode & 0o022) !== 0) return null; // reject group/world-writable staging dirs
🤖 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 `@src/lib/onboard/sandbox-prebuild.ts` around lines 51 - 84, The current
validation in resolveTrustedStagedBuildContext only checks realpath, basename,
and Dockerfile symlink/file state; add a permissions hardening step that rejects
build contexts whose resolved directory, or required parent chain, is
group/world-writable before returning TrustedStagedBuildContext. Keep the
existing O_NOFOLLOW and dev/ino TOCTOU checks intact, and do not try to address
the later docker build path reopen here since that is an inherent CLI
limitation.
🤖 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.

Nitpick comments:
In `@src/lib/onboard/sandbox-prebuild.ts`:
- Line 58: The build-directory prefix check in sandbox-prebuild currently
hardcodes the "nemoclaw-build-" string, and the same literal is duplicated in
related sandbox tests. Extract this prefix into a shared exported constant and
update the validation in sandbox-prebuild plus the corresponding references in
sandbox-prebuild.test and sandbox-create-launch.test (and any creator code) to
use that constant so the prefix stays consistent across the codebase.
- Around line 141-161: The early-return paths in prebuildSandboxImageIfEligible
are skipping logging, so BuildKit prebuild can be silently disabled when the
--from check fails or resolveTrustedStagedBuildContext rejects the staged
context. Move the log setup earlier in prebuildSandboxImageIfEligible and emit a
diagnostic message before returning null imageRef for these cases, using the
existing log variable and the trusted-context / fromDockerfile checks to report
why prebuild was skipped.
- Around line 51-84: The current validation in resolveTrustedStagedBuildContext
only checks realpath, basename, and Dockerfile symlink/file state; add a
permissions hardening step that rejects build contexts whose resolved directory,
or required parent chain, is group/world-writable before returning
TrustedStagedBuildContext. Keep the existing O_NOFOLLOW and dev/ino TOCTOU
checks intact, and do not try to address the later docker build path reopen here
since that is an inherent CLI limitation.

In `@test/e2e/fixtures/onboard-performance.ts`:
- Around line 83-89: The duration calculation in the helper that builds
`outputTimes` and `boundaries` uses a spread into `Math.max`, which can fail on
very large event sets. Refactor the return logic to compute the maximum gap with
a `reduce`-style pass over `boundaries` (or equivalent iterative logic) inside
this fixture helper so it no longer depends on spreading the whole array, while
preserving the same `startedAtMs`/`finishedAtMs` behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fd56bd15-7d9c-4a4c-b613-8121ac059d57

📥 Commits

Reviewing files that changed from the base of the PR and between 94fb805 and 1d215c9.

📒 Files selected for processing (11)
  • .github/workflows/e2e.yaml
  • src/lib/onboard/machine/live-flow-slice.test.ts
  • src/lib/onboard/sandbox-create-launch.test.ts
  • src/lib/onboard/sandbox-prebuild.test.ts
  • src/lib/onboard/sandbox-prebuild.ts
  • test/e2e-release-gate-workflow.test.ts
  • test/e2e/README.md
  • test/e2e/fixtures/onboard-performance.ts
  • test/e2e/live/full-e2e.test.ts
  • test/e2e/live/onboard-progress-budget.test.ts
  • test/e2e/support/onboard-performance.test.ts
💤 Files with no reviewable changes (2)
  • .github/workflows/e2e.yaml
  • test/e2e/live/onboard-progress-budget.test.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the automated review findings in c51290a.

  • Added explicit generated/custom context provenance. NemoClaw-generated contexts may use host BuildKit; user-supplied --from contexts remain on the OpenShell gateway builder, with a regression proving buildImage is not called.
  • Tightened the staged-context boundary: shared creator/validator prefix, private-directory mode check, resolved-path stat, O_NOFOLLOW descriptor open, regular-file fstat, generic policy-rejection logging, and distinct filesystem-inspection diagnostics with an EMFILE regression.
  • Removed the lstat/inode check that CodeQL flagged. O_NONBLOCK is intentionally retained so a FIFO swap cannot hang before fstat rejects it. The later Docker CLI pathname reopen is unavoidable; private mkdtemp staging and generated-context provenance bound that residual race.
  • Replaced the broad first-response extractor with payload-only OpenClaw parsing, joined split payloads, required a sentinel reply, and added user-echo rejection plus positive top-level/result payload tests.
  • Made the 180s/60s acceptance thresholds constants, contextualized trace read/parse/schema failures, made fallback evidence recognize every gateway-builder fallback, replaced the unbounded Math.max(...events) spread, and documented the custom-context builder boundary.

Two suggested changes were intentionally not applied:

  • MEASURE_COLD_ONBOARD remains target-gated because this test file also implements security-posture; forcing it on would turn that lane into a duplicate cold-budget measurement.
  • O_NONBLOCK remains for non-regular-file safety, as noted above.

Verification: signed/Verified commit; pre-commit CLI coverage + ratchet passed; pre-push CLI typecheck passed; focused 44 CLI, 11 integration, and 11 E2E-support tests passed; project overlap, source-shape, test-size/title, Biome, and live-test collection passed. npm run docs passed with only the existing unauthenticated-redirect and accent-contrast warnings.

Required live validation is running at https://github.com/NVIDIA/NemoClaw/actions/runs/28694920002 (full-e2e, cloud-onboard, security-posture).

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/e2e/live/full-e2e.test.ts (1)

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

Preserve raw trace evidence when parsing fails.

Line 130 deletes traceDirectory even when JSON.parse or readOnboardTraceWindow throws, which removes the best artifact for debugging malformed/missing trace failures. Delete only after a successful parse, or copy the raw trace into artifacts before cleanup.

Proposed adjustment
 function readAndDeleteTraceWindow(traceFile: string, traceDirectory: string): OnboardTraceWindow {
   try {
-    return readOnboardTraceWindow(JSON.parse(fs.readFileSync(traceFile, "utf8")) as unknown);
+    const traceWindow = readOnboardTraceWindow(JSON.parse(fs.readFileSync(traceFile, "utf8")) as unknown);
+    fs.rmSync(traceDirectory, { recursive: true, force: true });
+    return traceWindow;
   } catch (error) {
     throw new Error(
       `Cold onboard evidence requires a valid trace file with one successful nemoclaw.onboard root span: ${error instanceof Error ? error.message : String(error)}`,
       { cause: error },
     );
-  } finally {
-    fs.rmSync(traceDirectory, { recursive: true, force: true });
   }
 }
🤖 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/e2e/live/full-e2e.test.ts` around lines 122 - 132, The cleanup in
readAndDeleteTraceWindow is deleting traceDirectory in the finally block even
when JSON.parse or readOnboardTraceWindow fails, which destroys the raw evidence
needed for debugging. Move the fs.rmSync cleanup so it only runs after a
successful parse/return, or preserve the raw trace in an artifact before
removing it. Keep the error handling in readAndDeleteTraceWindow focused on
surfacing the parse failure while leaving malformed trace files available for
inspection.
🤖 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 `@test/e2e/live/full-e2e.test.ts`:
- Around line 216-219: The acceptance check for the sentinel first agent reply
is too loose because `toContain(EXPECTED_FIRST_REPLY)` allows extra text to
pass. Update the assertion in the `full-e2e.test.ts` sentinel check to compare
`compactAssistantReply` directly against `EXPECTED_FIRST_REPLY` so the test
enforces an exact reply, using the existing `compactAssistantReply` and
`EXPECTED_FIRST_REPLY` symbols to locate the change.

---

Nitpick comments:
In `@test/e2e/live/full-e2e.test.ts`:
- Around line 122-132: The cleanup in readAndDeleteTraceWindow is deleting
traceDirectory in the finally block even when JSON.parse or
readOnboardTraceWindow fails, which destroys the raw evidence needed for
debugging. Move the fs.rmSync cleanup so it only runs after a successful
parse/return, or preserve the raw trace in an artifact before removing it. Keep
the error handling in readAndDeleteTraceWindow focused on surfacing the parse
failure while leaving malformed trace files available for inspection.
🪄 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: 7a59ecf2-14bf-445f-8673-2e831027dcd6

📥 Commits

Reviewing files that changed from the base of the PR and between 3807c05 and c51290a.

📒 Files selected for processing (16)
  • docs/deployment/install-openclaw-plugins.mdx
  • docs/manage-sandboxes/install-plugins-hermes.mdx
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/lib/agent/base-image.ts
  • src/lib/onboard.ts
  • src/lib/onboard/build-context-stage.ts
  • src/lib/onboard/sandbox-create-launch.test.ts
  • src/lib/onboard/sandbox-prebuild.test.ts
  • src/lib/onboard/sandbox-prebuild.ts
  • src/lib/sandbox/build-context.ts
  • test/e2e/README.md
  • test/e2e/fixtures/onboard-performance.ts
  • test/e2e/live/agent-turn-latency-helpers.ts
  • test/e2e/live/full-e2e.test.ts
  • test/e2e/support/onboard-performance.test.ts
✅ Files skipped from review due to trivial changes (6)
  • docs/deployment/install-openclaw-plugins.mdx
  • docs/manage-sandboxes/install-plugins-hermes.mdx
  • src/lib/onboard/build-context-stage.ts
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • test/e2e/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/onboard/sandbox-create-launch.test.ts
  • test/e2e/fixtures/onboard-performance.ts
  • src/lib/onboard/sandbox-prebuild.test.ts

Comment thread test/e2e/live/full-e2e.test.ts
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ✅ All requested jobs passed

Run: 28694920002
Workflow ref: codex/harden-buildkit-e2e-budget
Requested targets: (default — all supported)
Requested jobs: full-e2e,cloud-onboard,security-posture
Summary: 3 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
full-e2e ✅ success
security-posture ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up CI fix: 39774f2 carries the generated/custom provenance on the staged build-context result itself and reuses one Docker-gateway decision in the onboarding entrypoint. This preserves the trust boundary while making src/lib/onboard.ts exactly net-neutral (+5/−5), so the growth guard’s concern is addressed without a formatter suppression or unrelated deletion.

Verification for this commit: 73 focused CLI tests, 5 prepared-context integration tests, CLI typecheck, test-size/source-shape/title checks, full pre-commit CLI coverage + ratchet, and pre-push typecheck all pass. One unrelated auto-pair-approval suite flake occurred on the first hook attempt; its exact 9-test file passed immediately in isolation, and the full hook retry passed.

The first required live run passed full-e2e, cloud-onboard, and both OpenClaw/Hermes security-posture jobs: https://github.com/NVIDIA/NemoClaw/actions/runs/28694920002. The same required lanes are now running against the exact final head at https://github.com/NVIDIA/NemoClaw/actions/runs/28695769833.

@cv

cv commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Final-head review disposition and runtime evidence:

  • GPT advisor runtime T1 is satisfied by final-head full-e2e: onboarding 155s, 164s through the first agent reply, 32s maximum output gap, BuildKit used, no fallback, and zero classic build steps (180s/60s hard budgets).
  • GPT runtime T2 (live custom --from) is covered at the trust-boundary seam instead of adding another costly live image build: the real stager labels custom/generated origin, tests cover both origins, and the prebuild regression proves custom origin returns before buildImage. CLI semantics then continue through the existing gateway-builder path. A second live custom build would add substantial E2E time without exercising additional branch logic.
  • The original progress-budget assertions map directly to the consolidated checks: [1/8], BuildKit use/no fallback, no classic steps, ≤60s silence, first real payload reply, successful exits, and ≤180s total.

Nemotron items reviewed but not changed:

  • The realpathSync(Dockerfile) call is a containment check, not the security atom. A symlink swap after it is rejected atomically by the subsequent open(..., O_NOFOLLOW); a regular-file swap is indistinguishable from the unavoidable later Docker CLI pathname reopen and requires the same UID inside a private 0700 context. Removing containment would not strengthen this boundary. Final-head CodeQL is green and alert docs: add security best practices #1203 is fixed.
  • O_NONBLOCK is intentional: without it, opening a FIFO can block before fstat can reject the non-regular descriptor. It is ignored for the accepted regular-file case.
  • Validation rejection and filesystem exceptions already have distinct operator messages (failed trust validation versus could not be inspected (<detail>)). Individual reason codes would expose more implementation detail without changing the fail-closed outcome; each validation class and one representative exception path are covered.
  • MEASURE_COLD_ONBOARD must remain target-gated because security-posture deliberately reuses full-e2e.test.ts; making it unconditional would duplicate the cold budget in that lane.
  • The two tiny test helpers remain local to different unit-test concerns; importing an E2E fixture into source-unit tests would couple otherwise disjoint Vitest projects.
  • Trace file read, JSON parse, and schema validation are all wrapped by the actionable Cold onboard evidence requires ... one successful nemoclaw.onboard root span error.

The first required live run passed all selected jobs, and the exact-final-head rerun is at https://github.com/NVIDIA/NemoClaw/actions/runs/28695769833.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ❌ Some jobs failed

Run: 28695769833
Workflow ref: codex/harden-buildkit-e2e-budget
Requested targets: (default — all supported)
Requested jobs: full-e2e,cloud-onboard,security-posture
Summary: 2 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
full-e2e ✅ success
security-posture ❌ failure

Failed jobs: security-posture. Check run artifacts for logs.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ✅ All requested jobs passed

Run: 28695769833
Workflow ref: codex/harden-buildkit-e2e-budget
Requested targets: (default — all supported)
Requested jobs: full-e2e,cloud-onboard,security-posture
Summary: 3 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
full-e2e ✅ success
security-posture ✅ success

@cv

cv commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Final follow-up on the exact head:

  • Standard PR automation is fully green: 42 passed, 2 intentionally skipped, with no failed or pending checks.
  • The first Hermes security-posture attempt reached the normal post-restore gateway restart and timed out with GATEWAY_HEALTH_TIMEOUT. The unchanged failed-job retry passed the complete live test in 6m08s, confirming a transient gateway-health flake: https://github.com/NVIDIA/NemoClaw/actions/runs/28695769833/attempts/2
  • The CodeRabbit raw-trace preservation nitpick is intentionally not applied. The trace is deliberately ephemeral: fixture cleanup independently removes the same temp directory, while CI uploads only the allowlisted e2e-artifacts tree, so moving deletion out of finally would not preserve CI evidence. Copying malformed raw trace data into that tree would violate the documented timing-only publication boundary; raw traces can contain attributes, events, paths, and error detail. The wrapped parse/schema error and redacted installer command artifacts remain available.
  • CodeRabbit withdrew the exact-sentinel suggestion after the payload-only performance-gate rationale.

No source changes were made after the final-head live evidence was collected.

@cv
cv merged commit 7875bd3 into main Jul 4, 2026
201 of 202 checks passed
@cv
cv deleted the codex/harden-buildkit-e2e-budget branch July 4, 2026 05:37
@cv cv mentioned this pull request Jul 4, 2026
21 tasks
cv added a commit that referenced this pull request Jul 4, 2026
## Summary

Hardens the managed-MCP work merged in #5876 so DCode rebuilds validate
every reconstructable input before crossing the destructive delete
boundary, preserve exact policy intent, and migrate legacy managed MCP
state fail-closed. Prepared rebuild artifacts and the derived MCP
runtime snapshot remain ephemeral and process-local; neither is
persisted in FSM or checkpoint state, so this does not implement #6224.

## Related Issue

Refs #5876
Refs #6195
Refs #6218

## Changes

- Revalidate DCode route, image, Dockerfile, reasoning, web-search, and
MCP inputs after preparation and before NIM stop or sandbox deletion;
restore MCP state and relock shields on failure.
- Preserve exact custom network policy replay while keeping generated
MCP rules under the MCP adapter's exclusive ownership.
- Add protocol-specific policy schema validation for REST, WebSocket,
JSON-RPC, and MCP matchers, including cross-rule `tools/call` conflict
rejection.
- Pin Deep Agents Code 0.1.30 and load only a strict, canonicalized
managed MCP projection from a process-local integrity-bound snapshot.
Sealed memfd is preferred; when OpenShell seccomp blocks it, an
anonymous `O_TMPFILE` inode is reopened read-only and bound by
descriptor, device, inode, size, kind, and SHA-256, with ambient
discovery disabled.
- Bind the canonical TypeScript secret-pattern source and flags to one
shared behavior corpus executed through the Bash and Python DCode
enforcement boundaries, including the full ECMAScript whitespace set.
- Add capability-v2 gating and legacy-v1 teardown/rollback that
preserves unrelated user configuration and fails closed on malformed,
unsafe, or drifted state.
- Add rebuild, migration, runtime-patch, schema, snapshot, and lifecycle
coverage; update the MCP, policy, security, command, and DCode
documentation.

Verification notes:

- Final DCode-adjacent run: 9 files, 187 tests passed; the focused
descriptor/projection run passed 4 files and 138 tests.
- Final review-follow-up run: 82 focused Bash/Python/TypeScript parity
and descriptor-fallback tests passed, including all 25 ECMAScript
whitespace code points under both `C` and `C.UTF-8` Bash locales.
- Full pre-squash-equivalent run: 1,068 files passed, 2 skipped; 12,149
tests passed, 35 skipped.
- CLI coverage ratchet passed with the repository include/exclude set
expressed as one Vitest glob: lines 65.24%, statements 64.45%, functions
67.06%, branches 57.21%.
- Python compile, Biome, ShellCheck, shfmt, source-shape, test-size,
repository, secret-scan, and diff checks passed. The normal push hook
passed CLI typechecking.
- Main-sync validation after merging #6265 passed: 9 CLI files/82 tests,
6 integration files/174 tests, an additional 3 preparation tests, CLI
typecheck, Biome, and diff checks. Generated-context provenance was
ported into the split preflight fixtures without restoring the obsolete
monolith.
- Exact-head CI for `9a31537785ef2d456901de622721ed215627fdec` passed:
40 checks green, all five required contexts passed, and there were 0
failures, cancellations, or pending checks. The only skips were the
expected docs-only job and two duplicate NVSkills request jobs. This
includes all five CLI shards plus the aggregate, both CodeQL languages,
both sandbox image builds, macOS, WSL, four self-hosted runtime checks,
CodeRabbit, and both review advisors.
- Exact-head live E2E for `9a31537785ef2d456901de622721ed215627fdec`
passed:
[`mcp-bridge`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844701),
[`mcp-bridge-dev`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844719),
and
[`ubuntu-repo-cloud-langchain-deepagents-code`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844639).
Stable and dev each passed OpenClaw, Hermes, and DCode 3/3;
authenticated MCP calls passed initially and after restart, credential
rotation, and rebuild, then removal denied access with no provider,
policy, tunnel, or credential residue. The dedicated DCode lane passed
Landlock 5/5, Python egress 14/14, headless inference 10/10, secret
boundary 8/8, Tavily 6/6, and TUI 4/4; BuildKit accepted the merged
generated-context handoff, and invalid-credential rebuild failure
remained pre-destructive with the original sandbox, marker, and route
recovered. Artifact inspection found one unchanged pre-existing harness
defect: two OpenShell audit-log filtering subassertions can false-pass
because awk treats `close` as reserved; runtime-output, sandbox-log,
env-file immutability, and raw-secret checks passed, and this PR does
not modify that E2E file.
- The base `test-cli` pre-commit invocation remains affected by Vitest
4.1.9 collapsing repeated `--coverage.exclude` arguments to a
zero-file/invalid summary. All other commit and push hooks passed;
targeted tests and the authoritative sharded CI coverage checks provide
the exact-head gate.
- `npm run docs` completed with 0 errors and 2 pre-existing Fern
warnings. Two documentation-writer audits confirmed the final behavior
is accurately documented.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] 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:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: independent security
and correctness reviews passed after fixes; destructive-boundary
rollback, capability migration, the process-local integrity-bound
snapshot handoff (sealed memfd preferred, anonymous `O_TMPFILE`
fallback), cross-language secret-pattern parity, policy fidelity, and
the #6224 boundary were checked.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [x] Full `npm test` passes (broad runtime changes only)
- [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)
- [x] 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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Enhanced managed MCP bridge support with managed-only configuration
snapshots for safer add/restart/rebuild/teardown.
* Network policy protocol rules now support protocol-specific matching
plus stricter `endpoint.path` validation.
* **Bug Fixes**
* Stronger fail-fast validation for MCP server names/hostnames and
endpoint details (rejected before changes are applied).
* Rebuild flows improved to preserve/replay custom policies and validate
after MCP preparation, with rollback on failure.
* **Documentation**
* Updated setup/quickstart/reference and MCP bridge/rebuild guidance for
managed MCP capability v2 behavior and stricter validation rules.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the v0.0.87 Release target label Jul 4, 2026
@ericksoa ericksoa mentioned this pull request Jul 4, 2026
21 tasks
ericksoa added a commit that referenced this pull request Jul 4, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [#6020](#6020) and
[#5876](#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [#6251](#6251) and
[#5989](#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [#6232](#6232),
[#6082](#6082),
[#6219](#6219),
[#6214](#6214),
[#6215](#6215),
[#6230](#6230), and
[#6260](#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [#6166](#6166),
[#6254](#6254),
[#6265](#6265),
[#6164](#6164), and
[#6017](#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [#6150](#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [#6234](#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [#6129](#6129),
[#5987](#5987),
[#5955](#5955), and
[#6220](#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [#5963](#5963),
[#6050](#6050),
[#6094](#6094),
[#6238](#6238),
[#5988](#5988),
[#6235](#6235),
[#6181](#6181), and
[#5986](#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [#6200](#6200),
[#6248](#6248),
[#6168](#6168),
[#6270](#6270), and
[#5649](#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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 — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [x] 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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv pushed a commit that referenced this pull request Jul 10, 2026
…nce (#6002) (#6663)

## Summary

Raise the `full-e2e` cold-onboard acceptance budget
(`ONBOARD_BUDGET_SECS`) from **180s → 205s**. This is the umbrella PR
for today's live-E2E failures on `main`; scope and evidence per job
below.

## Failure triage (main, 2026-07-10)

A full `E2E` dispatch on `main` (run
[29124128082](https://github.com/NVIDIA/NemoClaw/actions/runs/29124128082))
came back **78 passed / 5 skipped / 3 failed**. Each failure was
root-caused, not retried blindly:

| Job | Verdict | Root cause |
|-----|---------|-----------|
| `agent-turn-latency` | ✅ flake, self-cleared | Passed on first retry —
hosted-inference timing variance. |
| `full-e2e` | 🔧 **fixed here** | Consistent ~1s overshoot of a
too-tight 180s onboard budget (see below). |
| `rebuild-hermes` | ⏳ **verification pending** | Both observed failures
were infra (`operation was canceled`, `runner lost communication with
the server`) — **not** a test assertion or image-build error. Retry on
post-bump `main` in flight
([29129133666](https://github.com/NVIDIA/NemoClaw/actions/runs/29129133666)).
See "Hermes v0.18" below. |

## `full-e2e` — full analysis (no gaps)

**Symptom.** The `[1/8]-to-first-response` gate (`full-e2e.test.ts:215`)
failed **3 consecutive times**:

| Run | to-first-response | vs 180s budget |
|-----|-------------------|----------------|
|
[29124128082](https://github.com/NVIDIA/NemoClaw/actions/runs/29124128082)
| 180,829 ms | +0.8s |
|
[29125880976](https://github.com/NVIDIA/NemoClaw/actions/runs/29125880976)
| 181,550 ms | +1.5s |
|
[29127670707](https://github.com/NVIDIA/NemoClaw/actions/runs/29127670707)
| 180,602 ms | +0.6s |

**Not flake, not inference, not a code regression** — proven by the
`onboard-progress-budget.json` artifact decomposition:

| | `onboardSecs` | `totalSecs` | headroom | notes |
|---|---|---|---|---|
| Passing run
[29128496025](https://github.com/NVIDIA/NemoClaw/actions/runs/29128496025)
(`f4cd7ea9`) | **163** | 168 | +12s | BuildKit prebuild ✓, 0 classic
steps |
| Failing run
[29127670707](https://github.com/NVIDIA/NemoClaw/actions/runs/29127670707)
(`fcc121d5`) | **173** | 181 | −1s | BuildKit prebuild ✓, 0 classic
steps |

- The entire delta is in the **cold onboard/BuildKit image-build phase**
(163s → 173s, a ~10s run-to-run swing) on **identical, post-#6265
`main`** — both heads are after the Hermes v0.18 bump, so the bump is
not the cause.
- The first hosted agent turn is only **~5–8s** (`totalSecs −
onboardSecs`); inference is a rounding error.
- The 180s cap (introduced 4 days ago in #6265) left only ~7s of
headroom against a phase that varies ~10s with Docker Hub pull speed and
hosted-runner I/O — so slow-build runs tip over.

**Fix.** Raise to **205s**: covers the observed 173s worst case plus
build-variance headroom, while still catching gross onboard regressions
(a real regression blows well past 205s; `MAX_SILENCE_SECS` and
BuildKit-fallback assertions are unchanged).

## Hermes v0.18 context

Today's `main` includes `feat(hermes): upgrade to v0.18 and enable Slack
Block Kit` (#6507, 17:45Z), plus `#6624` (release-matched sandbox bases)
and `#6623` (surface cluster image build failures). Because
`rebuild-hermes` rebuilds the Hermes image, a v0.18-induced regression
*could* in principle surface as runner resource-exhaustion. **This PR
does not yet claim `rebuild-hermes` is a flake** — the in-flight retry
on post-bump `main` is the deciding evidence:
- retry **passes** → confirmed infra flake, no code change needed, this
PR ships as-is;
- retry **fails** (build error / OOM / repeat comms-loss) → v0.18 is
implicated and a fix is added to this branch before merge.

## Test evidence

- `commitlint`, `gitleaks`, test-size/shape budgets: passed
(pre-commit).
- No product-code change; single test-constant edit. Behavioral proof is
the artifact decomposition above.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Tests**
* Adjusted end-to-end onboarding timing thresholds by increasing the
“acceptance budget” to allow for typical variability in image build
times.
* Updated test parity mappings to ensure the added live coverage is
correctly linked with the corresponding faster test set.
* Added inline guidance for why timing can fluctuate between runs, and
why the previous cap needed more headroom.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This follow-up to NVIDIA#6166 validates the staged context before invoking
host BuildKit and moves the hard NVIDIA#6002 cold-path acceptance assertions
into the existing `full-e2e` lifecycle. It removes the redundant second
onboarding run while preserving a distinct merge-failing signal
alongside the advisory warm-system scorecard budget.

## Related Issue
Follow-up to NVIDIA#6166 and NVIDIA#6002.

## Changes
- Resolve and validate BuildKit contexts as private direct
`os.tmpdir()/nemoclaw-build-*` staging directories with a no-follow
regular Dockerfile, falling back to the gateway builder when validation
fails.
- Carry generated/custom provenance into the handoff so user-supplied
`--from` Dockerfiles remain on the OpenShell gateway-builder trust
boundary.
- Cover custom provenance, outside-temp, wrong-prefix,
writable-directory, symlink, non-regular-file, path-escape,
inspection-error logging, environment sanitization, and
compatibility-heartbeat behavior.
- Measure BuildKit success, fallback absence, output silence, and the
first real agent response during the job's first `full-e2e` onboarding
instead of running a second warm-cache onboarding test.
- Remove the redundant Vitest invocation from `e2e.yaml` and document
the hard 180-second cold-path contract separately from the advisory
390-second warm-system scorecard budget.

## Type of Change

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

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Reviewed every
production context stager against the new contract; explicit provenance
keeps custom Dockerfiles off host BuildKit, focused negative tests cover
each trust-boundary case, and the unavoidable post-validation Docker
pathname reopen is mitigated by private `mkdtemp` staging.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [x] 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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added optional cold-onboarding performance measurement to the live
end-to-end flow, including trace-based evidence output.
* **Bug Fixes**
* Hardened local prebuild handling for NemoClaw-generated staged build
contexts and tightened eligibility/`--from` validation behavior.
* Improved sandbox build-context metadata propagation and reused
Docker-driver gateway detection.
* **Tests**
* Expanded/refocused live end-to-end coverage and performance
assertions; added trace/probe parsing fixtures; improved test isolation
and cleanup.
* Updated e2e workflow and release-gate checks to target the correct
live Vitest test.
* **Documentation**
* Clarified `--from` build-context routing vs local BuildKit behavior on
local Docker-driver gateways.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary

Hardens the managed-MCP work merged in NVIDIA#5876 so DCode rebuilds validate
every reconstructable input before crossing the destructive delete
boundary, preserve exact policy intent, and migrate legacy managed MCP
state fail-closed. Prepared rebuild artifacts and the derived MCP
runtime snapshot remain ephemeral and process-local; neither is
persisted in FSM or checkpoint state, so this does not implement NVIDIA#6224.

## Related Issue

Refs NVIDIA#5876
Refs NVIDIA#6195
Refs NVIDIA#6218

## Changes

- Revalidate DCode route, image, Dockerfile, reasoning, web-search, and
MCP inputs after preparation and before NIM stop or sandbox deletion;
restore MCP state and relock shields on failure.
- Preserve exact custom network policy replay while keeping generated
MCP rules under the MCP adapter's exclusive ownership.
- Add protocol-specific policy schema validation for REST, WebSocket,
JSON-RPC, and MCP matchers, including cross-rule `tools/call` conflict
rejection.
- Pin Deep Agents Code 0.1.30 and load only a strict, canonicalized
managed MCP projection from a process-local integrity-bound snapshot.
Sealed memfd is preferred; when OpenShell seccomp blocks it, an
anonymous `O_TMPFILE` inode is reopened read-only and bound by
descriptor, device, inode, size, kind, and SHA-256, with ambient
discovery disabled.
- Bind the canonical TypeScript secret-pattern source and flags to one
shared behavior corpus executed through the Bash and Python DCode
enforcement boundaries, including the full ECMAScript whitespace set.
- Add capability-v2 gating and legacy-v1 teardown/rollback that
preserves unrelated user configuration and fails closed on malformed,
unsafe, or drifted state.
- Add rebuild, migration, runtime-patch, schema, snapshot, and lifecycle
coverage; update the MCP, policy, security, command, and DCode
documentation.

Verification notes:

- Final DCode-adjacent run: 9 files, 187 tests passed; the focused
descriptor/projection run passed 4 files and 138 tests.
- Final review-follow-up run: 82 focused Bash/Python/TypeScript parity
and descriptor-fallback tests passed, including all 25 ECMAScript
whitespace code points under both `C` and `C.UTF-8` Bash locales.
- Full pre-squash-equivalent run: 1,068 files passed, 2 skipped; 12,149
tests passed, 35 skipped.
- CLI coverage ratchet passed with the repository include/exclude set
expressed as one Vitest glob: lines 65.24%, statements 64.45%, functions
67.06%, branches 57.21%.
- Python compile, Biome, ShellCheck, shfmt, source-shape, test-size,
repository, secret-scan, and diff checks passed. The normal push hook
passed CLI typechecking.
- Main-sync validation after merging NVIDIA#6265 passed: 9 CLI files/82 tests,
6 integration files/174 tests, an additional 3 preparation tests, CLI
typecheck, Biome, and diff checks. Generated-context provenance was
ported into the split preflight fixtures without restoring the obsolete
monolith.
- Exact-head CI for `9a31537785ef2d456901de622721ed215627fdec` passed:
40 checks green, all five required contexts passed, and there were 0
failures, cancellations, or pending checks. The only skips were the
expected docs-only job and two duplicate NVSkills request jobs. This
includes all five CLI shards plus the aggregate, both CodeQL languages,
both sandbox image builds, macOS, WSL, four self-hosted runtime checks,
CodeRabbit, and both review advisors.
- Exact-head live E2E for `9a31537785ef2d456901de622721ed215627fdec`
passed:
[`mcp-bridge`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844701),
[`mcp-bridge-dev`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844719),
and
[`ubuntu-repo-cloud-langchain-deepagents-code`](https://github.com/NVIDIA/NemoClaw/actions/runs/28696844639).
Stable and dev each passed OpenClaw, Hermes, and DCode 3/3;
authenticated MCP calls passed initially and after restart, credential
rotation, and rebuild, then removal denied access with no provider,
policy, tunnel, or credential residue. The dedicated DCode lane passed
Landlock 5/5, Python egress 14/14, headless inference 10/10, secret
boundary 8/8, Tavily 6/6, and TUI 4/4; BuildKit accepted the merged
generated-context handoff, and invalid-credential rebuild failure
remained pre-destructive with the original sandbox, marker, and route
recovered. Artifact inspection found one unchanged pre-existing harness
defect: two OpenShell audit-log filtering subassertions can false-pass
because awk treats `close` as reserved; runtime-output, sandbox-log,
env-file immutability, and raw-secret checks passed, and this PR does
not modify that E2E file.
- The base `test-cli` pre-commit invocation remains affected by Vitest
4.1.9 collapsing repeated `--coverage.exclude` arguments to a
zero-file/invalid summary. All other commit and push hooks passed;
targeted tests and the authoritative sharded CI coverage checks provide
the exact-head gate.
- `npm run docs` completed with 0 errors and 2 pre-existing Fern
warnings. Two documentation-writer audits confirmed the final behavior
is accurately documented.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] 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:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: independent security
and correctness reviews passed after fixes; destructive-boundary
rollback, capability migration, the process-local integrity-bound
snapshot handoff (sealed memfd preferred, anonymous `O_TMPFILE`
fallback), cross-language secret-pattern parity, policy fidelity, and
the NVIDIA#6224 boundary were checked.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [x] Full `npm test` passes (broad runtime changes only)
- [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)
- [x] 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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Enhanced managed MCP bridge support with managed-only configuration
snapshots for safer add/restart/rebuild/teardown.
* Network policy protocol rules now support protocol-specific matching
plus stricter `endpoint.path` validation.
* **Bug Fixes**
* Stronger fail-fast validation for MCP server names/hostnames and
endpoint details (rejected before changes are applied).
* Rebuild flows improved to preserve/replay custom policies and validate
after MCP preparation, with rollback on failure.
* **Documentation**
* Updated setup/quickstart/reference and MCP bridge/rebuild guidance for
managed MCP capability v2 behavior and stricter validation rules.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [NVIDIA#6020](NVIDIA#6020) and
[NVIDIA#5876](NVIDIA#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [NVIDIA#6251](NVIDIA#6251) and
[NVIDIA#5989](NVIDIA#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [NVIDIA#6232](NVIDIA#6232),
[NVIDIA#6082](NVIDIA#6082),
[NVIDIA#6219](NVIDIA#6219),
[NVIDIA#6214](NVIDIA#6214),
[NVIDIA#6215](NVIDIA#6215),
[NVIDIA#6230](NVIDIA#6230), and
[NVIDIA#6260](NVIDIA#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [NVIDIA#6166](NVIDIA#6166),
[NVIDIA#6254](NVIDIA#6254),
[NVIDIA#6265](NVIDIA#6265),
[NVIDIA#6164](NVIDIA#6164), and
[NVIDIA#6017](NVIDIA#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [NVIDIA#6150](NVIDIA#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [NVIDIA#6234](NVIDIA#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [NVIDIA#6129](NVIDIA#6129),
[NVIDIA#5987](NVIDIA#5987),
[NVIDIA#5955](NVIDIA#5955), and
[NVIDIA#6220](NVIDIA#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [NVIDIA#5963](NVIDIA#5963),
[NVIDIA#6050](NVIDIA#6050),
[NVIDIA#6094](NVIDIA#6094),
[NVIDIA#6238](NVIDIA#6238),
[NVIDIA#5988](NVIDIA#5988),
[NVIDIA#6235](NVIDIA#6235),
[NVIDIA#6181](NVIDIA#6181), and
[NVIDIA#5986](NVIDIA#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [NVIDIA#6200](NVIDIA#6200),
[NVIDIA#6248](NVIDIA#6248),
[NVIDIA#6168](NVIDIA#6168),
[NVIDIA#6270](NVIDIA#6270), and
[NVIDIA#5649](NVIDIA#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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 — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [x] 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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…nce (NVIDIA#6002) (NVIDIA#6663)

## Summary

Raise the `full-e2e` cold-onboard acceptance budget
(`ONBOARD_BUDGET_SECS`) from **180s → 205s**. This is the umbrella PR
for today's live-E2E failures on `main`; scope and evidence per job
below.

## Failure triage (main, 2026-07-10)

A full `E2E` dispatch on `main` (run
[29124128082](https://github.com/NVIDIA/NemoClaw/actions/runs/29124128082))
came back **78 passed / 5 skipped / 3 failed**. Each failure was
root-caused, not retried blindly:

| Job | Verdict | Root cause |
|-----|---------|-----------|
| `agent-turn-latency` | ✅ flake, self-cleared | Passed on first retry —
hosted-inference timing variance. |
| `full-e2e` | 🔧 **fixed here** | Consistent ~1s overshoot of a
too-tight 180s onboard budget (see below). |
| `rebuild-hermes` | ⏳ **verification pending** | Both observed failures
were infra (`operation was canceled`, `runner lost communication with
the server`) — **not** a test assertion or image-build error. Retry on
post-bump `main` in flight
([29129133666](https://github.com/NVIDIA/NemoClaw/actions/runs/29129133666)).
See "Hermes v0.18" below. |

## `full-e2e` — full analysis (no gaps)

**Symptom.** The `[1/8]-to-first-response` gate (`full-e2e.test.ts:215`)
failed **3 consecutive times**:

| Run | to-first-response | vs 180s budget |
|-----|-------------------|----------------|
|
[29124128082](https://github.com/NVIDIA/NemoClaw/actions/runs/29124128082)
| 180,829 ms | +0.8s |
|
[29125880976](https://github.com/NVIDIA/NemoClaw/actions/runs/29125880976)
| 181,550 ms | +1.5s |
|
[29127670707](https://github.com/NVIDIA/NemoClaw/actions/runs/29127670707)
| 180,602 ms | +0.6s |

**Not flake, not inference, not a code regression** — proven by the
`onboard-progress-budget.json` artifact decomposition:

| | `onboardSecs` | `totalSecs` | headroom | notes |
|---|---|---|---|---|
| Passing run
[29128496025](https://github.com/NVIDIA/NemoClaw/actions/runs/29128496025)
(`f4cd7ea9`) | **163** | 168 | +12s | BuildKit prebuild ✓, 0 classic
steps |
| Failing run
[29127670707](https://github.com/NVIDIA/NemoClaw/actions/runs/29127670707)
(`fcc121d5`) | **173** | 181 | −1s | BuildKit prebuild ✓, 0 classic
steps |

- The entire delta is in the **cold onboard/BuildKit image-build phase**
(163s → 173s, a ~10s run-to-run swing) on **identical, post-NVIDIA#6265
`main`** — both heads are after the Hermes v0.18 bump, so the bump is
not the cause.
- The first hosted agent turn is only **~5–8s** (`totalSecs −
onboardSecs`); inference is a rounding error.
- The 180s cap (introduced 4 days ago in NVIDIA#6265) left only ~7s of
headroom against a phase that varies ~10s with Docker Hub pull speed and
hosted-runner I/O — so slow-build runs tip over.

**Fix.** Raise to **205s**: covers the observed 173s worst case plus
build-variance headroom, while still catching gross onboard regressions
(a real regression blows well past 205s; `MAX_SILENCE_SECS` and
BuildKit-fallback assertions are unchanged).

## Hermes v0.18 context

Today's `main` includes `feat(hermes): upgrade to v0.18 and enable Slack
Block Kit` (NVIDIA#6507, 17:45Z), plus `NVIDIA#6624` (release-matched sandbox bases)
and `NVIDIA#6623` (surface cluster image build failures). Because
`rebuild-hermes` rebuilds the Hermes image, a v0.18-induced regression
*could* in principle surface as runner resource-exhaustion. **This PR
does not yet claim `rebuild-hermes` is a flake** — the in-flight retry
on post-bump `main` is the deciding evidence:
- retry **passes** → confirmed infra flake, no code change needed, this
PR ships as-is;
- retry **fails** (build error / OOM / repeat comms-loss) → v0.18 is
implicated and a fix is added to this branch before merge.

## Test evidence

- `commitlint`, `gitleaks`, test-size/shape budgets: passed
(pre-commit).
- No product-code change; single test-constant edit. Behavioral proof is
the artifact decomposition above.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Tests**
* Adjusted end-to-end onboarding timing thresholds by increasing the
“acceptance budget” to allow for typical variability in image build
times.
* Updated test parity mappings to ensure the added live coverage is
correctly linked with the corresponding faster test set.
* Added inline guidance for why timing can fluctuate between runs, and
why the previous cap needed more headroom.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: security Security controls, permissions, secrets, or hardening v0.0.87 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants