fix(onboard): reuse the gateway web-search credential on rebuild recreate - #8774
Conversation
…eate `nemoclaw <name> rebuild --yes` destroyed the sandbox during preflight and then failed to recreate it with "Brave Search requires BRAVE_API_KEY ... in non-interactive mode", leaving no running instance. Rebuild resets the onboard session and derives a fresh checkpoint before it calls `onboard --resume`, so `stagedCredentialProviders` is always empty by the time recreate resolves web search. `resolveWebSearchForCreation` required that receipt before it would reuse the credential already registered with the sandbox's OpenShell gateway provider, so its reuse branch was unreachable on every rebuild and control fell through to non-interactive revalidation of a host key that rebuild can never read (#7097). Accept the recreate journal the replacement driver hands off as the equivalent durable-ownership proof: same sandbox name, same target-intent fingerprint, and past the delete boundary. A journal merely resident in the session is not that proof, because nothing binds it to this run. Both proofs stay paired with the unchanged exact live gateway binding check, so neither can reuse a provider bound to anything but this sandbox. This makes the recreate gate agree with the rebuild preflight (`canReuseGatewayWebSearchCredential`) and `messaging-prep`, which already accept the live binding alone, and with the documented contract in docs/manage-sandboxes/recover-rebuild-sandboxes.mdx. Closes #8717 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
📝 WalkthroughWalkthroughSandbox recreation now validates gateway web-search credential ownership through staged providers or eligible recreate journals. The change documents the contract and adds tests for matching, mismatched, incomplete, absent, and unowned journal conditions. ChangesSandbox web-search credential reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 27f3c92 in the TypeScript / code-coverage/cliThe overall coverage in commit 27f3c92 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Security reviewVerdict: PASS — no findings. I reviewed branch revision
The change is moderate at 279 additions and 4 deletions, so no large-LOC flag applies. The contributor commit is signed off and GitHub marks it Verified. |
|
CI disposition for branch revision
The PR-owned validation remains green: 10 focused regression cases, all 376 onboarding-handler tests, CLI type-checking, formatting, and documentation review pass. |
cv
left a comment
There was a problem hiding this comment.
The rebuild recreate path now reuses a web-search credential only when the driver-provided journal matches the sandbox and target intent, has reached deletion, and the live gateway binding matches exactly. Host credentials still take precedence. Focused tests cover the allowed path and each new rejection condition.
Security review: PASSReviewed the complete PR diff through branch revision Review checklist
Verification
No security findings or security-review blocker remain for this branch revision. |
Problem
nemoclaw <name> rebuild --yesdestroys the sandbox during preflight and then fails to recreate it:Exit code 1, sandbox left destroyed. Reported on Ubuntu 24.04 / v0.0.105; the code is platform-independent.
Root cause
resolveWebSearchForCreationrequired two proofs before reusing the credential already registered with the sandbox's OpenShell gateway provider: a session-scoped staged receipt and an exact live gateway binding match.The first is structurally unobtainable on rebuild:
runRebuildRecreatePhaseoverwrites the session withcreateSession({...})and rebuilds the checkpoint fromderiveCheckpointFromSessionbefore callingonboard --resume(rebuild-recreate-phase.ts:151-210), sostagedCredentialProvidersis[]and the checkpoint bindings are empty.registerCompletedCredentialProvidersat sandbox.ts:1930/1943 — after the read at:1892.So the reuse branch was unreachable on every rebuild, and control fell through to
ensureValidatedWebSearchCredential, which throws undernonInteractive(web-search-flow.ts:283-288) for a host key rebuild can never read.This is the residual half of #7097. That change taught the preflight to reuse the gateway binding (
canReuseGatewayWebSearchCredential) on the stated premise that "recreate reuses that gateway binding … so the preflight accepts the same binding instead of demanding a host key the recreate will never read." That premise holds for the rest of recreate but not for this one gate — which is why preflight destroys the sandbox and recreate then refuses.It also contradicts the shipped contract in recover-rebuild-sandboxes.mdx:216, which promises reuse on provider name + type + credential key with no receipt clause, and matches
messaging-prep.ts:101-110, which already accepts the live binding alone for this same provider.Fix
Accept the recreate journal the replacement driver hands off as the equivalent durable-ownership proof, in a new
ownsGatewayWebSearchProvider: the run must have been handed the journal (recreateJournalTargetIntentFingerprint, set only by rebuild-recreate-phase.ts:271), and that journal must name this sandbox, carry the same target-intent fingerprint, and be past the delete boundary.A journal merely resident in the session is deliberately not accepted — nothing binds it to this run: one survives a failed attempt, and
beginSandboxRecreateTransactionopens one straight atdeletedwhen the sandbox is already missing.The live gateway binding check is unchanged and still ANDed, so neither proof can reuse a provider bound to anything but this sandbox on this gateway. A host credential, when present, still takes precedence over both.
Nothing downstream needed to change:
preflightRequiredCredentialProviderBindingsaccepts the surviving matching provider with no planned token, the token-less def is filtered before upsert, andallRequiredBindingsLiveis satisfied by the live binding.Scope
src/lib/onboard/machine/handlers/sandbox.ts— the gate and its rationale.src/lib/onboard/lifecycle-contracts.md— record the alternative ownership proof and why a resident journal is not one.src/lib/onboard/machine/handlers/sandbox-rebuild-web-search-reuse.test.ts— new focused file, 10 cases.Not in scope: the same shape exists on the messaging side, in the
credentialNeedsValidationpredicate inselectionFromCompletedMessagingCheckpoint(sandbox-messaging.ts:353). It is not reachable on the rebuild pass itself —messagingDecisiongates onsandboxPromptProgress.messaging, which rebuild's session reset leaves false, soreconcileSandboxMessagingreturns the registry-authoritative plan first. But that pass persists the state that detonates later:checkpointMessaging(sandbox.ts:1286) setsmessaging = trueunconditionally while nothing is staged without a host token, so a rebuild that fails after that point leavesmessaging=completedwithstagedCredentialProviders=[], and the recovery command rebuild prints (onboard --resume --name <sb>, rebuild-mcp-phase.ts:145) then throws at messaging-channel-setup.ts:140-146.That is deliberately left out of this PR: open PR #7853 already rewrites that predicate and disarms it on exactly those routes, so fixing it here would collide. Noted in a comment on #7853 so the behaviour survives if that PR is narrowed. Traced by code inspection, not reproduced at runtime.
Maintainer Reviews
Documentation Writer Review
no-docs-neededdocs/manage-sandboxes/recover-rebuild-sandboxes.mdxalready documents matching gateway-provider credential reuse and failure remediation;npm run docs:sync-agent-variantspassed; documentation build completed with 0 errors and 2 existing Fern warningsVerification
mainand passes with the fix; the other 9 pass on both.deleted, journal naming another sandbox, journal not handed to this run, handed journal with a stale target intent, no journal and no receipt, live binding mismatch (fails closed), host key present. The pre-existing staged-receipt path is pinned unchanged.src/lib/onboard/machine/handlers/— 33 files, 376 tests, all pass.tsc -p tsconfig.src.jsonclean.npm run lint,checks:repository,test-size:check,test:titles:check,test:imports:checkall pass. Biome clean.local-model-profile,setup-nim-flow,command,created-sandbox-finalization,docker-llama-cpp-managed-lifecycleandgateway-restart-hermes-driftare pre-existing — verified byte-identical on unpatchedmain.Verified by unit test and by tracing the full recreate path; I have not run a live
rebuildagainst a Brave-backed sandbox, so QA re-verification on hardware is worth having before this is treated as closed.Closes #8717
Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation