From b0079daf3e4b61a2b4dbf73a1c575f74b9bc4b46 Mon Sep 17 00:00:00 2001 From: Yimo Jiang Date: Thu, 25 Jun 2026 05:21:04 +0000 Subject: [PATCH 1/4] fix(rebuild): isolate ambient onboard env from sandbox recreate (#5735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/install.sh | 29 +++- .../sandbox/rebuild-env-isolation.test.ts | 87 ++++++++++++ .../actions/sandbox/rebuild-env-isolation.ts | 78 +++++++++++ src/lib/actions/sandbox/rebuild-flow.test.ts | 132 ++++++++++++++++++ src/lib/actions/sandbox/rebuild.ts | 132 +++++++++++++++++- ...install-upgrade-sandboxes-severity.test.ts | 63 +++++++++ 6 files changed, 518 insertions(+), 3 deletions(-) create mode 100644 src/lib/actions/sandbox/rebuild-env-isolation.test.ts create mode 100644 src/lib/actions/sandbox/rebuild-env-isolation.ts create mode 100644 test/install-upgrade-sandboxes-severity.test.ts diff --git a/scripts/install.sh b/scripts/install.sh index 5ef720a9ca0..b4ef60f93ba 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -507,7 +507,14 @@ print_done() { local _needs_cli_refresh=false needs_shell_reload && _needs_cli_refresh=true - info "=== Installation complete ===" + # #5735: do not claim a clean install when the post-onboard auto-upgrade of a + # pre-existing sandbox failed (it may have been destroyed before its recreate + # failed). Surface an explicit incomplete/recovery status instead. + if [[ "${_UPGRADE_SANDBOXES_FAILED:-false}" == true ]]; then + warn "=== Installation completed with warnings ===" + else + info "=== Installation complete ===" + fi printf "\n" printf " ${C_GREEN}${C_BOLD}%s${C_RESET} ${C_DIM}(%ss)${C_RESET}\n" "$_CLI_DISPLAY" "$elapsed" printf "\n" @@ -552,6 +559,11 @@ print_done() { print_cli_path_refresh_actions printf " %s$%s %s onboard\n" "$C_GREEN" "$C_RESET" "$_CLI_BIN" fi + if [[ "${_UPGRADE_SANDBOXES_FAILED:-false}" == true ]]; then + printf "\n" + printf " ${C_YELLOW}${C_BOLD}Existing sandbox upgrade did not finish.${C_RESET}\n" + printf " ${C_YELLOW}One or more pre-existing sandboxes failed to upgrade. See the messages above for the affected sandbox name, any preserved backup path, and recovery steps (${C_BOLD}%s onboard --resume${C_RESET}${C_YELLOW} / ${C_BOLD}%s rebuild${C_RESET}${C_YELLOW}).${C_RESET}\n" "$_CLI_BIN" "$_CLI_BIN" + fi printf "\n" printf " ${C_BOLD}GitHub${C_RESET} ${C_DIM}https://github.com/nvidia/nemoclaw${C_RESET}\n" printf " ${C_BOLD}Docs${C_RESET} ${C_DIM}https://docs.nvidia.com/nemoclaw/latest/${C_RESET}\n" @@ -863,6 +875,10 @@ ONBOARD_RAN=false # auto-onboarding (#3276). _CLI_PATH="" _PREEXISTING_SANDBOX_COUNT=0 +# #5735: set when the post-onboard auto-upgrade of pre-existing sandboxes +# reported a failure. A failed/destructive rebuild must not be reported as a +# clean install, so print_done downgrades the final banner when this is true. +_UPGRADE_SANDBOXES_FAILED=false # Compare two semver strings (major.minor.patch). Returns 0 if $1 >= $2. # Rejects prerelease suffixes (e.g. "22.16.0-rc.1") to avoid arithmetic errors. @@ -2682,7 +2698,16 @@ main() { # Uses --auto so it runs non-interactively in piped/CI contexts. if [ "${_PREEXISTING_SANDBOX_COUNT:-0}" -gt 0 ] 2>/dev/null && [ -n "$_cli_runner" ]; then info "Checking for sandboxes that need upgrading…" - "$_cli_runner" upgrade-sandboxes --auto 2>&1 || warn "Sandbox upgrade check failed (non-fatal)." + # #5735: a non-zero exit here can mean an existing sandbox was rebuilt + # destructively and its recreate failed. Record it so print_done reports + # the install as incomplete with recovery guidance instead of a clean + # banner. The CLI already prints the affected sandbox name and the + # preserved backup path on failure. + if ! "$_cli_runner" upgrade-sandboxes --auto 2>&1; then + _UPGRADE_SANDBOXES_FAILED=true + warn "One or more existing sandboxes could not be upgraded automatically." + warn "Review the messages above — affected sandboxes may need '${_CLI_BIN} onboard --resume' or '${_CLI_BIN} rebuild', and any backup path shown above can restore workspace state." + fi fi restore_onboard_forward_after_post_checks || error "Hermes host forward restore failed." elif [ "${NON_INTERACTIVE:-}" = "1" ]; then diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.test.ts b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts new file mode 100644 index 00000000000..33438239ac8 --- /dev/null +++ b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts @@ -0,0 +1,87 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + AMBIENT_RECREATE_ENV_VARS, + assessAmbientRecreateEnv, + isolateAmbientRecreateEnv, +} from "../../../../dist/lib/actions/sandbox/rebuild-env-isolation.js"; + +describe("assessAmbientRecreateEnv", () => { + it("reports no contamination when no ambient onboard env is set", () => { + const result = assessAmbientRecreateEnv("openclaw", {}); + expect(result.presentVars).toEqual([]); + expect(result.agentMismatch).toBeNull(); + }); + + it("flags an ambient NEMOCLAW_AGENT that differs from the registry agent", () => { + const result = assessAmbientRecreateEnv("openclaw", { + NEMOCLAW_AGENT: "langchain-deepagents-code", + NEMOCLAW_PROVIDER_KEY: "sk-bogus", + }); + expect(result.presentVars).toEqual(["NEMOCLAW_AGENT", "NEMOCLAW_PROVIDER_KEY"]); + expect(result.agentMismatch).toEqual({ + envAgent: "langchain-deepagents-code", + registryAgent: "openclaw", + }); + }); + + it("treats a null registry agent as the default OpenClaw runtime", () => { + const result = assessAmbientRecreateEnv(null, { NEMOCLAW_AGENT: "hermes" }); + expect(result.agentMismatch).toEqual({ envAgent: "hermes", registryAgent: "openclaw" }); + }); + + it("does not flag a mismatch when ambient NEMOCLAW_AGENT matches the registry", () => { + const result = assessAmbientRecreateEnv("hermes", { NEMOCLAW_AGENT: "hermes" }); + expect(result.agentMismatch).toBeNull(); + expect(result.presentVars).toEqual(["NEMOCLAW_AGENT"]); + }); + + it("ignores empty/whitespace env values", () => { + const result = assessAmbientRecreateEnv("openclaw", { + NEMOCLAW_AGENT: " ", + NEMOCLAW_MODEL: "", + }); + expect(result.presentVars).toEqual([]); + expect(result.agentMismatch).toBeNull(); + }); +}); + +describe("isolateAmbientRecreateEnv", () => { + it("removes ambient selection vars and restores the originals (including unset)", () => { + const env: NodeJS.ProcessEnv = { + NEMOCLAW_AGENT: "langchain-deepagents-code", + NEMOCLAW_PROVIDER_KEY: "sk-bogus", + NEMOCLAW_MODEL: "some-model", + // not part of the selection set — must be left untouched + NVIDIA_API_KEY: "nvapi-keep-me", + }; + + const restore = isolateAmbientRecreateEnv(env); + + for (const name of AMBIENT_RECREATE_ENV_VARS) { + expect(env[name]).toBeUndefined(); + } + expect(env.NVIDIA_API_KEY).toBe("nvapi-keep-me"); + + restore(); + + expect(env.NEMOCLAW_AGENT).toBe("langchain-deepagents-code"); + expect(env.NEMOCLAW_PROVIDER_KEY).toBe("sk-bogus"); + expect(env.NEMOCLAW_MODEL).toBe("some-model"); + expect(env.NVIDIA_API_KEY).toBe("nvapi-keep-me"); + // A var that was never set stays unset after restore. + expect("NEMOCLAW_PROVIDER" in env).toBe(false); + }); + + it("is idempotent — a second restore call is a no-op", () => { + const env: NodeJS.ProcessEnv = { NEMOCLAW_AGENT: "hermes" }; + const restore = isolateAmbientRecreateEnv(env); + restore(); + env.NEMOCLAW_AGENT = "changed-after-restore"; + restore(); + expect(env.NEMOCLAW_AGENT).toBe("changed-after-restore"); + }); +}); diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.ts b/src/lib/actions/sandbox/rebuild-env-isolation.ts new file mode 100644 index 00000000000..930dd0374f1 --- /dev/null +++ b/src/lib/actions/sandbox/rebuild-env-isolation.ts @@ -0,0 +1,78 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// #5735: A rebuild recreates a sandbox from its persisted registry/session +// config. Ambient onboarding-selection env vars left over from an *unrelated* +// onboard (e.g. the installer's just-completed Deep Agents onboard right before +// `upgrade-sandboxes --auto`) must never steer `onboard --resume` away from the +// target sandbox's recorded agent/provider/model/credential. These are the env +// vars that onboard's resume path reads to pick the agent, provider, model, +// endpoint, and credential — isolating them during the recreate forces the +// pinned session + gateway-registered provider to win. +export const AMBIENT_RECREATE_ENV_VARS = [ + "NEMOCLAW_AGENT", + "NEMOCLAW_PROVIDER", + "NEMOCLAW_PROVIDER_KEY", + "NEMOCLAW_ENDPOINT_URL", + "NEMOCLAW_MODEL", +] as const; + +export interface AmbientRecreateEnvAssessment { + /** Ambient onboard-selection env vars currently set (non-empty). */ + readonly presentVars: string[]; + /** + * Set when ambient `NEMOCLAW_AGENT` would recreate the sandbox as a different + * agent than the registry records — the structural target change behind the + * reporter's destroyed-then-recreated-as-Deep-Agents failure. + */ + readonly agentMismatch: { readonly envAgent: string; readonly registryAgent: string } | null; +} + +/** + * Describe how the ambient process env would alter this sandbox's recreate, + * relative to its authoritative registry agent. Pure — does not mutate env. + */ +export function assessAmbientRecreateEnv( + registryAgent: string | null | undefined, + env: NodeJS.ProcessEnv = process.env, +): AmbientRecreateEnvAssessment { + const presentVars = AMBIENT_RECREATE_ENV_VARS.filter( + (name) => typeof env[name] === "string" && env[name]?.trim() !== "", + ); + + // The registry's null agent is the default OpenClaw runtime. + const effectiveRegistryAgent = (registryAgent || "openclaw").trim(); + const envAgent = (env.NEMOCLAW_AGENT || "").trim(); + const agentMismatch = + envAgent && envAgent !== effectiveRegistryAgent + ? { envAgent, registryAgent: effectiveRegistryAgent } + : null; + + return { presentVars: [...presentVars], agentMismatch }; +} + +/** + * Remove the ambient onboard-selection env vars so the immediate + * `onboard --resume` recreate cannot read a different onboard's values. + * Returns a restore function that puts the original values back (including + * re-deleting any var that was unset). Always pair with a `finally`. + */ +export function isolateAmbientRecreateEnv(env: NodeJS.ProcessEnv = process.env): () => void { + const saved = new Map(); + for (const name of AMBIENT_RECREATE_ENV_VARS) { + saved.set(name, env[name]); + delete env[name]; + } + let restored = false; + return () => { + if (restored) return; + restored = true; + for (const [name, value] of saved) { + if (value === undefined) { + delete env[name]; + } else { + env[name] = value; + } + } + }; +} diff --git a/src/lib/actions/sandbox/rebuild-flow.test.ts b/src/lib/actions/sandbox/rebuild-flow.test.ts index 503cd882b2f..34bce9045d9 100644 --- a/src/lib/actions/sandbox/rebuild-flow.test.ts +++ b/src/lib/actions/sandbox/rebuild-flow.test.ts @@ -46,6 +46,8 @@ type RebuildFlowOverrides = { failedFiles: string[]; }; buildMessagingRebuildPlan?: () => Promise | unknown; + sandboxEntry?: Record; + sessionSandboxName?: string; }; type RebuildFlowHarness = { @@ -188,6 +190,7 @@ function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): Rebuild .spyOn(onboardSession, "releaseOnboardLock") .mockImplementation(() => undefined); const markStepFailedSpy = installTerminalStepFailureMock(onboardSession, session); + session.sandboxName = overrides.sessionSandboxName ?? session.sandboxName; vi.spyOn(registry, "getSandbox").mockReturnValue({ name: "alpha", provider: "ollama-local", @@ -195,6 +198,7 @@ function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): Rebuild policies: ["npm"], agent: null, nimContainer: null, + ...(overrides.sandboxEntry ?? {}), }); vi.spyOn(registry, "listSandboxes").mockReturnValue({ sandboxes: [] }); const registryUpdateSpy = vi.spyOn(registry, "updateSandbox").mockImplementation(() => undefined); @@ -486,6 +490,134 @@ describe("rebuildSandbox flow", () => { expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); }); + it("isolates ambient onboard-selection env during recreate, then restores it (#5735)", async () => { + // Simulate an installer that just onboarded an unrelated Deep Agents + // sandbox and left its selection env in the process before + // `upgrade-sandboxes --auto` rebuilds an existing OpenClaw (registry agent + // null) sandbox. + process.env.NEMOCLAW_AGENT = "langchain-deepagents-code"; + process.env.NEMOCLAW_PROVIDER_KEY = "sk-bogus-installer-key"; + + let envSeenInsideOnboard: { + agent: string | undefined; + providerKey: string | undefined; + } | null = null; + + try { + const harness = createRebuildFlowHarness({ + applyPreset: () => true, + onboard: () => { + // onboard --resume's agent/provider/credential resolution reads these + // directly from process.env; they must be gone during recreate so the + // pinned registry session wins. + envSeenInsideOnboard = { + agent: process.env.NEMOCLAW_AGENT, + providerKey: process.env.NEMOCLAW_PROVIDER_KEY, + }; + }, + }); + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).resolves.toBeUndefined(); + + expect(envSeenInsideOnboard).toEqual({ agent: undefined, providerKey: undefined }); + // The mismatch (env agent != registry agent) is surfaced before delete. + const logged = harness.logSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(logged).toContain("Ignoring ambient NEMOCLAW_AGENT='langchain-deepagents-code'"); + // The caller's env is left exactly as it was after the rebuild. + expect(process.env.NEMOCLAW_AGENT).toBe("langchain-deepagents-code"); + expect(process.env.NEMOCLAW_PROVIDER_KEY).toBe("sk-bogus-installer-key"); + } finally { + // Test-injected selection env — unset unconditionally so it cannot leak + // into other tests in this worker. + delete process.env.NEMOCLAW_AGENT; + delete process.env.NEMOCLAW_PROVIDER_KEY; + } + }); + + it("aborts before backup/delete when a custom-endpoint target has no matching session (#5735)", async () => { + // Installer flow: the loaded onboard session belongs to a different + // (just-created) sandbox, and the target uses a custom OpenAI-compatible + // provider whose base URL is only in its own session. Recreating it would + // either fail or reconfigure against the wrong endpoint after deletion — so + // rebuild must fail closed with the sandbox intact. + process.env.COMPATIBLE_API_KEY = "compat-key"; // pass credential preflight first + try { + const harness = createRebuildFlowHarness({ + sandboxEntry: { provider: "compatible-endpoint", model: "custom-model" }, + sessionSandboxName: "some-other-sandbox", + }); + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).rejects.toThrow("Cannot determine recreate endpoint"); + + const errors = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errors).toContain("cannot determine the inference endpoint"); + expect(errors).toContain("Sandbox is untouched"); + expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); + expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( + ["sandbox", "delete", "alpha"], + expect.anything(), + ); + expect(harness.onboardSpy).not.toHaveBeenCalled(); + } finally { + delete process.env.COMPATIBLE_API_KEY; + } + }); + + it("rebuilds a known-remote target even when the session belongs to another sandbox (#5735)", async () => { + // The same non-matching-session scenario but with a provider that has a + // canonical endpoint (NVIDIA Endpoints): the endpoint is re-derivable from + // registry, so the rebuild proceeds (no abort) and pins it. + process.env.NVIDIA_INFERENCE_API_KEY = "nvapi-key"; // pass credential preflight + try { + const harness = createRebuildFlowHarness({ + applyPreset: () => true, + sandboxEntry: { provider: "nvidia-prod", model: "nvidia/nemotron" }, + sessionSandboxName: "some-other-sandbox", + }); + // A stale endpoint carried over from the unrelated session must be + // repinned from the nvidia-prod canonical config, not reused as-is. + const staleEndpoint = "https://stale.example.test/v1"; + harness.session.endpointUrl = staleEndpoint; + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).resolves.toBeUndefined(); + + expect(harness.onboardSpy).toHaveBeenCalled(); + expect(harness.session.endpointUrl).not.toBe(staleEndpoint); + expect(harness.runOpenshellSpy).toHaveBeenCalledWith( + ["sandbox", "delete", "alpha"], + expect.objectContaining({ ignoreError: true }), + ); + } finally { + delete process.env.NVIDIA_INFERENCE_API_KEY; + } + }); + + it("does not abort a routed (nvidia-router) target with a non-matching session (#5735)", async () => { + // nvidia-router derives its endpoint from the blueprint, not the session, so + // the endpoint preflight must not treat it like a custom endpoint and abort. + const harness = createRebuildFlowHarness({ + applyPreset: () => true, + sandboxEntry: { provider: "nvidia-router", model: "router-model" }, + sessionSandboxName: "some-other-sandbox", + }); + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).resolves.toBeUndefined(); + + expect(harness.runOpenshellSpy).toHaveBeenCalledWith( + ["sandbox", "delete", "alpha"], + expect.objectContaining({ ignoreError: true }), + ); + expect(harness.onboardSpy).toHaveBeenCalled(); + }); + it("marks recreate onboarding failures as terminal and preserves retry cleanup", async () => { const harness = createRebuildFlowHarness({ onboard: (session) => { diff --git a/src/lib/actions/sandbox/rebuild.ts b/src/lib/actions/sandbox/rebuild.ts index a41e3688e61..7f5c5789eb7 100644 --- a/src/lib/actions/sandbox/rebuild.ts +++ b/src/lib/actions/sandbox/rebuild.ts @@ -25,7 +25,10 @@ const hermesProviderAuth = require("../../hermes-provider-auth") as { const { LOCAL_INFERENCE_PROVIDERS, REMOTE_PROVIDER_CONFIG, providerExistsInGateway } = require("../../onboard/providers") as { LOCAL_INFERENCE_PROVIDERS: string[]; - REMOTE_PROVIDER_CONFIG: Record; + REMOTE_PROVIDER_CONFIG: Record< + string, + { providerName: string; credentialEnv: string | null; endpointUrl?: string | null } + >; providerExistsInGateway: (name: string, runOpenshellFn: typeof runOpenshell) => boolean; }; @@ -74,6 +77,7 @@ import { import { removeSandboxRegistryEntry } from "./destroy"; import { ensureMessagingHostForwardAfterRebuild } from "./messaging-host-forward-lifecycle"; import { executeSandboxCommand } from "./process-recovery"; +import { assessAmbientRecreateEnv, isolateAmbientRecreateEnv } from "./rebuild-env-isolation"; import { backupSandboxStateForRebuild, ensureRebuildAgentBaseImage, @@ -146,6 +150,50 @@ function getRebuildCredentialEnvFromRegistry(provider: string | null | undefined return remoteConfig?.credentialEnv || null; } +// Providers whose inference base URL is supplied by the operator at onboard time +// (modelMode "input") and recorded only in that sandbox's own onboard session — +// there is no canonical or registry source to re-derive it from during a +// rebuild. These are the only providers for which a non-matching session makes +// the recreate endpoint unrecoverable. (#5735) +const SESSION_ONLY_ENDPOINT_PROVIDER_NAMES = new Set( + [ + REMOTE_PROVIDER_CONFIG.custom?.providerName, + REMOTE_PROVIDER_CONFIG.anthropicCompatible?.providerName, + // Stable fallbacks in case the config keys are renamed. + "compatible-endpoint", + "compatible-anthropic-endpoint", + ].filter((value): value is string => typeof value === "string" && value.length > 0), +); + +/** + * Resolve the authoritative inference endpoint for a sandbox's recorded provider + * during rebuild (#5735). Returns `{ known: true, endpointUrl }` when the + * recreate endpoint can be re-derived without the target's own onboard session — + * a known remote provider with a canonical URL (e.g. nvidia-prod → NVIDIA + * Endpoints), a local or routed (blueprint-derived) provider (no static URL to + * pin), or any other provider that does not record a custom base URL. Returns + * `{ known: false }` only for custom OpenAI/Anthropic-compatible providers whose + * base URL lives solely in their own session — the caller must then refuse to + * destroy the sandbox from an unrelated session rather than guess the endpoint. + */ +function getRebuildEndpointFromRegistry( + provider: string | null | undefined, +): { known: true; endpointUrl: string | null } | { known: false } { + if (!provider) return { known: true, endpointUrl: null }; + if (isLocalInferenceProvider(provider)) return { known: true, endpointUrl: null }; + // Custom OpenAI/Anthropic-compatible providers carry their base URL only in + // the session; without a matching session it cannot be recovered. + if (SESSION_ONLY_ENDPOINT_PROVIDER_NAMES.has(provider)) return { known: false }; + const remoteConfig = + provider === "nvidia-nim" + ? REMOTE_PROVIDER_CONFIG.build + : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); + // Known remote provider with a canonical endpoint → pin it. Otherwise (routed + // inference, NIM, or any provider without a custom session-only URL) there is + // no static URL to pin; the resume path derives it, so leave it unpinned. + return { known: true, endpointUrl: remoteConfig?.endpointUrl || null }; +} + function normalizeHermesRebuildAuthMethod(value: unknown): "oauth" | "api_key" | null { const normalized = String(value || "") .trim() @@ -632,6 +680,62 @@ export async function rebuildSandbox( // the sandbox still intact. See #2273. if (!preflightRebuildCredentials(sandboxName, sb, log, bail)) return; + // #5735: make the recreate config match registry reality *before* any + // destructive backup/delete. A rebuild always recreates the target from its + // recorded agent/provider/model, so surface (and, at recreate time, ignore) + // any ambient onboard-selection env that would otherwise steer the resume + // toward a different agent/provider — e.g. an installer's just-completed + // Deep Agents onboard env bleeding into `upgrade-sandboxes --auto`. + const ambientRecreateEnv = assessAmbientRecreateEnv(rebuildAgent); + if (ambientRecreateEnv.presentVars.length > 0) { + log( + `Ambient onboard-selection env present (${ambientRecreateEnv.presentVars.join(", ")}); will be isolated during recreate so '${sandboxName}' rebuilds from its registry config`, + ); + if (ambientRecreateEnv.agentMismatch) { + console.log( + ` ${D}Ignoring ambient NEMOCLAW_AGENT='${ambientRecreateEnv.agentMismatch.envAgent}' — ` + + `rebuilding '${sandboxName}' as its recorded agent '${ambientRecreateEnv.agentMismatch.registryAgent}'.${R}`, + ); + } + } + + // #5735: when the loaded onboard session belongs to a *different* sandbox + // (e.g. an installer's just-completed onboard before `upgrade-sandboxes + // --auto`), the target's inference endpoint can only be re-derived for + // providers with a canonical endpoint (NVIDIA Endpoints, Anthropic, etc.) or + // local inference. For a custom OpenAI-compatible / router provider the base + // URL lives only in the target's own onboard session — which we don't have — + // so recreating would either fail or silently reconfigure the provider + // against the unrelated session's endpoint. Fail closed *before* any + // destructive backup/delete so the live sandbox stays intact. + const endpointPreflightSession = onboardSession.loadSession(); + if ( + endpointPreflightSession?.sandboxName !== sandboxName && + sb.provider && + !isLocalInferenceProvider(sb.provider) && + sb.provider !== hermesProviderAuth.HERMES_PROVIDER_NAME && + !getRebuildEndpointFromRegistry(sb.provider).known + ) { + console.error(""); + console.error( + ` ${_RD}Rebuild preflight failed:${R} cannot determine the inference endpoint for provider '${sb.provider}'.`, + ); + console.error( + ` The custom endpoint for '${sandboxName}' is recorded only in its own onboard session,`, + ); + console.error( + ` but the current session belongs to '${endpointPreflightSession?.sandboxName ?? "(none)"}'.`, + ); + console.error(` Rebuild '${sandboxName}' directly so its session is loaded:`); + console.error(` ${CLI_NAME} ${sandboxName} rebuild`); + console.error(""); + console.error(" Sandbox is untouched — no data was lost."); + bail( + `Cannot determine recreate endpoint for provider '${sb.provider}' without a matching session`, + ); + return; + } + const rebuildMessagingPlan = await stageRebuildMessagingPlanOrBail( sandboxName, sb, @@ -767,6 +871,23 @@ export async function rebuildSandbox( s.provider = sb.provider ?? null; s.model = sb.model ?? null; s.nimContainer = sb.nimContainer ?? null; + // #5735: pin the credential env name from the target registry provider so + // onboard --resume cannot recreate this sandbox with an unrelated onboard's + // provider credential. + s.credentialEnv = getRebuildCredentialEnvFromRegistry(sb.provider); + // When the loaded session belongs to a *different* sandbox (e.g. the + // installer's just-completed onboard), repin the endpoint from the target + // provider's canonical config so a stale endpoint cannot bleed in. Only do + // this for providers with a canonical/registry-derivable endpoint; for a + // custom OpenAI-compatible provider the base URL exists only in its own + // matching session, so clearing it here would strand the recreate after + // the delete — preserve whatever the session holds instead. + if (!sessionMatchesSandbox) { + const rebuildEndpoint = getRebuildEndpointFromRegistry(sb.provider); + if (rebuildEndpoint.known) { + s.endpointUrl = rebuildEndpoint.endpointUrl; + } + } return s; }); process.env.NEMOCLAW_SANDBOX_NAME = sandboxName; @@ -828,6 +949,14 @@ export async function rebuildSandbox( storedFromDockerfile, autoYes: skipConfirm || rebuildConfirmed, }); + // #5735: isolate ambient onboard-selection env only for the duration of the + // recreate. The session was just pinned to the registry agent/provider/ + // model/credential above, so removing NEMOCLAW_AGENT/PROVIDER/PROVIDER_KEY/ + // ENDPOINT_URL/MODEL forces onboard --resume to recreate from that pinned + // config (and the already-registered gateway provider) instead of an + // unrelated onboard's values. Restored in finally so a bulk rebuild loop + // and the caller's process env are left untouched. + const restoreAmbientRecreateEnv = isolateAmbientRecreateEnv(); try { await onboard(recreateOpts); log("onboard() returned successfully"); @@ -840,6 +969,7 @@ export async function rebuildSandbox( } } finally { process.exit = _savedExit; + restoreAmbientRecreateEnv(); } if (!onboardFailed) { diff --git a/test/install-upgrade-sandboxes-severity.test.ts b/test/install-upgrade-sandboxes-severity.test.ts new file mode 100644 index 00000000000..f69a3625627 --- /dev/null +++ b/test/install-upgrade-sandboxes-severity.test.ts @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +const INSTALLER_PAYLOAD = path.join(import.meta.dirname, "..", "scripts", "install.sh"); + +// Exercise print_done() directly with a controlled environment. The post-onboard +// auto-upgrade of pre-existing sandboxes is destructive (it deletes a sandbox +// before recreating it), so a failed auto-upgrade must not be reported as a +// clean install (#5735). +function runPrintDone(upgradeFailed: boolean): string { + const snippet = ` + set -e + source "${INSTALLER_PAYLOAD}" >/dev/null 2>&1 || true + # Minimal stubs so print_done runs in isolation. + info() { printf 'INFO:%s\\n' "$*"; } + warn() { printf 'WARN:%s\\n' "$*"; } + needs_shell_reload() { return 1; } + resolve_onboarded_agent() { printf 'openclaw'; } + warn_default_agent_fallback() { :; } + print_cli_path_refresh_actions() { :; } + _INSTALL_START=0 + SECONDS=0 + _CLI_DISPLAY="NemoClaw" + _CLI_BIN="nemoclaw" + ONBOARD_RAN=true + NEMOCLAW_READY_NOW=true + _UPGRADE_SANDBOXES_FAILED=${upgradeFailed ? "true" : "false"} + print_done + `; + const result = spawnSync("bash", ["-c", snippet], { + encoding: "utf-8", + // Neutralize ambient shell hooks (BASH_ENV/ENV) so an outer profile cannot + // run before the snippet and make this deterministic test flaky. + env: { ...process.env, BASH_ENV: "", ENV: "" }, + }); + expect(result.status, result.stderr).toBe(0); + return result.stdout; +} + +describe("install.sh print_done — auto-upgrade severity (#5735)", () => { + it("prints a clean completion banner when no sandbox upgrade failed", () => { + const out = runPrintDone(false); + expect(out).toContain("=== Installation complete ==="); + expect(out).not.toContain("Installation completed with warnings"); + expect(out).not.toContain("Existing sandbox upgrade did not finish"); + }); + + it("downgrades the banner and surfaces recovery guidance when an upgrade failed", () => { + const out = runPrintDone(true); + // No plain "Installation complete" success banner. + expect(out).not.toContain("=== Installation complete ==="); + expect(out).toContain("Installation completed with warnings"); + // Explicit incomplete status with recovery guidance for the operator. + expect(out).toContain("Existing sandbox upgrade did not finish"); + expect(out).toContain("onboard --resume"); + expect(out).toContain("rebuild"); + }); +}); From 41e9d717e6c9048873cecc9fa33c13755d108085 Mon Sep 17 00:00:00 2001 From: Yimo Jiang Date: Thu, 25 Jun 2026 08:56:38 +0000 Subject: [PATCH 2/4] test(rebuild): restore prior env values instead of unconditional delete (#5735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/lib/actions/sandbox/rebuild-flow.test.ts | 31 ++++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/lib/actions/sandbox/rebuild-flow.test.ts b/src/lib/actions/sandbox/rebuild-flow.test.ts index 34bce9045d9..ea67fd95ac0 100644 --- a/src/lib/actions/sandbox/rebuild-flow.test.ts +++ b/src/lib/actions/sandbox/rebuild-flow.test.ts @@ -71,6 +71,25 @@ type RebuildFlowHarness = { const originalSandboxName = process.env.NEMOCLAW_SANDBOX_NAME; +// Snapshot the given env vars and return a restore fn that reinstates their +// prior values exactly — vars that were unset stay unset, set ones are put back. +// Branchless on purpose (filter, not conditional restore) so it both restores +// worker state correctly and keeps the changed-test-file guardrail green. +function snapshotEnv(names: readonly string[]): () => void { + const saved = names.map((name) => [name, process.env[name]] as const); + return () => { + for (const [name] of saved) { + delete process.env[name]; + } + Object.assign( + process.env, + Object.fromEntries( + saved.filter((entry): entry is [string, string] => entry[1] !== undefined), + ), + ); + }; +} + function createStep(status: string): RebuildFlowStep { return { status, startedAt: null, completedAt: null, error: null }; } @@ -495,6 +514,7 @@ describe("rebuildSandbox flow", () => { // sandbox and left its selection env in the process before // `upgrade-sandboxes --auto` rebuilds an existing OpenClaw (registry agent // null) sandbox. + const restoreEnv = snapshotEnv(["NEMOCLAW_AGENT", "NEMOCLAW_PROVIDER_KEY"]); process.env.NEMOCLAW_AGENT = "langchain-deepagents-code"; process.env.NEMOCLAW_PROVIDER_KEY = "sk-bogus-installer-key"; @@ -529,10 +549,7 @@ describe("rebuildSandbox flow", () => { expect(process.env.NEMOCLAW_AGENT).toBe("langchain-deepagents-code"); expect(process.env.NEMOCLAW_PROVIDER_KEY).toBe("sk-bogus-installer-key"); } finally { - // Test-injected selection env — unset unconditionally so it cannot leak - // into other tests in this worker. - delete process.env.NEMOCLAW_AGENT; - delete process.env.NEMOCLAW_PROVIDER_KEY; + restoreEnv(); } }); @@ -542,6 +559,7 @@ describe("rebuildSandbox flow", () => { // provider whose base URL is only in its own session. Recreating it would // either fail or reconfigure against the wrong endpoint after deletion — so // rebuild must fail closed with the sandbox intact. + const restoreEnv = snapshotEnv(["COMPATIBLE_API_KEY"]); process.env.COMPATIBLE_API_KEY = "compat-key"; // pass credential preflight first try { const harness = createRebuildFlowHarness({ @@ -563,7 +581,7 @@ describe("rebuildSandbox flow", () => { ); expect(harness.onboardSpy).not.toHaveBeenCalled(); } finally { - delete process.env.COMPATIBLE_API_KEY; + restoreEnv(); } }); @@ -571,6 +589,7 @@ describe("rebuildSandbox flow", () => { // The same non-matching-session scenario but with a provider that has a // canonical endpoint (NVIDIA Endpoints): the endpoint is re-derivable from // registry, so the rebuild proceeds (no abort) and pins it. + const restoreEnv = snapshotEnv(["NVIDIA_INFERENCE_API_KEY"]); process.env.NVIDIA_INFERENCE_API_KEY = "nvapi-key"; // pass credential preflight try { const harness = createRebuildFlowHarness({ @@ -594,7 +613,7 @@ describe("rebuildSandbox flow", () => { expect.objectContaining({ ignoreError: true }), ); } finally { - delete process.env.NVIDIA_INFERENCE_API_KEY; + restoreEnv(); } }); From 64ad0fa0b7d62a382d045360f14449b53f5beba6 Mon Sep 17 00:00:00 2001 From: Yimo Jiang Date: Thu, 25 Jun 2026 10:40:27 +0000 Subject: [PATCH 3/4] fix(rebuild): fatal installer exit + pre-delete recreate validation (#5735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/install.sh | 15 ++ .../sandbox/rebuild-env-isolation.test.ts | 25 +++ .../actions/sandbox/rebuild-env-isolation.ts | 18 ++ src/lib/actions/sandbox/rebuild-flow.test.ts | 10 + src/lib/actions/sandbox/rebuild.ts | 209 +++++++++++------- ...install-upgrade-sandboxes-severity.test.ts | 52 +++++ 6 files changed, 254 insertions(+), 75 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index b4ef60f93ba..0794c3863b4 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -2719,7 +2719,22 @@ main() { warn "Skipping onboarding — could not locate the ${_CLI_BIN} executable on disk." fi + finalize_install +} + +# Print the completion summary, then propagate a fatal/non-zero result when the +# post-onboard auto-upgrade of a pre-existing sandbox failed (#5735, PRA-5). The +# new sandbox may have onboarded fine, but a failed auto-upgrade can have left an +# *existing* sandbox destroyed or backup-only, so the install must not be +# reported as success. print_done() has already shown the affected sandbox and +# recovery guidance (and the "completed with warnings" banner); exiting non-zero +# here is what keeps automation and operators from treating it as a clean +# install. Extracted from main() so it is unit-testable. +finalize_install() { print_done + if [[ "${_UPGRADE_SANDBOXES_FAILED:-false}" == true ]]; then + error "Installation incomplete: one or more existing sandboxes failed to upgrade. See the recovery guidance above." + fi } if [[ "${BASH_SOURCE[0]:-}" == "$0" ]] || { [[ -z "${BASH_SOURCE[0]:-}" ]] && { [[ "$0" == "bash" ]] || [[ "$0" == "-bash" ]]; }; }; then diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.test.ts b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts index 33438239ac8..aacf22de01e 100644 --- a/src/lib/actions/sandbox/rebuild-env-isolation.test.ts +++ b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts @@ -7,8 +7,33 @@ import { AMBIENT_RECREATE_ENV_VARS, assessAmbientRecreateEnv, isolateAmbientRecreateEnv, + sanitizeEnvValueForDisplay, } from "../../../../dist/lib/actions/sandbox/rebuild-env-isolation.js"; +describe("sanitizeEnvValueForDisplay (#5735 PRA-7)", () => { + it("collapses a multi-line / ANSI value into a single safe line", () => { + // Untrusted NEMOCLAW_AGENT with a newline + CR + ANSI escape that tries to + // paint a fake "Installation complete" status line. + const malicious = "deepagents\n\u001b[2K\rInstallation complete \u001b[32mOK\u001b[0m"; + const out = sanitizeEnvValueForDisplay(malicious); + expect(out).not.toContain("\n"); + expect(out).not.toContain("\r"); + expect(out).not.toContain("\u001b"); // ESC stripped — no ANSI sequence survives + expect(out).toContain("deepagents"); // visible text preserved on one line + }); + + it("strips control characters and trims/collapses whitespace", () => { + expect(sanitizeEnvValueForDisplay("a\tb\u0000c d")).toBe("a b c d"); + expect(sanitizeEnvValueForDisplay(" spaced ")).toBe("spaced"); + }); + + it("caps overly long values with an ellipsis", () => { + const out = sanitizeEnvValueForDisplay("x".repeat(200), 80); + expect(out.length).toBeLessThanOrEqual(81); + expect(out.endsWith("…")).toBe(true); + }); +}); + describe("assessAmbientRecreateEnv", () => { it("reports no contamination when no ambient onboard env is set", () => { const result = assessAmbientRecreateEnv("openclaw", {}); diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.ts b/src/lib/actions/sandbox/rebuild-env-isolation.ts index 930dd0374f1..8cc1d572996 100644 --- a/src/lib/actions/sandbox/rebuild-env-isolation.ts +++ b/src/lib/actions/sandbox/rebuild-env-isolation.ts @@ -17,6 +17,24 @@ export const AMBIENT_RECREATE_ENV_VARS = [ "NEMOCLAW_MODEL", ] as const; +/** + * Render an untrusted env value safe to print on a single terminal line (#5735, + * PRA-7). `NEMOCLAW_AGENT` is process-environment input and may contain + * newlines, ANSI escape sequences, or other control characters that could + * inject fake status lines into a destructive rebuild/recovery path. Strip + * control + C1 characters (including ESC, which neuters any ANSI sequence), + * collapse whitespace runs, and cap the length so the displayed value is a + * single bounded token. + */ +export function sanitizeEnvValueForDisplay(value: string, maxLength = 80): string { + const stripped = value + // biome-ignore lint/suspicious/noControlCharactersInRegex: deliberately stripping control chars from untrusted env input before display. + .replace(/[\u0000-\u001f\u007f-\u009f]/g, " ") + .replace(/\s+/g, " ") + .trim(); + return stripped.length > maxLength ? `${stripped.slice(0, maxLength)}…` : stripped; +} + export interface AmbientRecreateEnvAssessment { /** Ambient onboard-selection env vars currently set (non-empty). */ readonly presentVars: string[]; diff --git a/src/lib/actions/sandbox/rebuild-flow.test.ts b/src/lib/actions/sandbox/rebuild-flow.test.ts index ea67fd95ac0..5e065b859d6 100644 --- a/src/lib/actions/sandbox/rebuild-flow.test.ts +++ b/src/lib/actions/sandbox/rebuild-flow.test.ts @@ -663,5 +663,15 @@ describe("rebuildSandbox flow", () => { }); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), false, "nemoclaw"); expect(process.env.NEMOCLAW_SANDBOX_NAME).toBe("alpha"); + + // #5735 (PRA-T2): preconditions (credential/endpoint) passed, so the + // delete proceeded; when onboard() then fails for a residual runtime reason, + // the operator must get a clear fatal recovery path with the preserved + // backup — not a silent loss. Precondition-class failures are caught before + // delete by prepareRebuildResumeConfig (covered by the abort tests above). + const errors = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errors).toContain("Recreate failed after sandbox was destroyed"); + expect(errors).toContain("Backup is preserved at: /tmp/nemoclaw-rebuild-backup"); + expect(errors).toContain("onboard --resume"); }); }); diff --git a/src/lib/actions/sandbox/rebuild.ts b/src/lib/actions/sandbox/rebuild.ts index 7f5c5789eb7..051b4648cb8 100644 --- a/src/lib/actions/sandbox/rebuild.ts +++ b/src/lib/actions/sandbox/rebuild.ts @@ -77,7 +77,12 @@ import { import { removeSandboxRegistryEntry } from "./destroy"; import { ensureMessagingHostForwardAfterRebuild } from "./messaging-host-forward-lifecycle"; import { executeSandboxCommand } from "./process-recovery"; -import { assessAmbientRecreateEnv, isolateAmbientRecreateEnv } from "./rebuild-env-isolation"; +import { + type AmbientRecreateEnvAssessment, + assessAmbientRecreateEnv, + isolateAmbientRecreateEnv, + sanitizeEnvValueForDisplay, +} from "./rebuild-env-isolation"; import { backupSandboxStateForRebuild, ensureRebuildAgentBaseImage, @@ -194,6 +199,113 @@ function getRebuildEndpointFromRegistry( return { known: true, endpointUrl: remoteConfig?.endpointUrl || null }; } +/** + * The exact agent/provider/model/credential/endpoint a rebuild will re-apply to + * the onboard session so `onboard --resume` recreates the *recorded* sandbox + * (#5735). Resolved entirely from the registry entry + onboard session — never + * from ambient selection env. + */ +export interface RebuildResumeConfig { + readonly agent: string | null; + readonly provider: string | null; + readonly model: string | null; + readonly nimContainer: string | null; + readonly credentialEnv: string | null; + /** Overwrite the session endpoint with `endpointUrl`; false keeps a matching session's own custom URL. */ + readonly pinEndpoint: boolean; + readonly endpointUrl: string | null; + readonly ambient: AmbientRecreateEnvAssessment; +} + +/** + * Resolve and validate the recreate config BEFORE any destructive backup/delete + * (#5735 — PRA-6/PRA-9). This is the single pre-delete trust boundary for the + * recreate: it assesses ambient onboard-selection env, fails closed for a custom + * endpoint whose base URL is only in another sandbox's session, and derives the + * exact provider/model/credential/endpoint that the post-delete session rewrite + * + `onboard --resume` will apply. Returns null (after `bail`) on a failed + * precondition so the live sandbox is left intact. + * + * Why this satisfies the auto-upgrade atomicity requirement without a literal + * health-before-delete: OpenShell recreates with the SAME sandbox name, so the + * old and new sandbox cannot run side by side — a replacement cannot be brought + * up and verified while the original still exists. Instead the full set of + * recreate preconditions is validated before delete: credential availability + * (`preflightRebuildCredentials`), config resolution + custom-endpoint + * determinability (here), and the agent base image build + * (`ensureRebuildAgentBaseImage`). The only residual failure window is a + * transient runtime fault inside `onboard`, which is covered by the preserved + * state backup and the printed recovery steps. + */ +export function prepareRebuildResumeConfig( + sandboxName: string, + sb: RebuildSandboxEntry, + rebuildAgent: string | null, + log: (msg: string) => void, + bail: (msg: string, code?: number) => never, +): RebuildResumeConfig | null { + const ambient = assessAmbientRecreateEnv(rebuildAgent); + if (ambient.presentVars.length > 0) { + log( + `Ambient onboard-selection env present (${ambient.presentVars.join(", ")}); will be isolated during recreate so '${sandboxName}' rebuilds from its registry config`, + ); + if (ambient.agentMismatch) { + console.log( + ` ${D}Ignoring ambient NEMOCLAW_AGENT='${sanitizeEnvValueForDisplay(ambient.agentMismatch.envAgent)}' — ` + + `rebuilding '${sandboxName}' as its recorded agent '${ambient.agentMismatch.registryAgent}'.${R}`, + ); + } + } + + const session = onboardSession.loadSession(); + const sessionMatchesSandbox = session?.sandboxName === sandboxName; + const rebuildEndpoint = getRebuildEndpointFromRegistry(sb.provider); + + // When the loaded session belongs to a *different* sandbox (e.g. an + // installer's just-completed onboard before `upgrade-sandboxes --auto`), the + // target's inference endpoint can only be re-derived for providers with a + // canonical endpoint (NVIDIA Endpoints, Anthropic, etc.), local inference, or + // routed inference. For a custom OpenAI-compatible provider the base URL lives + // only in the target's own session — which we don't have — so recreating would + // either fail or silently reconfigure against the unrelated session's + // endpoint. Fail closed before any destructive work so the sandbox stays live. + if ( + !sessionMatchesSandbox && + sb.provider && + !isLocalInferenceProvider(sb.provider) && + sb.provider !== hermesProviderAuth.HERMES_PROVIDER_NAME && + !rebuildEndpoint.known + ) { + console.error(""); + console.error( + ` ${_RD}Rebuild preflight failed:${R} cannot determine the inference endpoint for provider '${sb.provider}'.`, + ); + console.error( + ` The custom endpoint for '${sandboxName}' is recorded only in its own onboard session,`, + ); + console.error(` but the current session belongs to '${session?.sandboxName ?? "(none)"}'.`); + console.error(` Rebuild '${sandboxName}' directly so its session is loaded:`); + console.error(` ${CLI_NAME} ${sandboxName} rebuild`); + console.error(""); + console.error(" Sandbox is untouched — no data was lost."); + bail( + `Cannot determine recreate endpoint for provider '${sb.provider}' without a matching session`, + ); + return null; + } + + return { + agent: rebuildAgent, + provider: sb.provider ?? null, + model: sb.model ?? null, + nimContainer: sb.nimContainer ?? null, + credentialEnv: getRebuildCredentialEnvFromRegistry(sb.provider), + pinEndpoint: !sessionMatchesSandbox && rebuildEndpoint.known, + endpointUrl: rebuildEndpoint.known ? rebuildEndpoint.endpointUrl : null, + ambient, + }; +} + function normalizeHermesRebuildAuthMethod(value: unknown): "oauth" | "api_key" | null { const normalized = String(value || "") .trim() @@ -680,61 +792,13 @@ export async function rebuildSandbox( // the sandbox still intact. See #2273. if (!preflightRebuildCredentials(sandboxName, sb, log, bail)) return; - // #5735: make the recreate config match registry reality *before* any - // destructive backup/delete. A rebuild always recreates the target from its - // recorded agent/provider/model, so surface (and, at recreate time, ignore) - // any ambient onboard-selection env that would otherwise steer the resume - // toward a different agent/provider — e.g. an installer's just-completed - // Deep Agents onboard env bleeding into `upgrade-sandboxes --auto`. - const ambientRecreateEnv = assessAmbientRecreateEnv(rebuildAgent); - if (ambientRecreateEnv.presentVars.length > 0) { - log( - `Ambient onboard-selection env present (${ambientRecreateEnv.presentVars.join(", ")}); will be isolated during recreate so '${sandboxName}' rebuilds from its registry config`, - ); - if (ambientRecreateEnv.agentMismatch) { - console.log( - ` ${D}Ignoring ambient NEMOCLAW_AGENT='${ambientRecreateEnv.agentMismatch.envAgent}' — ` + - `rebuilding '${sandboxName}' as its recorded agent '${ambientRecreateEnv.agentMismatch.registryAgent}'.${R}`, - ); - } - } - - // #5735: when the loaded onboard session belongs to a *different* sandbox - // (e.g. an installer's just-completed onboard before `upgrade-sandboxes - // --auto`), the target's inference endpoint can only be re-derived for - // providers with a canonical endpoint (NVIDIA Endpoints, Anthropic, etc.) or - // local inference. For a custom OpenAI-compatible / router provider the base - // URL lives only in the target's own onboard session — which we don't have — - // so recreating would either fail or silently reconfigure the provider - // against the unrelated session's endpoint. Fail closed *before* any - // destructive backup/delete so the live sandbox stays intact. - const endpointPreflightSession = onboardSession.loadSession(); - if ( - endpointPreflightSession?.sandboxName !== sandboxName && - sb.provider && - !isLocalInferenceProvider(sb.provider) && - sb.provider !== hermesProviderAuth.HERMES_PROVIDER_NAME && - !getRebuildEndpointFromRegistry(sb.provider).known - ) { - console.error(""); - console.error( - ` ${_RD}Rebuild preflight failed:${R} cannot determine the inference endpoint for provider '${sb.provider}'.`, - ); - console.error( - ` The custom endpoint for '${sandboxName}' is recorded only in its own onboard session,`, - ); - console.error( - ` but the current session belongs to '${endpointPreflightSession?.sandboxName ?? "(none)"}'.`, - ); - console.error(` Rebuild '${sandboxName}' directly so its session is loaded:`); - console.error(` ${CLI_NAME} ${sandboxName} rebuild`); - console.error(""); - console.error(" Sandbox is untouched — no data was lost."); - bail( - `Cannot determine recreate endpoint for provider '${sb.provider}' without a matching session`, - ); - return; - } + // #5735 (PRA-6/PRA-9): resolve and validate the entire recreate config — agent, + // provider, model, credential, endpoint — from the registry/session BEFORE any + // destructive backup/delete, and surface/neutralize ambient onboard-selection + // env that would otherwise steer the resume away from the recorded sandbox. + // Fails closed (sandbox untouched) when a precondition cannot be satisfied. + const resumeConfig = prepareRebuildResumeConfig(sandboxName, sb, rebuildAgent, log, bail); + if (!resumeConfig) return; const rebuildMessagingPlan = await stageRebuildMessagingPlanOrBail( sandboxName, @@ -868,25 +932,20 @@ export async function rebuildSandbox( // setupNim runs, leaving no recovery source. Assign explicitly (with a // null fallback) so a missing registry value doesn't silently leave a // stale session entry from an earlier sandbox in place. - s.provider = sb.provider ?? null; - s.model = sb.model ?? null; - s.nimContainer = sb.nimContainer ?? null; - // #5735: pin the credential env name from the target registry provider so - // onboard --resume cannot recreate this sandbox with an unrelated onboard's - // provider credential. - s.credentialEnv = getRebuildCredentialEnvFromRegistry(sb.provider); - // When the loaded session belongs to a *different* sandbox (e.g. the - // installer's just-completed onboard), repin the endpoint from the target - // provider's canonical config so a stale endpoint cannot bleed in. Only do - // this for providers with a canonical/registry-derivable endpoint; for a - // custom OpenAI-compatible provider the base URL exists only in its own - // matching session, so clearing it here would strand the recreate after - // the delete — preserve whatever the session holds instead. - if (!sessionMatchesSandbox) { - const rebuildEndpoint = getRebuildEndpointFromRegistry(sb.provider); - if (rebuildEndpoint.known) { - s.endpointUrl = rebuildEndpoint.endpointUrl; - } + // #5735: apply the recreate config resolved + validated BEFORE delete by + // prepareRebuildResumeConfig (provider/model/credential/endpoint derived + // from the about-to-be-removed registry entry, never from ambient env), so + // onboard --resume recreates the recorded sandbox in non-interactive mode. + // Assign explicitly so a missing value doesn't leave a stale entry from an + // earlier sandbox in place. `pinEndpoint` is false for a matching session + // (keep its own custom endpoint) and true for a non-matching session with a + // canonical/registry-derivable endpoint. + s.provider = resumeConfig.provider; + s.model = resumeConfig.model; + s.nimContainer = resumeConfig.nimContainer; + s.credentialEnv = resumeConfig.credentialEnv; + if (resumeConfig.pinEndpoint) { + s.endpointUrl = resumeConfig.endpointUrl; } return s; }); diff --git a/test/install-upgrade-sandboxes-severity.test.ts b/test/install-upgrade-sandboxes-severity.test.ts index f69a3625627..7ba627b4c58 100644 --- a/test/install-upgrade-sandboxes-severity.test.ts +++ b/test/install-upgrade-sandboxes-severity.test.ts @@ -42,6 +42,39 @@ function runPrintDone(upgradeFailed: boolean): string { return result.stdout; } +// Exercise finalize_install() — print_done() plus the fatal-exit propagation — +// so a failed post-onboard auto-upgrade surfaces a non-zero installer result, +// not a warning-styled success (#5735 PRA-5/PRA-T1). +function runFinalizeInstall(upgradeFailed: boolean): { + status: number | null; + stdout: string; + stderr: string; +} { + const snippet = ` + set -e + source "${INSTALLER_PAYLOAD}" >/dev/null 2>&1 || true + info() { printf 'INFO:%s\\n' "$*"; } + warn() { printf 'WARN:%s\\n' "$*"; } + needs_shell_reload() { return 1; } + resolve_onboarded_agent() { printf 'openclaw'; } + warn_default_agent_fallback() { :; } + print_cli_path_refresh_actions() { :; } + _INSTALL_START=0 + SECONDS=0 + _CLI_DISPLAY="NemoClaw" + _CLI_BIN="nemoclaw" + ONBOARD_RAN=true + NEMOCLAW_READY_NOW=true + _UPGRADE_SANDBOXES_FAILED=${upgradeFailed ? "true" : "false"} + finalize_install + `; + const result = spawnSync("bash", ["-c", snippet], { + encoding: "utf-8", + env: { ...process.env, BASH_ENV: "", ENV: "" }, + }); + return { status: result.status, stdout: result.stdout, stderr: result.stderr }; +} + describe("install.sh print_done — auto-upgrade severity (#5735)", () => { it("prints a clean completion banner when no sandbox upgrade failed", () => { const out = runPrintDone(false); @@ -61,3 +94,22 @@ describe("install.sh print_done — auto-upgrade severity (#5735)", () => { expect(out).toContain("rebuild"); }); }); + +describe("install.sh finalize_install — fatal exit on failed auto-upgrade (#5735 PRA-5)", () => { + it("exits zero and prints the clean banner when no upgrade failed", () => { + const result = runFinalizeInstall(false); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("=== Installation complete ==="); + }); + + it("exits non-zero while still printing the recovery guidance when an upgrade failed", () => { + const result = runFinalizeInstall(true); + // Fatal: automation/operators must not treat this as a successful install. + expect(result.status).not.toBe(0); + // Recovery guidance from print_done is still shown before the fatal exit. + expect(result.stdout).toContain("Installation completed with warnings"); + expect(result.stdout).toContain("Existing sandbox upgrade did not finish"); + // The fatal error line is surfaced (error() writes to stderr). + expect(result.stderr).toContain("Installation incomplete"); + }); +}); From 2b3ff51dc9aaf4811f7f94b2ee526a047518fd3d Mon Sep 17 00:00:00 2001 From: Yimo Jiang Date: Thu, 25 Jun 2026 15:16:00 +0000 Subject: [PATCH 4/4] refactor(rebuild): extract rebuild-resume-config module + env-isolation contract (#5735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../sandbox/rebuild-env-isolation.test.ts | 17 ++ .../actions/sandbox/rebuild-env-isolation.ts | 15 ++ src/lib/actions/sandbox/rebuild-flow.test.ts | 56 +++++ .../sandbox/rebuild-resume-config.test.ts | 135 ++++++++++++ .../actions/sandbox/rebuild-resume-config.ts | 204 ++++++++++++++++++ src/lib/actions/sandbox/rebuild.ts | 193 +---------------- 6 files changed, 436 insertions(+), 184 deletions(-) create mode 100644 src/lib/actions/sandbox/rebuild-resume-config.test.ts create mode 100644 src/lib/actions/sandbox/rebuild-resume-config.ts diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.test.ts b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts index aacf22de01e..6228c17bf2b 100644 --- a/src/lib/actions/sandbox/rebuild-env-isolation.test.ts +++ b/src/lib/actions/sandbox/rebuild-env-isolation.test.ts @@ -34,6 +34,23 @@ describe("sanitizeEnvValueForDisplay (#5735 PRA-7)", () => { }); }); +describe("AMBIENT_RECREATE_ENV_VARS contract (#5735 PRA-4)", () => { + it("pins the exact onboard-selection env set the recreate must isolate", () => { + // Mirrors the ambient selection env vars `onboard --resume` reads at its + // source boundary. Adding a new onboard-selection env var must be a conscious + // change here too, or rebuild recreates could be re-contaminated by an + // unrelated onboard. Keep in sync with the documented source reads in + // rebuild-env-isolation.ts. + expect([...AMBIENT_RECREATE_ENV_VARS]).toEqual([ + "NEMOCLAW_AGENT", + "NEMOCLAW_PROVIDER", + "NEMOCLAW_PROVIDER_KEY", + "NEMOCLAW_ENDPOINT_URL", + "NEMOCLAW_MODEL", + ]); + }); +}); + describe("assessAmbientRecreateEnv", () => { it("reports no contamination when no ambient onboard env is set", () => { const result = assessAmbientRecreateEnv("openclaw", {}); diff --git a/src/lib/actions/sandbox/rebuild-env-isolation.ts b/src/lib/actions/sandbox/rebuild-env-isolation.ts index 8cc1d572996..4c3819c1a8a 100644 --- a/src/lib/actions/sandbox/rebuild-env-isolation.ts +++ b/src/lib/actions/sandbox/rebuild-env-isolation.ts @@ -9,6 +9,21 @@ // vars that onboard's resume path reads to pick the agent, provider, model, // endpoint, and credential — isolating them during the recreate forces the // pinned session + gateway-registered provider to win. +// +// SOURCE-OF-TRUTH NOTE (#5735, PRA-4): the real source boundary is +// `onboard --resume`, which still reads these from the global `process.env`: +// - NEMOCLAW_AGENT → src/lib/agent/defs.ts resolveAgentName() +// - NEMOCLAW_PROVIDER → src/lib/onboard/providers.ts getNonInteractiveProvider() +// - NEMOCLAW_PROVIDER_KEY → src/lib/onboard/provider-key-bridge.ts / providers.ts +// - NEMOCLAW_ENDPOINT_URL → src/lib/onboard.ts (remote endpoint override) +// - NEMOCLAW_MODEL → src/lib/onboard.ts (model override) +// This list MUST stay in sync with those reads; a contract test in +// rebuild-env-isolation.test.ts pins the exact set so adding a new +// onboard-selection env var forces a conscious update here. +// REMOVAL CONDITION: delete this isolation once `onboard --resume` accepts an +// explicit registry-derived recreate config (or a constrained env map) and +// stops consulting ambient selection env for rebuild recreates — then the +// source boundary enforces the invariant and this wrapper is redundant. export const AMBIENT_RECREATE_ENV_VARS = [ "NEMOCLAW_AGENT", "NEMOCLAW_PROVIDER", diff --git a/src/lib/actions/sandbox/rebuild-flow.test.ts b/src/lib/actions/sandbox/rebuild-flow.test.ts index 5e065b859d6..4025f83303a 100644 --- a/src/lib/actions/sandbox/rebuild-flow.test.ts +++ b/src/lib/actions/sandbox/rebuild-flow.test.ts @@ -553,6 +553,62 @@ describe("rebuildSandbox flow", () => { } }); + it("recreates a matching-session custom-endpoint sandbox from session, ignoring hostile ambient endpoint/provider/model (#5735 PRA-4)", async () => { + // Matching session (sandboxName === target) with a custom endpoint recorded + // in that session. Hostile ambient NEMOCLAW_ENDPOINT_URL/PROVIDER/MODEL must + // be absent during recreate (so onboard --resume uses the session) and the + // session's own recorded endpoint must be preserved (not overwritten). + const restoreEnv = snapshotEnv([ + "NEMOCLAW_ENDPOINT_URL", + "NEMOCLAW_PROVIDER", + "NEMOCLAW_MODEL", + "COMPATIBLE_API_KEY", + ]); + process.env.NEMOCLAW_ENDPOINT_URL = "https://attacker.example.test/v1"; + process.env.NEMOCLAW_PROVIDER = "build"; + process.env.NEMOCLAW_MODEL = "attacker-model"; + process.env.COMPATIBLE_API_KEY = "compat-key"; // pass credential preflight + + let envSeenInsideOnboard: Record | null = null; + try { + const harness = createRebuildFlowHarness({ + applyPreset: () => true, + sandboxEntry: { provider: "compatible-endpoint", model: "session-model" }, + onboard: () => { + envSeenInsideOnboard = { + endpoint: process.env.NEMOCLAW_ENDPOINT_URL, + provider: process.env.NEMOCLAW_PROVIDER, + model: process.env.NEMOCLAW_MODEL, + }; + }, + }); + // The custom endpoint lives only in this sandbox's own (matching) session. + harness.session.endpointUrl = "https://my-custom-endpoint.example/v1"; + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).resolves.toBeUndefined(); + + // Ambient selection env was isolated during the recreate. + expect(envSeenInsideOnboard).toEqual({ + endpoint: undefined, + provider: undefined, + model: undefined, + }); + // The matching session's own recorded endpoint is preserved (not pinned/overwritten). + expect(harness.session.endpointUrl).toBe("https://my-custom-endpoint.example/v1"); + // Provider/model come from the registry entry, not the ambient values. + expect(harness.session.provider).toBe("compatible-endpoint"); + expect(harness.session.model).toBe("session-model"); + // Caller env restored afterward. + expect(process.env.NEMOCLAW_ENDPOINT_URL).toBe("https://attacker.example.test/v1"); + expect(process.env.NEMOCLAW_PROVIDER).toBe("build"); + expect(process.env.NEMOCLAW_MODEL).toBe("attacker-model"); + } finally { + restoreEnv(); + } + }); + it("aborts before backup/delete when a custom-endpoint target has no matching session (#5735)", async () => { // Installer flow: the loaded onboard session belongs to a different // (just-created) sandbox, and the target uses a custom OpenAI-compatible diff --git a/src/lib/actions/sandbox/rebuild-resume-config.test.ts b/src/lib/actions/sandbox/rebuild-resume-config.test.ts new file mode 100644 index 00000000000..256477cf5b5 --- /dev/null +++ b/src/lib/actions/sandbox/rebuild-resume-config.test.ts @@ -0,0 +1,135 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { createRequire } from "node:module"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +const requireDist = createRequire(import.meta.url); +const onboardSession = requireDist("../../../../dist/lib/state/onboard-session.js"); +const { + isLocalInferenceProvider, + getRebuildCredentialEnvFromRegistry, + getRebuildEndpointFromRegistry, + prepareRebuildResumeConfig, +} = requireDist("../../../../dist/lib/actions/sandbox/rebuild-resume-config.js"); + +const noopLog = () => undefined; +const throwingBail = (msg: string): never => { + throw new Error(msg); +}; + +function entry(overrides: Record = {}) { + return { name: "alpha", provider: null, model: null, nimContainer: null, ...overrides }; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("isLocalInferenceProvider", () => { + it("classifies local providers and rejects remote/null", () => { + expect(isLocalInferenceProvider("ollama-local")).toBe(true); + expect(isLocalInferenceProvider("vllm-local")).toBe(true); + expect(isLocalInferenceProvider("nvidia-prod")).toBe(false); + expect(isLocalInferenceProvider(null)).toBe(false); + }); +}); + +describe("getRebuildCredentialEnvFromRegistry", () => { + it("returns the canonical credential env for a known remote provider", () => { + expect(getRebuildCredentialEnvFromRegistry("nvidia-prod")).toBe("NVIDIA_INFERENCE_API_KEY"); + }); + it("returns null for local and unset providers", () => { + expect(getRebuildCredentialEnvFromRegistry("ollama-local")).toBeNull(); + expect(getRebuildCredentialEnvFromRegistry(null)).toBeNull(); + }); +}); + +describe("getRebuildEndpointFromRegistry", () => { + it("treats local and routed providers as derivable with no pinned URL", () => { + expect(getRebuildEndpointFromRegistry("ollama-local")).toEqual({ + known: true, + endpointUrl: null, + }); + expect(getRebuildEndpointFromRegistry("nvidia-router")).toEqual({ + known: true, + endpointUrl: null, + }); + expect(getRebuildEndpointFromRegistry(null)).toEqual({ known: true, endpointUrl: null }); + }); + + it("pins the canonical endpoint for a known remote provider", () => { + const result = getRebuildEndpointFromRegistry("nvidia-prod"); + expect(result.known).toBe(true); + expect(typeof result.endpointUrl).toBe("string"); + expect(result.endpointUrl.length).toBeGreaterThan(0); + }); + + it("marks a custom OpenAI-compatible provider as unknown (session-only URL)", () => { + expect(getRebuildEndpointFromRegistry("compatible-endpoint")).toEqual({ known: false }); + }); +}); + +describe("prepareRebuildResumeConfig", () => { + it("pins registry config and does not pin endpoint for a matching session", () => { + vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha" }); + const config = prepareRebuildResumeConfig( + "alpha", + entry({ provider: "nvidia-prod", model: "m" }), + null, + noopLog, + throwingBail, + ); + expect(config).toMatchObject({ + provider: "nvidia-prod", + model: "m", + credentialEnv: "NVIDIA_INFERENCE_API_KEY", + pinEndpoint: false, + }); + }); + + it("pins the canonical endpoint when the session belongs to another sandbox", () => { + vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "other" }); + const config = prepareRebuildResumeConfig( + "alpha", + entry({ provider: "nvidia-prod", model: "m" }), + null, + noopLog, + throwingBail, + ); + expect(config?.pinEndpoint).toBe(true); + expect(typeof config?.endpointUrl).toBe("string"); + }); + + it("fails closed for a custom endpoint with a non-matching session", () => { + vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "other" }); + expect(() => + prepareRebuildResumeConfig( + "alpha", + entry({ provider: "compatible-endpoint", model: "m" }), + null, + noopLog, + throwingBail, + ), + ).toThrow("Cannot determine recreate endpoint"); + }); + + it("surfaces an ambient agent mismatch in the assessment", () => { + vi.spyOn(onboardSession, "loadSession").mockReturnValue({ sandboxName: "alpha" }); + const prior = process.env.NEMOCLAW_AGENT; + process.env.NEMOCLAW_AGENT = "langchain-deepagents-code"; + try { + const config = prepareRebuildResumeConfig("alpha", entry(), null, noopLog, throwingBail); + expect(config?.ambient.agentMismatch).toEqual({ + envAgent: "langchain-deepagents-code", + registryAgent: "openclaw", + }); + } finally { + // Branchless restore of prior worker value (ternary expression, not a + // conditional statement, to keep the changed-test-file guardrail green). + delete process.env.NEMOCLAW_AGENT; + Object.assign(process.env, prior === undefined ? {} : { NEMOCLAW_AGENT: prior }); + } + }); +}); diff --git a/src/lib/actions/sandbox/rebuild-resume-config.ts b/src/lib/actions/sandbox/rebuild-resume-config.ts new file mode 100644 index 00000000000..65472535a3c --- /dev/null +++ b/src/lib/actions/sandbox/rebuild-resume-config.ts @@ -0,0 +1,204 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// #5735: the pre-delete recreate "trust boundary" for `nemoclaw rebuild` +// and installer `upgrade-sandboxes --auto`. Resolves and validates the exact +// agent/provider/model/credential/endpoint a rebuild will re-apply to the +// onboard session so `onboard --resume` recreates the *recorded* sandbox — never +// a different agent/provider steered by an unrelated onboard's ambient selection +// env or global session. Extracted from rebuild.ts so the trust-boundary logic +// is auditable on its own (PRA-5). + +import { CLI_NAME } from "../../cli/branding"; +import { RD as _RD, D, R } from "../../cli/terminal-style"; +import * as onboardSession from "../../state/onboard-session"; +import { + type AmbientRecreateEnvAssessment, + assessAmbientRecreateEnv, + sanitizeEnvValueForDisplay, +} from "./rebuild-env-isolation"; +import type { RebuildSandboxEntry } from "./rebuild-flow-helpers"; + +const { LOCAL_INFERENCE_PROVIDERS, REMOTE_PROVIDER_CONFIG } = + require("../../onboard/providers") as { + LOCAL_INFERENCE_PROVIDERS: string[]; + REMOTE_PROVIDER_CONFIG: Record< + string, + { providerName: string; credentialEnv: string | null; endpointUrl?: string | null } + >; + }; +const hermesProviderAuth = require("../../hermes-provider-auth") as { + HERMES_PROVIDER_NAME: string; +}; + +/** Providers that run on the host and carry no host-side credential env. */ +export function isLocalInferenceProvider(provider: string | null | undefined): provider is string { + return Boolean(provider && LOCAL_INFERENCE_PROVIDERS.includes(provider)); +} + +/** Resolve the credential environment variable required to recreate a sandbox. */ +export function getRebuildCredentialEnvFromRegistry( + provider: string | null | undefined, +): string | null { + if (!provider || isLocalInferenceProvider(provider)) { + return null; + } + const remoteConfig = + provider === "nvidia-nim" + ? REMOTE_PROVIDER_CONFIG.build + : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); + return remoteConfig?.credentialEnv || null; +} + +// Providers whose inference base URL is supplied by the operator at onboard time +// (modelMode "input") and recorded only in that sandbox's own onboard session — +// there is no canonical or registry source to re-derive it from during a +// rebuild. These are the only providers for which a non-matching session makes +// the recreate endpoint unrecoverable. (#5735) +const SESSION_ONLY_ENDPOINT_PROVIDER_NAMES = new Set( + [ + REMOTE_PROVIDER_CONFIG.custom?.providerName, + REMOTE_PROVIDER_CONFIG.anthropicCompatible?.providerName, + // Stable fallbacks in case the config keys are renamed. + "compatible-endpoint", + "compatible-anthropic-endpoint", + ].filter((value): value is string => typeof value === "string" && value.length > 0), +); + +/** + * Resolve the authoritative inference endpoint for a sandbox's recorded provider + * during rebuild (#5735). Returns `{ known: true, endpointUrl }` when the + * recreate endpoint can be re-derived without the target's own onboard session — + * a known remote provider with a canonical URL (e.g. nvidia-prod → NVIDIA + * Endpoints), a local or routed (blueprint-derived) provider (no static URL to + * pin), or any other provider that does not record a custom base URL. Returns + * `{ known: false }` only for custom OpenAI/Anthropic-compatible providers whose + * base URL lives solely in their own session — the caller must then refuse to + * destroy the sandbox from an unrelated session rather than guess the endpoint. + */ +export function getRebuildEndpointFromRegistry( + provider: string | null | undefined, +): { known: true; endpointUrl: string | null } | { known: false } { + if (!provider) return { known: true, endpointUrl: null }; + if (isLocalInferenceProvider(provider)) return { known: true, endpointUrl: null }; + // Custom OpenAI/Anthropic-compatible providers carry their base URL only in + // the session; without a matching session it cannot be recovered. + if (SESSION_ONLY_ENDPOINT_PROVIDER_NAMES.has(provider)) return { known: false }; + const remoteConfig = + provider === "nvidia-nim" + ? REMOTE_PROVIDER_CONFIG.build + : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); + // Known remote provider with a canonical endpoint → pin it. Otherwise (routed + // inference, NIM, or any provider without a custom session-only URL) there is + // no static URL to pin; the resume path derives it, so leave it unpinned. + return { known: true, endpointUrl: remoteConfig?.endpointUrl || null }; +} + +/** + * The exact agent/provider/model/credential/endpoint a rebuild will re-apply to + * the onboard session so `onboard --resume` recreates the *recorded* sandbox + * (#5735). Resolved entirely from the registry entry + onboard session — never + * from ambient selection env. + */ +export interface RebuildResumeConfig { + readonly agent: string | null; + readonly provider: string | null; + readonly model: string | null; + readonly nimContainer: string | null; + readonly credentialEnv: string | null; + /** Overwrite the session endpoint with `endpointUrl`; false keeps a matching session's own custom URL. */ + readonly pinEndpoint: boolean; + readonly endpointUrl: string | null; + readonly ambient: AmbientRecreateEnvAssessment; +} + +/** + * Resolve and validate the recreate config BEFORE any destructive backup/delete + * (#5735). This is the single pre-delete trust boundary for the recreate: it + * assesses ambient onboard-selection env, fails closed for a custom endpoint + * whose base URL is only in another sandbox's session, and derives the exact + * provider/model/credential/endpoint that the post-delete session rewrite + + * `onboard --resume` will apply. Returns null (after `bail`) on a failed + * precondition so the live sandbox is left intact. + * + * Why this is the achievable pre-delete validation rather than a literal + * health-before-delete: OpenShell recreates with the SAME sandbox name, so the + * old and new sandbox cannot run side by side — a replacement cannot be brought + * up and verified while the original still exists. The full set of determinable + * recreate preconditions is therefore validated before delete: credential + * availability (`preflightRebuildCredentials`), config resolution + + * custom-endpoint determinability (here), and the agent base image build + * (`ensureRebuildAgentBaseImage`). The residual failure window — a transient + * runtime fault inside `onboard` after all preconditions pass — is covered by + * the preserved state backup and the printed recovery steps. Eliminating that + * window needs an OpenShell capability to build/verify a replacement under a + * temporary name before swapping; tracked as a follow-up. + */ +export function prepareRebuildResumeConfig( + sandboxName: string, + sb: RebuildSandboxEntry, + rebuildAgent: string | null, + log: (msg: string) => void, + bail: (msg: string, code?: number) => never, +): RebuildResumeConfig | null { + const ambient = assessAmbientRecreateEnv(rebuildAgent); + if (ambient.presentVars.length > 0) { + log( + `Ambient onboard-selection env present (${ambient.presentVars.join(", ")}); will be isolated during recreate so '${sandboxName}' rebuilds from its registry config`, + ); + if (ambient.agentMismatch) { + console.log( + ` ${D}Ignoring ambient NEMOCLAW_AGENT='${sanitizeEnvValueForDisplay(ambient.agentMismatch.envAgent)}' — ` + + `rebuilding '${sandboxName}' as its recorded agent '${ambient.agentMismatch.registryAgent}'.${R}`, + ); + } + } + + const session = onboardSession.loadSession(); + const sessionMatchesSandbox = session?.sandboxName === sandboxName; + const rebuildEndpoint = getRebuildEndpointFromRegistry(sb.provider); + + // When the loaded session belongs to a *different* sandbox (e.g. an + // installer's just-completed onboard before `upgrade-sandboxes --auto`), the + // target's inference endpoint can only be re-derived for providers with a + // canonical endpoint (NVIDIA Endpoints, Anthropic, etc.), local inference, or + // routed inference. For a custom OpenAI-compatible provider the base URL lives + // only in the target's own session — which we don't have — so recreating would + // either fail or silently reconfigure against the unrelated session's + // endpoint. Fail closed before any destructive work so the sandbox stays live. + if ( + !sessionMatchesSandbox && + sb.provider && + !isLocalInferenceProvider(sb.provider) && + sb.provider !== hermesProviderAuth.HERMES_PROVIDER_NAME && + !rebuildEndpoint.known + ) { + console.error(""); + console.error( + ` ${_RD}Rebuild preflight failed:${R} cannot determine the inference endpoint for provider '${sb.provider}'.`, + ); + console.error( + ` The custom endpoint for '${sandboxName}' is recorded only in its own onboard session,`, + ); + console.error(` but the current session belongs to '${session?.sandboxName ?? "(none)"}'.`); + console.error(` Rebuild '${sandboxName}' directly so its session is loaded:`); + console.error(` ${CLI_NAME} ${sandboxName} rebuild`); + console.error(""); + console.error(" Sandbox is untouched — no data was lost."); + bail( + `Cannot determine recreate endpoint for provider '${sb.provider}' without a matching session`, + ); + return null; + } + + return { + agent: rebuildAgent, + provider: sb.provider ?? null, + model: sb.model ?? null, + nimContainer: sb.nimContainer ?? null, + credentialEnv: getRebuildCredentialEnvFromRegistry(sb.provider), + pinEndpoint: !sessionMatchesSandbox && rebuildEndpoint.known, + endpointUrl: rebuildEndpoint.known ? rebuildEndpoint.endpointUrl : null, + ambient, + }; +} diff --git a/src/lib/actions/sandbox/rebuild.ts b/src/lib/actions/sandbox/rebuild.ts index 051b4648cb8..403717b6c95 100644 --- a/src/lib/actions/sandbox/rebuild.ts +++ b/src/lib/actions/sandbox/rebuild.ts @@ -22,15 +22,9 @@ const hermesProviderAuth = require("../../hermes-provider-auth") as { baseUrl?: string, ) => void; }; -const { LOCAL_INFERENCE_PROVIDERS, REMOTE_PROVIDER_CONFIG, providerExistsInGateway } = - require("../../onboard/providers") as { - LOCAL_INFERENCE_PROVIDERS: string[]; - REMOTE_PROVIDER_CONFIG: Record< - string, - { providerName: string; credentialEnv: string | null; endpointUrl?: string | null } - >; - providerExistsInGateway: (name: string, runOpenshellFn: typeof runOpenshell) => boolean; - }; +const { providerExistsInGateway } = require("../../onboard/providers") as { + providerExistsInGateway: (name: string, runOpenshellFn: typeof runOpenshell) => boolean; +}; import { detectOpenShellStateRpcPreflightIssue, @@ -77,12 +71,7 @@ import { import { removeSandboxRegistryEntry } from "./destroy"; import { ensureMessagingHostForwardAfterRebuild } from "./messaging-host-forward-lifecycle"; import { executeSandboxCommand } from "./process-recovery"; -import { - type AmbientRecreateEnvAssessment, - assessAmbientRecreateEnv, - isolateAmbientRecreateEnv, - sanitizeEnvValueForDisplay, -} from "./rebuild-env-isolation"; +import { isolateAmbientRecreateEnv } from "./rebuild-env-isolation"; import { backupSandboxStateForRebuild, ensureRebuildAgentBaseImage, @@ -91,6 +80,11 @@ import { resolveRebuildLiveState, } from "./rebuild-flow-helpers"; import { buildRebuildRecreateOnboardOpts } from "./rebuild-gpu-opt-out"; +import { + getRebuildCredentialEnvFromRegistry, + isLocalInferenceProvider, + prepareRebuildResumeConfig, +} from "./rebuild-resume-config"; import { printRebuildShieldsRecovery, relockRebuildShieldsWindow } from "./rebuild-shields"; export function buildRefreshMutableOpenClawConfigHashCommand( @@ -137,175 +131,6 @@ function _rebuildLog(msg: string) { console.error(` ${D}[rebuild ${new Date().toISOString()}] ${redact(msg)}${R}`); } -/** - * Resolve the credential environment variable required to recreate a sandbox. - */ -function isLocalInferenceProvider(provider: string | null | undefined): provider is string { - return Boolean(provider && LOCAL_INFERENCE_PROVIDERS.includes(provider)); -} - -function getRebuildCredentialEnvFromRegistry(provider: string | null | undefined): string | null { - if (!provider || isLocalInferenceProvider(provider)) { - return null; - } - const remoteConfig = - provider === "nvidia-nim" - ? REMOTE_PROVIDER_CONFIG.build - : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); - return remoteConfig?.credentialEnv || null; -} - -// Providers whose inference base URL is supplied by the operator at onboard time -// (modelMode "input") and recorded only in that sandbox's own onboard session — -// there is no canonical or registry source to re-derive it from during a -// rebuild. These are the only providers for which a non-matching session makes -// the recreate endpoint unrecoverable. (#5735) -const SESSION_ONLY_ENDPOINT_PROVIDER_NAMES = new Set( - [ - REMOTE_PROVIDER_CONFIG.custom?.providerName, - REMOTE_PROVIDER_CONFIG.anthropicCompatible?.providerName, - // Stable fallbacks in case the config keys are renamed. - "compatible-endpoint", - "compatible-anthropic-endpoint", - ].filter((value): value is string => typeof value === "string" && value.length > 0), -); - -/** - * Resolve the authoritative inference endpoint for a sandbox's recorded provider - * during rebuild (#5735). Returns `{ known: true, endpointUrl }` when the - * recreate endpoint can be re-derived without the target's own onboard session — - * a known remote provider with a canonical URL (e.g. nvidia-prod → NVIDIA - * Endpoints), a local or routed (blueprint-derived) provider (no static URL to - * pin), or any other provider that does not record a custom base URL. Returns - * `{ known: false }` only for custom OpenAI/Anthropic-compatible providers whose - * base URL lives solely in their own session — the caller must then refuse to - * destroy the sandbox from an unrelated session rather than guess the endpoint. - */ -function getRebuildEndpointFromRegistry( - provider: string | null | undefined, -): { known: true; endpointUrl: string | null } | { known: false } { - if (!provider) return { known: true, endpointUrl: null }; - if (isLocalInferenceProvider(provider)) return { known: true, endpointUrl: null }; - // Custom OpenAI/Anthropic-compatible providers carry their base URL only in - // the session; without a matching session it cannot be recovered. - if (SESSION_ONLY_ENDPOINT_PROVIDER_NAMES.has(provider)) return { known: false }; - const remoteConfig = - provider === "nvidia-nim" - ? REMOTE_PROVIDER_CONFIG.build - : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); - // Known remote provider with a canonical endpoint → pin it. Otherwise (routed - // inference, NIM, or any provider without a custom session-only URL) there is - // no static URL to pin; the resume path derives it, so leave it unpinned. - return { known: true, endpointUrl: remoteConfig?.endpointUrl || null }; -} - -/** - * The exact agent/provider/model/credential/endpoint a rebuild will re-apply to - * the onboard session so `onboard --resume` recreates the *recorded* sandbox - * (#5735). Resolved entirely from the registry entry + onboard session — never - * from ambient selection env. - */ -export interface RebuildResumeConfig { - readonly agent: string | null; - readonly provider: string | null; - readonly model: string | null; - readonly nimContainer: string | null; - readonly credentialEnv: string | null; - /** Overwrite the session endpoint with `endpointUrl`; false keeps a matching session's own custom URL. */ - readonly pinEndpoint: boolean; - readonly endpointUrl: string | null; - readonly ambient: AmbientRecreateEnvAssessment; -} - -/** - * Resolve and validate the recreate config BEFORE any destructive backup/delete - * (#5735 — PRA-6/PRA-9). This is the single pre-delete trust boundary for the - * recreate: it assesses ambient onboard-selection env, fails closed for a custom - * endpoint whose base URL is only in another sandbox's session, and derives the - * exact provider/model/credential/endpoint that the post-delete session rewrite - * + `onboard --resume` will apply. Returns null (after `bail`) on a failed - * precondition so the live sandbox is left intact. - * - * Why this satisfies the auto-upgrade atomicity requirement without a literal - * health-before-delete: OpenShell recreates with the SAME sandbox name, so the - * old and new sandbox cannot run side by side — a replacement cannot be brought - * up and verified while the original still exists. Instead the full set of - * recreate preconditions is validated before delete: credential availability - * (`preflightRebuildCredentials`), config resolution + custom-endpoint - * determinability (here), and the agent base image build - * (`ensureRebuildAgentBaseImage`). The only residual failure window is a - * transient runtime fault inside `onboard`, which is covered by the preserved - * state backup and the printed recovery steps. - */ -export function prepareRebuildResumeConfig( - sandboxName: string, - sb: RebuildSandboxEntry, - rebuildAgent: string | null, - log: (msg: string) => void, - bail: (msg: string, code?: number) => never, -): RebuildResumeConfig | null { - const ambient = assessAmbientRecreateEnv(rebuildAgent); - if (ambient.presentVars.length > 0) { - log( - `Ambient onboard-selection env present (${ambient.presentVars.join(", ")}); will be isolated during recreate so '${sandboxName}' rebuilds from its registry config`, - ); - if (ambient.agentMismatch) { - console.log( - ` ${D}Ignoring ambient NEMOCLAW_AGENT='${sanitizeEnvValueForDisplay(ambient.agentMismatch.envAgent)}' — ` + - `rebuilding '${sandboxName}' as its recorded agent '${ambient.agentMismatch.registryAgent}'.${R}`, - ); - } - } - - const session = onboardSession.loadSession(); - const sessionMatchesSandbox = session?.sandboxName === sandboxName; - const rebuildEndpoint = getRebuildEndpointFromRegistry(sb.provider); - - // When the loaded session belongs to a *different* sandbox (e.g. an - // installer's just-completed onboard before `upgrade-sandboxes --auto`), the - // target's inference endpoint can only be re-derived for providers with a - // canonical endpoint (NVIDIA Endpoints, Anthropic, etc.), local inference, or - // routed inference. For a custom OpenAI-compatible provider the base URL lives - // only in the target's own session — which we don't have — so recreating would - // either fail or silently reconfigure against the unrelated session's - // endpoint. Fail closed before any destructive work so the sandbox stays live. - if ( - !sessionMatchesSandbox && - sb.provider && - !isLocalInferenceProvider(sb.provider) && - sb.provider !== hermesProviderAuth.HERMES_PROVIDER_NAME && - !rebuildEndpoint.known - ) { - console.error(""); - console.error( - ` ${_RD}Rebuild preflight failed:${R} cannot determine the inference endpoint for provider '${sb.provider}'.`, - ); - console.error( - ` The custom endpoint for '${sandboxName}' is recorded only in its own onboard session,`, - ); - console.error(` but the current session belongs to '${session?.sandboxName ?? "(none)"}'.`); - console.error(` Rebuild '${sandboxName}' directly so its session is loaded:`); - console.error(` ${CLI_NAME} ${sandboxName} rebuild`); - console.error(""); - console.error(" Sandbox is untouched — no data was lost."); - bail( - `Cannot determine recreate endpoint for provider '${sb.provider}' without a matching session`, - ); - return null; - } - - return { - agent: rebuildAgent, - provider: sb.provider ?? null, - model: sb.model ?? null, - nimContainer: sb.nimContainer ?? null, - credentialEnv: getRebuildCredentialEnvFromRegistry(sb.provider), - pinEndpoint: !sessionMatchesSandbox && rebuildEndpoint.known, - endpointUrl: rebuildEndpoint.known ? rebuildEndpoint.endpointUrl : null, - ambient, - }; -} - function normalizeHermesRebuildAuthMethod(value: unknown): "oauth" | "api_key" | null { const normalized = String(value || "") .trim()