fix(onboard): catch SandboxBaseImageResolutionError in build context staging (#8102) - #8193
Conversation
…staging (#8102) When NEMOCLAW_HERMES_SANDBOX_BASE_IMAGE_REF is set to a non-tracked or unresolvable digest, resolveSandboxBaseImage throws a typed SandboxBaseImageResolutionError. In the fresh-onboard path, this error propagated uncaught through ensureAgentBaseImage -> createAgentSandbox -> stageCreateSandboxBuildContext -> resolveSandboxBuildContext with no surrounding handler, causing Node.js to crash with a raw stack trace instead of a clean, single-line error. The rebuild preflight callers (rebuild-custom-image-preflight.ts and rebuild-managed-image-preflight.ts) already wrap stageCreateSandboxBuildContext in a broad catch, so they are unaffected. The gap is exclusively in the fresh-onboard path via prepared-dcode-rebuild.ts. Catch SandboxBaseImageResolutionError at both createAgentSandbox call sites in stageCreateSandboxBuildContext, print the error message via the existing error() callback (already human-readable: names the override ref and the reason it was rejected), and call exit(1). Non-SandboxBaseImageResolutionError exceptions are re-thrown unchanged. No buildCtx temp dir is created before the throw, so there is no cleanup gap. Fixes #8102 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughOnboarding reports ChangesSandbox resolution error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit c55ef87 in the TypeScript / code-coverage/cliThe overall coverage in commit c55ef87 in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/onboard/build-context-stage.test.ts`:
- Around line 373-400: Update the test around stageCreateSandboxBuildContext to
create a unique temporary directory with fs.mkdtempSync(), place
agent.Dockerfile inside it, and wrap the assertion and error checks in a
try/finally block. Remove the temporary directory recursively in finally so
cleanup is guaranteed even when an assertion fails, while preserving the
existing test behavior.
🪄 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: f90a211b-005c-40a3-8c20-b3e27dee612a
📒 Files selected for processing (2)
src/lib/onboard/build-context-stage.test.tssrc/lib/onboard/build-context-stage.ts
| const agentDockerfilePath = path.join(os.tmpdir(), "agent.Dockerfile"); | ||
| fs.writeFileSync(agentDockerfilePath, "FROM scratch\n"); | ||
| const errors: string[] = []; | ||
| const resolutionError = new SandboxBaseImageResolutionError( | ||
| "Hermes Agent sandbox base image override 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base@sha256:deadbeef' is outside the trusted repository 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base'.", | ||
| ); | ||
|
|
||
| expect(() => | ||
| stageCreateSandboxBuildContext({ | ||
| root: "/repo", | ||
| fromDockerfile: agentDockerfilePath, | ||
| agent: { | ||
| name: "hermes", | ||
| displayName: "Hermes Agent", | ||
| dockerfilePath: agentDockerfilePath, | ||
| } as any, | ||
| createAgentSandbox: () => { | ||
| throw resolutionError; | ||
| }, | ||
| log: vi.fn(), | ||
| error: (msg) => errors.push(msg), | ||
| exit: throwingExit, | ||
| }), | ||
| ).toThrow("exit 1"); | ||
|
|
||
| expect(errors).toEqual([` ${resolutionError.message}`]); | ||
| fs.rmSync(agentDockerfilePath, { force: true }); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use an isolated temporary directory and guaranteed cleanup.
Line 373 writes to a shared fixed path. Line 399 deletes that same path. Parallel tests or another process can have its file overwritten or removed. If an assertion fails, cleanup does not run.
Create a unique directory with fs.mkdtempSync() and remove it in finally.
Proposed fix
- const agentDockerfilePath = path.join(os.tmpdir(), "agent.Dockerfile");
- fs.writeFileSync(agentDockerfilePath, "FROM scratch\n");
- const errors: string[] = [];
- const resolutionError = new SandboxBaseImageResolutionError(
- "Hermes Agent sandbox base image override 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base@sha256:deadbeef' is outside the trusted repository 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base'.",
- );
+ const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "build-context-stage-"));
+ try {
+ const agentDockerfilePath = path.join(tempDir, "agent.Dockerfile");
+ fs.writeFileSync(agentDockerfilePath, "FROM scratch\n");
+ const errors: string[] = [];
+ const resolutionError = new SandboxBaseImageResolutionError(
+ "Hermes Agent sandbox base image override 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base@sha256:deadbeef' is outside the trusted repository 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base'.",
+ );
- expect(() => /* existing assertion */).toThrow("exit 1");
- expect(errors).toEqual([` ${resolutionError.message}`]);
- fs.rmSync(agentDockerfilePath, { force: true });
+ expect(() => /* existing assertion */).toThrow("exit 1");
+ expect(errors).toEqual([` ${resolutionError.message}`]);
+ } finally {
+ fs.rmSync(tempDir, { recursive: true, force: true });
+ }Based on learnings, temporary directories and files are resources that Vitest does not manage and must be cleaned up explicitly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const agentDockerfilePath = path.join(os.tmpdir(), "agent.Dockerfile"); | |
| fs.writeFileSync(agentDockerfilePath, "FROM scratch\n"); | |
| const errors: string[] = []; | |
| const resolutionError = new SandboxBaseImageResolutionError( | |
| "Hermes Agent sandbox base image override 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base@sha256:deadbeef' is outside the trusted repository 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base'.", | |
| ); | |
| expect(() => | |
| stageCreateSandboxBuildContext({ | |
| root: "/repo", | |
| fromDockerfile: agentDockerfilePath, | |
| agent: { | |
| name: "hermes", | |
| displayName: "Hermes Agent", | |
| dockerfilePath: agentDockerfilePath, | |
| } as any, | |
| createAgentSandbox: () => { | |
| throw resolutionError; | |
| }, | |
| log: vi.fn(), | |
| error: (msg) => errors.push(msg), | |
| exit: throwingExit, | |
| }), | |
| ).toThrow("exit 1"); | |
| expect(errors).toEqual([` ${resolutionError.message}`]); | |
| fs.rmSync(agentDockerfilePath, { force: true }); | |
| }); | |
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "build-context-stage-")); | |
| try { | |
| const agentDockerfilePath = path.join(tempDir, "agent.Dockerfile"); | |
| fs.writeFileSync(agentDockerfilePath, "FROM scratch\n"); | |
| const errors: string[] = []; | |
| const resolutionError = new SandboxBaseImageResolutionError( | |
| "Hermes Agent sandbox base image override 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base@sha256:deadbeef' is outside the trusted repository 'ghcr.io/nvidia/nemoclaw/hermes-sandbox-base'.", | |
| ); | |
| expect(() => | |
| stageCreateSandboxBuildContext({ | |
| root: "/repo", | |
| fromDockerfile: agentDockerfilePath, | |
| agent: { | |
| name: "hermes", | |
| displayName: "Hermes Agent", | |
| dockerfilePath: agentDockerfilePath, | |
| } as any, | |
| createAgentSandbox: () => { | |
| throw resolutionError; | |
| }, | |
| log: vi.fn(), | |
| error: (msg) => errors.push(msg), | |
| exit: throwingExit, | |
| }), | |
| ).toThrow("exit 1"); | |
| expect(errors).toEqual([` ${resolutionError.message}`]); | |
| } finally { | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 373-373: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(agentDockerfilePath, "FROM scratch\n")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/build-context-stage.test.ts` around lines 373 - 400, Update
the test around stageCreateSandboxBuildContext to create a unique temporary
directory with fs.mkdtempSync(), place agent.Dockerfile inside it, and wrap the
assertion and error checks in a try/finally block. Remove the temporary
directory recursively in finally so cleanup is guaranteed even when an assertion
fails, while preserving the existing test behavior.
Source: Learnings
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Exact-head blocker on d900cf4: CodeQL alert 1864 is still open at src/lib/onboard/build-context-stage.test.ts:374 (high severity, insecure temporary file). The test constructs a predictable path directly under the OS temp directory, and there is no author response or newer head yet. Please create a unique directory with fs.mkdtempSync, place agent.Dockerfile inside it, and guarantee recursive cleanup in a finally block as CodeRabbit also recommended. Then push a signed/verified commit and let CodeQL plus the advisor lanes rerun; the existing approval is explicitly conditional on green. |
CodeQL flagged js/insecure-temporary-file (high) on the build-context staging test: it created/referenced Dockerfiles at predictable, fixed names directly under os.tmpdir() (`agent.Dockerfile` / `default.Dockerfile`), which is symlink-race-prone. Route every staged-Dockerfile path through the existing `makeTmpDir` helper (fs.mkdtempSync) so each lives in an unpredictable, per-test directory that afterEach already cleans up. Covers all five sites, matching the safe pattern the file already uses elsewhere. No behavior change — these are mock fixtures and one real write in the Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> #8102 regression test.
…-override-clean-error-8102
<!-- markdownlint-disable MD041 --> ## Summary Prepares the canonical v0.0.102 release documentation from the current release-labeled scope. The change adds a dated changelog for all 38 user-facing shipping PRs and corrects the OpenClaw agent command reference for the behavior delivered by #8191. ## Changes - Add `docs/changelog/2026-08-04.mdx` with the v0.0.102 release summary, detailed behavior changes, support boundaries, security evidence links, and links to durable documentation. - Update `docs/reference/commands.mdx` to describe non-JSON OpenClaw output capture, its combined limit, marker handling, stream suppression, recovery guidance, and exit behavior. - [#8167](#8167) -> `docs/changelog/2026-08-04.mdx`: Records authenticated attachment of operator-managed llama.cpp servers. - [#8129](#8129) -> `docs/changelog/2026-08-04.mdx`: Records the Experimental managed vLLM profile for two DGX Spark systems. - [#7983](#7983) -> `docs/changelog/2026-08-04.mdx`: Records qualification of the May 2026 GB300WS factory image. - [#8207](#8207) -> `docs/changelog/2026-08-04.mdx`: Records the qualified DGX Station driver transaction. - [#8208](#8208) -> `docs/changelog/2026-08-04.mdx`: Records mode-bound Express resume state. - [#8158](#8158) -> `docs/changelog/2026-08-04.mdx`: Records recovery of host-global dual-Station runtime ownership. - [#8145](#8145) -> `docs/changelog/2026-08-04.mdx`: Records Windows-host Ollama validation from Docker Desktop's network context. - [#8190](#8190) -> `docs/changelog/2026-08-04.mdx`: Records HTTP model pulls when WSL has no local Ollama executable. - [#8195](#8195) -> `docs/changelog/2026-08-04.mdx`: Records reuse of a healthy installer-managed CLI. - [#8053](#8053) -> `docs/changelog/2026-08-04.mdx`: Records early rejection of incompatible OpenShell gateway versions. - [#8098](#8098) -> `docs/changelog/2026-08-04.mdx`: Records the bounded package-service-to-standalone gateway recovery transition. - [#8216](#8216) -> `docs/changelog/2026-08-04.mdx`: Records the final dashboard port selected during multi-sandbox onboarding. - [#8146](#8146) -> `docs/changelog/2026-08-04.mdx`: Records managed startup-state restoration for stopped sandboxes. - [#8092](#8092) -> `docs/changelog/2026-08-04.mdx`: Records gateway watchdog recovery for classified not-serving states. - [#8182](#8182) -> `docs/changelog/2026-08-04.mdx`: Records consistent managed-recovery wait configuration. - [#8040](#8040) -> `docs/changelog/2026-08-04.mdx`: Records Docker sandbox rollback authority through late validation. - [#8130](#8130) -> `docs/changelog/2026-08-04.mdx`: Records bounded Shields deadline recovery and durable containment. - [#8086](#8086) -> `docs/changelog/2026-08-04.mdx`: Records repair of narrowly validated permission-only configuration drift. - [#8122](#8122) -> `docs/changelog/2026-08-04.mdx`: Records prompt failure and guidance for corrupt transition locks. - [#8124](#8124) -> `docs/changelog/2026-08-04.mdx`: Records policy restoration flags, previews, and target revalidation. - [#7886](#7886) -> `docs/changelog/2026-08-04.mdx`: Records explicit destruction after pre-delete Shields hardening failures while preserving recovery authority. - [#7901](#7901) -> `docs/changelog/2026-08-04.mdx`: Records multi-port uninstall behavior and shared-resource preservation. - [#7984](#7984) -> `docs/changelog/2026-08-04.mdx`: Records one classified transient remote MCP startup retry. - [#7954](#7954) -> `docs/changelog/2026-08-04.mdx`: Records bounded hosted-inference probe replies. - [#7574](#7574) -> `docs/changelog/2026-08-04.mdx`: Records preservation of validated reasoning capabilities through onboarding. - [#8089](#8089) -> `docs/changelog/2026-08-04.mdx`: Records proxy routing for Hermes WhatsApp pairing and media traffic. - [#7682](#7682) -> `docs/changelog/2026-08-04.mdx`: Records native Hermes session deletion and identifier validation. - [#8150](#8150) -> `docs/changelog/2026-08-04.mdx`: Records corporate CA trust for LangChain Deep Agents Code image builds. - [#8156](#8156) -> `docs/changelog/2026-08-04.mdx`: Records reviewed managed runtime dependency remediation. - [#8180](#8180) -> `docs/changelog/2026-08-04.mdx`: Records reviewed MCP discovery runtime dependency updates. - [#8196](#8196) -> `docs/changelog/2026-08-04.mdx`: Records private npm dependency remediation across managed images. - [#8203](#8203) -> `docs/changelog/2026-08-04.mdx`: Records reviewed Hermes and LangChain Deep Agents Code Python dependency updates. - [#8125](#8125) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for invalid enumerated CLI values. - [#8193](#8193) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for unresolved sandbox base images. - [#8118](#8118) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for changed gateway authority. - [#8191](#8191) -> `docs/changelog/2026-08-04.mdx`, `docs/reference/commands.mdx`: Records output capture, marker handling, recovery guidance, and exit behavior for non-JSON OpenClaw agent commands. - [#8187](#8187) -> `docs/changelog/2026-08-04.mdx`: Records the aligned interactive-installation start across supported agents. - [#8153](#8153) -> `docs/changelog/2026-08-04.mdx`: Records current product capabilities and support boundaries. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] 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: - [x] Tests not applicable — justification: This documentation-only release preparation does not change executable behavior. Existing changelog and published-route tests pass. - [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: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Independently reviewed `docs/changelog/2026-08-04.mdx` and `docs/reference/commands.mdx` at commit `b89913780`. All 38 user-facing v0.0.102 PRs are represented, #8191 behavior matches the implementation, and the writing rules, documentation style, controlled terminology, route structure, and skip policy pass review. Targeted tests pass 36/36 and the documentation build completes with 0 errors. - Agent: Codex Desktop independent documentation writer <!-- docs-review-head-sha: b899137 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project integration test/changelog-docs.test.ts test/check-docs-published-routes.test.ts` passed 36/36. - [x] Applicable broad gate passed — not applicable to documentation-only changes; `npm run docs` completed successfully with 0 errors. - [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) — completed with 0 errors and 2 existing Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [x] New doc pages include SPDX header and frontmatter (new pages only) — the native dated changelog uses the required parser-safe MDX SPDX comment and intentionally has no frontmatter. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added release notes for v0.0.102, covering authentication, hardware setup, WSL, installer recovery, sandbox resilience, policy management, inference reliability, CLI improvements, and unified quickstarts. - Updated command documentation to explain how non-JSON agent output is collected, replayed, and reported. - **Bug Fixes** - Improved command-output recovery guidance when output exceeds limits or contains unsupported fallback markers. - Preserved accurate command exit-status reporting after output processing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
When
NEMOCLAW_HERMES_SANDBOX_BASE_IMAGE_REFis set to a non-tracked or unresolvable digest,resolveSandboxBaseImagethrows a typedSandboxBaseImageResolutionError. In the fresh-onboard path this error propagated uncaught throughensureAgentBaseImage→createAgentSandbox→stageCreateSandboxBuildContext→resolveSandboxBuildContextwith no surrounding handler, causing Node.js to crash with a raw stack trace instead of a clean, single-line error.Closes #8102.
Reproduction
Environment
a5562015, Node v22.22.2Observed on
main(before fix)Observed on
fix/...(after fix)Analysis
stageCreateSandboxBuildContextinsrc/lib/onboard/build-context-stage.tscallsinput.createAgentSandbox(input.agent)at two points — theelse if (input.agent)branch and theisSameFile(fromResolved, agentDockerfile)branch — with no try/catch. The rebuild preflight callers (rebuild-custom-image-preflight.ts,rebuild-managed-image-preflight.ts) already wrapstageCreateSandboxBuildContextin a broadcatch (err)that returns{ ok: false, detail: err.message }, so they are unaffected. The gap is exclusively in the fresh-onboard path, whereprepared-dcode-rebuild.ts:resolveSandboxBuildContextcallsstageCreateSandboxBuildContextandonboard.tscallsresolveSandboxBuildContext— neither has a surrounding handler forSandboxBaseImageResolutionError.Because the throw originates inside
ensureAgentBaseImage, beforecreateAgentSandboxever callsfs.mkdtempSync, no temporary build-context directory is created, so there is no cleanup gap on the failure path.Fix
Import
SandboxBaseImageResolutionErrorinbuild-context-stage.tsand wrap bothinput.createAgentSandbox(input.agent)call sites with a try/catch. On aSandboxBaseImageResolutionError, call the existingerror()callback (the message already names the override ref and the specific rejection reason) andexit(1). Non-SandboxBaseImageResolutionErrorexceptions are re-thrown unchanged.Three tests are added to the existing
build-context-stage.test.ts:else if (input.agent)path → clean exit (the reporter's exact scenario).--from=<agent Dockerfile>path → clean exit.Changes
src/lib/onboard/build-context-stage.ts: importSandboxBaseImageResolutionError; wrap bothcreateAgentSandboxcall sitessrc/lib/onboard/build-context-stage.test.ts: add 3 tests for#8102Type of Change
Verification
npx prek run --all-filespassesnpm testpasses (touched files at minimum)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit