fix(rebuild): isolate ambient onboard env from sandbox recreate (#5735) - #5787
Merged
Conversation
The installer runs `upgrade-sandboxes --auto` right after onboarding. When the operator exported a different agent/provider for that onboard (e.g. NEMOCLAW_AGENT=langchain-deepagents-code, NEMOCLAW_PROVIDER_KEY=sk-...), those ambient values leaked into each existing sandbox's rebuild: the old OpenClaw sandbox was backed up, deleted, then recreated as the wrong agent (Deep Agents) with an invalid key — destroying the sandbox before a failed/mismatched recreate, while the installer still printed a clean completion banner. A rebuild must recreate a sandbox from its own recorded registry/session config, never from an unrelated onboard's ambient env. - Isolate NEMOCLAW_AGENT/PROVIDER/PROVIDER_KEY/ENDPOINT_URL/MODEL for the duration of the `onboard --resume` recreate so the registry-pinned session (and the already-registered gateway provider) wins. Restored in `finally`. - Surface the agent mismatch before any destructive backup/delete. - Pin credentialEnv from the target registry provider, and repin the endpoint from the provider's canonical config when the loaded session belongs to a different sandbox. - Fail closed *before* delete when a non-matching session targets a custom/OpenAI-compatible provider whose base URL is only in its own session (nvidia-router and known remotes remain registry/blueprint-derivable, so they are not aborted). - Installer: a failed `upgrade-sandboxes --auto` no longer prints a clean "Installation complete" banner; it reports completion with warnings and recovery guidance. Proven end-to-end through the real worktree CLI against a live gateway: with the contaminating Deep Agents env set, pre-fix `upgrade-sandboxes --auto` deleted an OpenClaw sandbox and recreated it as Deep Agents (incomplete state restore); post-fix the same command rebuilt it as OpenClaw and it stayed Ready. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
6 tasks
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ambient recreate environment isolation and rebuild resume preflight for sandbox rebuilds, updates rebuild tests for mismatched and routed sessions, and changes installer completion messaging to flag failed auto-upgrades with recovery guidance. ChangesSandbox rebuild isolation and installer warning banner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/actions/sandbox/rebuild-flow.test.ts`:
- Around line 498-535: The test cleanup is deleting NEMOCLAW_AGENT and
NEMOCLAW_PROVIDER_KEY unconditionally, which can wipe out values that were
already present in the worker. In rebuild-flow.test.ts, update the affected test
blocks around createRebuildFlowHarness and rebuildSandbox to snapshot the
original process.env values before overriding them, then restore the previous
values in finally (or delete only if they were originally unset). Apply the same
save/restore pattern anywhere the test mutates COMPATIBLE_API_KEY and
NVIDIA_INFERENCE_API_KEY as well.
🪄 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: 330cbc6f-b694-4a7e-acc5-50cfb2e41214
📒 Files selected for processing (6)
scripts/install.shsrc/lib/actions/sandbox/rebuild-env-isolation.test.tssrc/lib/actions/sandbox/rebuild-env-isolation.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild.tstest/install-upgrade-sandboxes-severity.test.ts
…te (#5735) CodeRabbit: the rebuild-flow tests overwrote NEMOCLAW_AGENT/PROVIDER_KEY/ COMPATIBLE_API_KEY/NVIDIA_INFERENCE_API_KEY and unconditionally deleted them in `finally`, which would wipe a value a worker already had set. Snapshot the prior values and reinstate them exactly (unset stays unset) via a branchless `snapshotEnv` helper — branchless so it also keeps the changed-test-file if-statement guardrail green. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…5735) Address the PR Review Advisor required/justify items for the auto-upgrade rebuild path: - Installer (PRA-5): a failed post-onboard `upgrade-sandboxes --auto` now propagates a fatal/non-zero installer result. Extract `finalize_install()` which prints the completion summary + recovery guidance and then exits via the fatal `error` path when `_UPGRADE_SANDBOXES_FAILED=true`, so automation and operators cannot treat a destructive upgrade failure as success. - Rebuild (PRA-6/PRA-9): consolidate the recreate preconditions into a single pre-delete trust boundary, `prepareRebuildResumeConfig()`. It assesses ambient onboard-selection env, fails closed for an undeterminable custom endpoint, and resolves the exact provider/model/credential/endpoint BEFORE any destructive backup/delete; the post-delete session rewrite merely applies that result. OpenShell recreates with the same sandbox name (no side-by-side replacement), so this validate-before-delete + preserved-backup recovery is the achievable atomicity guarantee — documented in the helper. - Security (PRA-7): sanitize the untrusted `NEMOCLAW_AGENT` value before printing the ignored-ambient-agent message via `sanitizeEnvValueForDisplay()` (strip control/ANSI, single line, length-capped) so it cannot inject terminal output. Tests: installer fatal-exit cases (PRA-T1), recreate-failure backup-recovery contract (PRA-T2), and env-value sanitization (PRA-T3). Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
PR Review Advisor response (#5735)Latest push addresses the advisor worklist. Summary of how each item is resolved or justified. Required — fixed in codePRA-5 / PRA-T1 / PRA-T7 — failed auto-upgrade now propagates a fatal installer exit. PRA-6 / PRA-T2 / PRA-T5 / PRA-T6 — pre-delete recreate-config validation, with the atomicity limit documented.
The provider/model/credential/endpoint the recreate will use is now resolved and validated before delete and merely applied to the session afterward (previously resolved post-delete). The only residual failure window is a transient runtime fault inside PRA-7 / PRA-T3 — sanitize the printed In-scope improvement — fixedPRA-9 — extracted Resolve / justify — source-of-truth framingPRA-1 (installer severity), PRA-2 (delete-then-recreate), PRA-3 (ambient env/global session), PRA-4 (custom-endpoint preflight), PRA-8 (contamination contract):
PRA-T4 — runtime/CLI validation that PRA-T8 — #2273 atomicity: addressed as far as the same-name OpenShell constraint allows (precondition validation before delete + backup-based recovery for the residual transient window), documented above. CodeRabbitThe env-restore thread is addressed (branchless Signed-off-by: Yimo Jiang yimoj@nvidia.com |
5 tasks
wscurran
added
area: onboarding
Contributor
…on contract (#5735) Address the round-2 PR Review Advisor items for the auto-upgrade rebuild path. - PRA-5 (improvement): extract the pre-delete recreate trust boundary — SESSION_ONLY_ENDPOINT_PROVIDER_NAMES, getRebuildEndpointFromRegistry, RebuildResumeConfig, prepareRebuildResumeConfig, isLocalInferenceProvider, getRebuildCredentialEnvFromRegistry — into a focused `rebuild-resume-config.ts` module with direct unit tests. Call order before backup/delete and env isolation around onboard() are unchanged; rebuild.ts shrinks accordingly. - PRA-4: document the source boundary, sync requirement, and removal condition for the ambient-env isolation list in `rebuild-env-isolation.ts`; add a contract test pinning AMBIENT_RECREATE_ENV_VARS and a matching-session custom-endpoint regression with hostile ambient NEMOCLAW_ENDPOINT_URL/ PROVIDER/MODEL (session config used, ambient absent during recreate, caller env restored). Architecture/atomicity items (delete-then-recreate, ambient/session contamination) are justified in the PR discussion; the residual health-before-delete window is constrained by OpenShell same-name recreate and tracked in #5801. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
PR Review Advisor response — round 2 (#5735)Thanks — the latest advisor run resolved 5 prior items. This push addresses the remaining ones. PRA-5 (improvement) — doneExtracted PRA-4 (resolve/justify) — documented + tested
PRA-1, PRA-2 (resolve/justify) — re-affirmedSame source-of-truth framing as PRA-4 / the round-1 comment: invalid state = ambient env / global session steering recreate (and delete-then-recreate ordering); source boundary = PRA-3 (Required, atomicity) — achievable validation done; maintainer override for the residual windowThe destructive order (delete-before-recreate) is an OpenShell architectural constraint: recreate reuses the same sandbox name, so a replacement cannot be built and health-checked while the original still exists — true side-by-side health-before-delete is not possible in this PR without a new OpenShell capability. What this PR does instead, before any delete:
This catches the determinable failure classes before destruction; the residual window is a transient runtime fault inside Test follow-ups
Signed-off-by: Yimo Jiang yimoj@nvidia.com |
cv
approved these changes
Jun 25, 2026
This was referenced Jun 26, 2026
20 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…IA#5735) (NVIDIA#5787) ## Summary The installer runs `upgrade-sandboxes --auto` immediately after onboarding. When the operator had exported a different agent/provider for that onboard (e.g. `NEMOCLAW_AGENT=langchain-deepagents-code` + `NEMOCLAW_PROVIDER_KEY=sk-...`), those ambient values leaked into every existing sandbox's rebuild: the old OpenClaw sandbox was backed up, **deleted, then recreated as the wrong agent (Deep Agents) with an invalid key** — destroying the sandbox before a failed/mismatched recreate, while the installer still printed a clean "Installation complete" banner. A rebuild must recreate a sandbox from its own recorded registry/session config, never from an unrelated onboard's ambient env. ## Related Issue Fixes NVIDIA#5735 ## Changes - **Isolate ambient onboard-selection env during recreate** — `NEMOCLAW_AGENT`, `NEMOCLAW_PROVIDER`, `NEMOCLAW_PROVIDER_KEY`, `NEMOCLAW_ENDPOINT_URL`, `NEMOCLAW_MODEL` are removed for the duration of the `onboard --resume` recreate (restored in `finally`), so the registry-pinned session and the already-registered gateway provider win. New `rebuild-env-isolation.ts` helper. - **Surface the agent mismatch before any destructive backup/delete.** - **Pin authoritative resume fields** — `credentialEnv` from the target registry provider; repin the endpoint from the provider's canonical config when the loaded session belongs to a different sandbox. - **Fail closed before delete** when a non-matching session targets a custom OpenAI/Anthropic-compatible provider whose base URL exists only in its own session (`nvidia-router` and known remotes stay registry/blueprint-derivable and are not aborted). - **Installer severity** — a failed `upgrade-sandboxes --auto` no longer prints a clean completion banner; it reports "completed with warnings" plus the affected sandbox / backup path / recovery commands. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [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 - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed **Real-CLI E2E (live gateway, isolated dedicated-port gateway + temp registry):** created a real OpenClaw `nvidia-prod` sandbox, marked it stale, exported the reporter's `NEMOCLAW_AGENT=langchain-deepagents-code` + `NEMOCLAW_PROVIDER_KEY=sk-...`, and ran `./bin/nemoclaw.js upgrade-sandboxes --auto`. - **Pre-fix:** the OpenClaw sandbox was deleted and recreated as **Deep Agents** (registry `agent=langchain-deepagents-code`) with incomplete state restore — data loss. - **Post-fix:** the same command logged *"Ignoring ambient NEMOCLAW_AGENT=… — rebuilding as its recorded agent 'openclaw'"* and rebuilt it as **OpenClaw**, staying `Ready`. Targeted unit tests: `rebuild-env-isolation.test.ts`, `rebuild-flow.test.ts` (env isolation + custom-endpoint pre-delete abort + nvidia-router/known-remote proceed), `install-upgrade-sandboxes-severity.test.ts`, plus `repro-2201.test.ts` and the sandbox/onboard suites. `tsc -p tsconfig.cli.json` clean; reviewed with `codex review --uncommitted` (findings addressed). --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added helpers to sanitize and isolate ambient recreate-selection environment values during sandbox rebuild/resume. * Rebuild now derives a consistent `onboard --resume` configuration, with conditional endpoint pinning and safer resume/session updates. * Installer now reports “completed with warnings” when automatic sandbox upgrades fail. * **Bug Fixes** * Prevented ambient onboarding variables from steering recreate/resume behavior. * Installer now returns a non-zero status when the auto-upgrade did not finish. * **Tests** * Added/extended Vitest coverage for env isolation, rebuild edge cases, and installer banner/exit-code severity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- Supersedes NVIDIA#5770 (reopened from NVIDIA/NemoClaw branch so trusted advisor workflows can run). --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
10 tasks
cv
added a commit
that referenced
this pull request
Jul 15, 2026
…#5801) (#6835) ## Summary When a rebuild's recreate step hits a transient fault **after** the old sandbox was deleted but **before** create recorded its name, the printed manual-recovery step 2 (`nemoclaw onboard --resume`) fails verbatim with `no sandbox name was recorded. Re-run with --name`, even though the next printed line asserts the command "will recreate sandbox '<name>'". This carries `--name <sandbox>` on the resume retry form so the documented recovery runs as printed. ## Related Issue Addresses the residual-recovery gap discussed in #5801 (the non-atomic rebuild window from #2273 / #5787). This does not close #5801 — the atomic build-verify-swap it asks for still needs upstream work — it makes the existing preserved-backup mitigation actually followable. ## Changes - `src/lib/actions/sandbox/rebuild-mcp-phase.ts`: the no-MCP `onboard --resume` fallback in `printMcpRebuildRetryCommand` now emits `--name <sandbox>`. The MCP-bearing form already carried the name via the `<name> rebuild` subcommand; only this fallback dropped it. - Tests updated/added in `rebuild-mcp-phase.test.ts` and the shared `test/helpers/rebuild-flow-recovery-cases.ts` to lock the sandbox name into the printed resume command. No new abstraction, config, or fallback path — a one-argument fix to an existing printed command. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — justification: no user-facing docs describe this internal recovery print; the change makes an existing printed command runnable. - [x] 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: **requesting maintainer review; change is limited to recovery-guidance text plus its tests, no lifecycle/side-effect behavior.** ## Verification - [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 — `npm run check:diff` exit 0 - [x] Targeted behavior tests pass for the current change set — `npx vitest run src/lib/actions/sandbox/rebuild-mcp-phase.test.ts` (9 passed) and every rebuild-flow / recovery-helper file consuming the shared assertion passes in isolation (rebuild-flow.test.ts plus the 8 `rebuild-*` files that use `rebuild-flow-recovery-cases.ts`). Note for honesty: running all of those files together under full parallel load intermittently trips pre-existing 5000 ms-timeout flakiness in unrelated DCode/provider tests (`rebuild-dcode-*`, `rebuild-local-provider-recreate`); those pass individually, live in files this change does not modify (it only edits a print function and its two test files), and fail with variable membership run-to-run — the signature of a load/timeout flake rather than a regression. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed ### Reproduction / evidence Verified by execution on `main` (`3de1de6b1`), docker driver, with a shim that fails only `openshell sandbox create` (a post-delete transient fault): - **Before:** rebuild prints `2. Run: nemoclaw onboard --resume --tool-disclosure progressive`; running it verbatim fails with `Cannot resume non-interactive onboard: ... no sandbox name was recorded. Re-run with --name <sandbox>`. - **After:** the compiled printer emits `nemoclaw onboard --resume --name rebuild-probe --tool-disclosure progressive`; that exact command completed recovery live (sandbox returned `Ready`/healthy), followed by `snapshot restore` of the preserved backup (14 directories, 1 file). --- Signed-off-by: Hokonoken <41166525+Hokonoken@users.noreply.github.com> Signed-off-by: Hokonoken <41166525+Hokonoken@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The installer runs
upgrade-sandboxes --autoimmediately after onboarding. When the operator had exported a different agent/provider for that onboard (e.g.NEMOCLAW_AGENT=langchain-deepagents-code+NEMOCLAW_PROVIDER_KEY=sk-...), those ambient values leaked into every existing sandbox's rebuild: the old OpenClaw sandbox was backed up, deleted, then recreated as the wrong agent (Deep Agents) with an invalid key — destroying the sandbox before a failed/mismatched recreate, while the installer still printed a clean "Installation complete" banner. A rebuild must recreate a sandbox from its own recorded registry/session config, never from an unrelated onboard's ambient env.Related Issue
Fixes #5735
Changes
NEMOCLAW_AGENT,NEMOCLAW_PROVIDER,NEMOCLAW_PROVIDER_KEY,NEMOCLAW_ENDPOINT_URL,NEMOCLAW_MODELare removed for the duration of theonboard --resumerecreate (restored infinally), so the registry-pinned session and the already-registered gateway provider win. Newrebuild-env-isolation.tshelper.credentialEnvfrom the target registry provider; repin the endpoint from the provider's canonical config when the loaded session belongs to a different sandbox.nvidia-routerand known remotes stay registry/blueprint-derivable and are not aborted).upgrade-sandboxes --autono longer prints a clean completion banner; it reports "completed with warnings" plus the affected sandbox / backup path / recovery commands.Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesReal-CLI E2E (live gateway, isolated dedicated-port gateway + temp registry): created a real OpenClaw
nvidia-prodsandbox, marked it stale, exported the reporter'sNEMOCLAW_AGENT=langchain-deepagents-code+NEMOCLAW_PROVIDER_KEY=sk-..., and ran./bin/nemoclaw.js upgrade-sandboxes --auto.agent=langchain-deepagents-code) with incomplete state restore — data loss.Ready.Targeted unit tests:
rebuild-env-isolation.test.ts,rebuild-flow.test.ts(env isolation + custom-endpoint pre-delete abort + nvidia-router/known-remote proceed),install-upgrade-sandboxes-severity.test.ts, plusrepro-2201.test.tsand the sandbox/onboard suites.tsc -p tsconfig.cli.jsonclean; reviewed withcodex review --uncommitted(findings addressed).Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
onboard --resumeconfiguration, with conditional endpoint pinning and safer resume/session updates.Supersedes #5770 (reopened from NVIDIA/NemoClaw branch so trusted advisor workflows can run).