fix(sandbox): report why a sandbox config read failed - #9198
Conversation
readSandboxConfig raised a diagnostic carrying the reason OpenShell gave for a failed `sandbox exec -- cat`, but raised it inside a try whose catch discarded every error. The reason never reached the caller, so every failed read reported the generic "Is the sandbox running?" text — wrong whenever the sandbox was running and the exec failed for another reason. Let that diagnostic reach the caller. When OpenShell reports no reason, the stopped-sandbox text stays as the best remaining guess. The reason comes from stderr and the spawn error only. `result.output` is stdout-first, and stdout here is the config the read printed, so using it would put config contents into a CLI error. Refs: NVIDIA#9104 Signed-off-by: harjoth <harjoth.khara@gmail.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 (2)
📝 WalkthroughWalkthroughSandbox configuration read failures now report safe error details without exposing stdout contents. Explicit ChangesSandbox config diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change improves failed sandbox-config diagnostics without changing successful behavior or exit-code handling. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Maintainer gate status: blocked by required CI at latest PR commit 9b63efe. The behavior-preserving branch refresh and authorized full rerun reproduced the same shard 12 failure in No contributor change is requested from this result. The existing approval does not satisfy the merge gate while Evidence: https://github.com/NVIDIA/NemoClaw/actions/runs/31871380586/job/94980992068 |
cv
left a comment
There was a problem hiding this comment.
Security review: PASS
Reviewed commit 9eb62a99bf95cfbc7da420a4d7d17c3d54fdf3ba across secrets, input handling, authorization, dependencies, error handling, cryptography, configuration, security tests, and system security. The change propagates process diagnostics without including configuration output. Regression coverage confirms that partial configuration output does not enter the error. No security findings.
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry required before planning the v0.0.110 release. The entry summarizes user-facing changes merged since v0.0.109 and links each change to its published documentation route and source PR. ## Changes - Add `docs/changelog/2026-08-17.mdx` with the exact `## v0.0.110` release heading. - Cover managed local inference, endpoint validation, onboarding and recovery, explicit experimental Portable OpenClaw, messaging and policy cleanup, backup and security hardening, and release qualification. - Preserve the documentation skip list and the current supported-agent matrix; test-only refactors, dormant activation work, and Pi-only changes are intentionally excluded. ### Source-to-doc mapping - #8711 -> `docs/changelog/2026-08-17.mdx`: Add the Muse Glimmer llama.cpp profile. - #9099 -> `docs/changelog/2026-08-17.mdx`: Update the Muse Glimmer vLLM runtime. - #9319 -> `docs/changelog/2026-08-17.mdx`: Select the provider required by an explicit serving profile. - #9311 -> `docs/changelog/2026-08-17.mdx`: Report probe-image pull failures separately. - #9345 -> `docs/changelog/2026-08-17.mdx`: Reuse mirrored Windows Ollama. - #9284 -> `docs/changelog/2026-08-17.mdx`: Complete the required Ollama upgrade. - #9320 -> `docs/changelog/2026-08-17.mdx`: Reject unsafe custom endpoint URLs before mutation. - #9119 -> `docs/changelog/2026-08-17.mdx`: Reject unsupported custom endpoint URL components. - #9236 -> `docs/changelog/2026-08-17.mdx`: Require native Anthropic tool-use evidence. - #9347 -> `docs/changelog/2026-08-17.mdx`: Distinguish Gemini runtime 404 diagnostics. - #9307 -> `docs/changelog/2026-08-17.mdx`: Preserve the recorded API family when only the model drifts. - #9233 -> `docs/changelog/2026-08-17.mdx`: Fail incomplete Hermes route synchronization. - #9185 -> `docs/changelog/2026-08-17.mdx`: Serialize Model Router lifecycle work across gateways. - #9112 -> `docs/changelog/2026-08-17.mdx`: Stop Model Router after the last routed sandbox is destroyed. - #9229 -> `docs/changelog/2026-08-17.mdx`: Verify fresh sandbox execution readiness. - #9299 -> `docs/changelog/2026-08-17.mdx`: Verify a separate agent API host forward before reporting ready. - #9318 -> `docs/changelog/2026-08-17.mdx`: Honor explicit sandbox recreation. - #9325 -> `docs/changelog/2026-08-17.mdx`: Measure readiness reuse windows from collection completion. - #9352 -> `docs/changelog/2026-08-17.mdx`: Guide users away from the deprecated global start command. - #9370 -> `docs/changelog/2026-08-17.mdx`: Persist managed OpenClaw agent identity. - #9366 -> `docs/changelog/2026-08-17.mdx`: Pass messaging dependencies during reused onboarding. - #9321 -> `docs/changelog/2026-08-17.mdx`: Detect proxied connect sessions. - #9285 -> `docs/changelog/2026-08-17.mdx`: Run probe-only recovery when absent authority cannot be created. - #9282 -> `docs/changelog/2026-08-17.mdx`: Complete probe-only recovery without platform evidence. - #8920 -> `docs/changelog/2026-08-17.mdx`: Preserve legacy gateway identity. - #9198 -> `docs/changelog/2026-08-17.mdx`: Report sandbox config-read failures. - #9201 -> `docs/changelog/2026-08-17.mdx`: Remove only the exact Docker orphan on destroy. - #9176 -> `docs/changelog/2026-08-17.mdx`: Use rootless Podman for Portable lifecycle operations. - #9197 -> `docs/changelog/2026-08-17.mdx`: Preflight Portable CPU delegation. - #9289 -> `docs/changelog/2026-08-17.mdx`: Narrow Portable policy defaults. - #9270 -> `docs/changelog/2026-08-17.mdx`: Preserve Portable model intent. - #9339 -> `docs/changelog/2026-08-17.mdx`: Reconcile timed-out Portable stop state. - #9209 -> `docs/changelog/2026-08-17.mdx`: Clean receipt-owned Portable Podman resources. - #9186 -> `docs/changelog/2026-08-17.mdx`: Separate Podman activation readiness. - #9376 -> `docs/changelog/2026-08-17.mdx`: Settle Portable OpenClaw pairing before readiness. - #9296 -> `docs/changelog/2026-08-17.mdx`: Retire messaging channel presets the host no longer configures. - #9327 -> `docs/changelog/2026-08-17.mdx`: Drop retired channels from reused messaging selections. - #9306 -> `docs/changelog/2026-08-17.mdx`: Remove gateway-enforced presets without a local record. - #9248 -> `docs/changelog/2026-08-17.mdx`: Activate Google Chat pairing approval. - #9374 -> `docs/changelog/2026-08-17.mdx`: Accept schema-owned messaging plan fields. - #9317 -> `docs/changelog/2026-08-17.mdx`: Accept safe hard-linked package files during backup. - #9288 -> `docs/changelog/2026-08-17.mdx`: Remove managed CLI shims with destroyed user data. - #9239 -> `docs/changelog/2026-08-17.mdx`: Read voice credentials from fixed descriptors. - #9269 -> `docs/changelog/2026-08-17.mdx`: Accept bounded native OpenClaw device modes. - #9371 -> `docs/changelog/2026-08-17.mdx`: Isolate OpenClaw startup-guard output. - #9351 -> `docs/changelog/2026-08-17.mdx`: Restore staging Launchable validation. - #9350 -> `docs/changelog/2026-08-17.mdx`: Retry transient collaborator-permission reads. - #9353 -> `docs/changelog/2026-08-17.mdx`: Retry transient exact-artifact downloads. - #9226 -> `docs/changelog/2026-08-17.mdx`: Add bounded Brev readiness diagnostics. - #9237 -> `docs/changelog/2026-08-17.mdx`: Report same-commit E2E reliability. - #9232 -> `docs/changelog/2026-08-17.mdx`: Execute native-runtime qualification. - #9275 -> `docs/changelog/2026-08-17.mdx`: Define E2E selection and retry guidance. - #9234 -> `docs/changelog/2026-08-17.mdx`: Move documentation review after merge. - #9365 -> `docs/changelog/2026-08-17.mdx`: Mount documentation reviewer inputs before startup. ## 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 - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated release-entry contract. - [ ] Tests 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: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; documentation-only change. - 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 test/changelog-docs.test.ts` (7 passed) - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to one prose-only changelog page; `npm run docs` passed the repository's strict documentation gate. - [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) — passed with 0 errors and the 2 existing Fern warnings. - [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) — the SPDX header is present; dated changelog pages intentionally do not use frontmatter. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.110. * Documented experimental managed llama.cpp and Portable OpenClaw profiles. * Covered inference validation, onboarding and recovery improvements, rootless lifecycle handling, messaging and policy updates, backups, credential handling, filesystem protections, and release qualification updates. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
nemoclaw inference settold a reporter "Is the sandbox running?" about a sandbox that was Phase Ready. It says that about every failed config read, because the diagnostic naming the real reason was thrown inside atrywhosecatchdiscarded it. This lets that reason reach the user.Before:
Cannot read openclaw config (/sandbox/.openclaw/openclaw.json). / Is the sandbox running? / Start the sandbox and retry.After:
Cannot read openclaw config (/sandbox/.openclaw/openclaw.json): exec session setup failed: container not readyRelated Issue
Refs: #9104
Changes
src/lib/sandbox/config.ts—readSandboxConfigrunsopenshell sandbox exec -- cat <configPath>. On a failed exec it built a diagnostic carrying OpenShell's reason and threw it throughconfigFail, but the enclosingcatch { raw = ""; }swallowed it unconditionally, so execution always fell through to the generic stopped-sandbox message below. That made the detailed branch dead code.SandboxConfigErrornow escapes the catch; unexpected errors still become an empty read.result.output, which is stdout-first (captureOutput,adapters/openshell/client.ts:161) — and stdout here is the agent config thecatprinted. Surfacing a diagnostic that could carry it would put config contents, credentials included, into a CLI error.test/sandbox-config-read-failure-diagnostic.test.ts— new. Drives the real read path (realspawnSync, realcaptureOpenshellCommand) against a stub OpenShell binary selected withNEMOCLAW_OPENSHELL_BIN.No new abstraction, configuration, fallback, or compatibility path.
On the exit code, which is the issue's headline
I could not reproduce exit 0, and this PR does not claim to fix it. Driving the real binary with a stub OpenShell whose
sandbox exec -- catfails, both grammars exit 1 on currentmain:The contract holds in source too:
configFailthrowsSandboxConfigErrorwith exit code 1,readInSandboxConfigOrFailcarries it intoInferenceSetError, and bothinference:setandsandbox:inference:setcallfailWithLines(…, error.exitCode). The three siblingsandbox configcommands do the same. No commit has touched these files sincev0.0.108, the reported version.What I did reproduce is the issue's other stated expectation — "the message should say so accurately" — so that is what this fixes. If the reporter can still see
exit=0, the wrapper and shell around the invocation are worth capturing, since the CLI itself returns 1 here.Type of Change
Quality Gates
grep -rn "Is the sandbox running\|Cannot read" docs/ fern/returns only one unrelated line (docs/reference/commands.mdx:2558, prose about policy presets). No flag, command surface, or documented output changes.result.outputchange above is the part worth a second pair of eyes.Documentation Writer Review
no-docs-neededsrc/lib/sandbox/config.ts,test/sandbox-config-read-failure-diagnostic.test.ts, linked issue [Ubuntu 24.04][Inference] inference set exits 0 after failing to read the sandbox openclaw config #9104, and the owning inference and sandbox-configuration guides. The merge with current main preserves both reviewed PR-owned blobs. The change replaces an undocumented generic fallback with OpenShell's available stderr or spawn-error diagnostic, preserves the generic fallback when no diagnostic exists, and does not change commands, flags, configuration, defaults, exit behavior, or a documented recovery procedure. No documentation page contains the old fallback or the new transport-specific example, and the diagnostic does not include partial stdout or sandbox configuration content.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; this is a bounded change to one function, not a runtime or test-harness change.npm run docsbuilds without warnings (doc changes only)Red, then green
The new test against unmodified
origin/mainsources (git checkout origin/main -- src/lib/sandbox/config.ts):With the fix:
Real behavior, through the real CLI
Stub OpenShell on
PATH, isolatedHOME, one registered sandbox;sandbox exec -- catfails withexec session setup failed: container not ready. Only one OpenShell call is made — the read is the pre-flight gate (#6997), so nothing is mutated either way.The "Start the sandbox and retry." hint correctly disappears:
readInSandboxConfigOrFailappends it only to a message asking whether the sandbox is running.Blast radius
readSandboxConfigfeedsconfig get,config set,config rotate-token,inference set, and the tunnel allowed-origins reader. All of them already routeSandboxConfigErrorto a non-zero exit, so each gets the same better diagnostic and nothing else changes. Found by grepping the symbol and the changed literals repo-wide, not by picking adjacent directories.Limits
Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests