fix(rebuild): recover managed MCP after exec loss#7196
Conversation
Use host-side MCP recovery only when an explicit forced rebuild cannot run a sandbox no-op. Keep policy, ownership, target, provider, and transaction errors fail-closed. Closes #7062 Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughForced rebuilds now probe sandbox execution before MCP teardown and use read-only host-side MCP recovery when the probe is unavailable. Deletion outcomes are reconciled before cleanup, with force propagation, ambiguity handling, validation tests, and updated documentation. ChangesForced MCP rebuild recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildPipeline
participant RebuildDestroyPhase
participant MCPPreparation
participant SandboxExec
participant HostSideRecovery
participant OpenShell
participant NIM
RebuildPipeline->>RebuildDestroyPhase: pass normalized force
RebuildDestroyPhase->>MCPPreparation: prepare MCP for rebuild
MCPPreparation->>SandboxExec: execute pre-mutation no-op
SandboxExec-->>MCPPreparation: success or unavailable
MCPPreparation->>HostSideRecovery: validate and preserve MCP intent
RebuildDestroyPhase->>OpenShell: delete sandbox
OpenShell-->>RebuildDestroyPhase: delete result and gateway state
RebuildDestroyPhase->>NIM: stop after confirmed deletion
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7196.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Exact-head maintainer audit: the implementation and fail-closed unit/integration coverage are clean, required CI and the selected protected E2E lanes pass, and there are no unresolved review threads. The advisor's non-blocking request for a deterministic live exec-loss scenario is valid but separable from this fix. I filed #7198 with explicit credential-redaction and recovery acceptance criteria. Independent review is requested because this PR is authored by the current maintainer account. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Pushed the reviewed #7062 recovery completion as two signed, GitHub-Verified, append-only commits with kagura-agent credited:
The explicit Nonzero deletion now queries the exact recorded gateway with a bounded probe: explicit absence continues, confirmed Ready/Running attempts MCP and temporary-shields restoration, and timeout/partial/other ambiguous results preserve ownership and recovery metadata. Local NIM cleanup starts only after deletion is positively confirmed. Validation on the final tree:
Fresh exact-head CI and advisor checks are now running on |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/mcp-bridge.ts (1)
94-102: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConsolidate the duplicated
McpRebuildPreparationcontract and its two export paths into one canonical source.Both files independently declare the identical
McpRebuildPreparationinterface shape, and both exposeprepareMcpBridgesForExecUnavailableRebuildunder the same name through two different routes (a pass-through wrapper inmcp-bridge.tsvs. a direct re-export inmcp-bridge-rebuild.ts, both ultimately delegating tomcp-bridge-rebuild-exec-unavailable.ts). This PR had to hand-edit the same two optional hooks in two places to keep them in sync — exactly the "duplicate sources of truth" / "forwarding wrappers that add a new layer without retiring the old owner" pattern called out forsrc/lib/{actions,domain,adapters,state}/**.
src/lib/actions/sandbox/mcp-bridge.ts#L94-L102: importMcpRebuildPreparationfrom a single canonical module (e.g.,mcp-bridge-rebuild-exec-unavailable.tsor a shared contracts file) instead of redeclaring it here; drop the local interface.src/lib/actions/sandbox/mcp-bridge.ts#L157-L161: remove this pass-through wrapper, or make it the sole canonical export and havemcp-bridge-rebuild.tsimport from it instead of independently re-exporting the underlying implementation.src/lib/actions/sandbox/mcp-bridge-rebuild.ts#L41-L52: drop the independently-declaredMcpRebuildPreparationinterface (import the canonical one) and pick a single re-export path forprepareMcpBridgesForExecUnavailableRebuildshared withmcp-bridge.tsrather than maintaining a second parallel export.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/mcp-bridge.ts` around lines 94 - 102, The McpRebuildPreparation contract and prepareMcpBridgesForExecUnavailableRebuild export are duplicated across two modules. In src/lib/actions/sandbox/mcp-bridge.ts#L94-L102, remove the local interface and import the canonical type; in src/lib/actions/sandbox/mcp-bridge-rebuild.ts#L41-L52, remove its duplicate interface and use the same import. Consolidate the two export paths so both modules share one canonical prepareMcpBridgesForExecUnavailableRebuild route, removing the unnecessary pass-through wrapper or independent re-export.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/actions/sandbox/mcp-bridge.ts`:
- Around line 94-102: The McpRebuildPreparation contract and
prepareMcpBridgesForExecUnavailableRebuild export are duplicated across two
modules. In src/lib/actions/sandbox/mcp-bridge.ts#L94-L102, remove the local
interface and import the canonical type; in
src/lib/actions/sandbox/mcp-bridge-rebuild.ts#L41-L52, remove its duplicate
interface and use the same import. Consolidate the two export paths so both
modules share one canonical prepareMcpBridgesForExecUnavailableRebuild route,
removing the unnecessary pass-through wrapper or independent re-export.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee87a128-bdd3-48c0-ab0e-57adeef0c435
📒 Files selected for processing (16)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/gateway-state-owning-gateway.test.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/mcp-bridge-policy.test.tssrc/lib/actions/sandbox/mcp-bridge-policy.tssrc/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.tssrc/lib/actions/sandbox/mcp-bridge-rebuild.tssrc/lib/actions/sandbox/mcp-bridge.tssrc/lib/actions/sandbox/rebuild-destroy-phase.test.tssrc/lib/actions/sandbox/rebuild-destroy-phase.tssrc/lib/actions/sandbox/rebuild-mcp-phase.test.tssrc/lib/actions/sandbox/rebuild-mcp-phase.tssrc/lib/actions/sandbox/rebuild-pipeline.tstest/helpers/rebuild-flow-recovery-cases.tstest/mcp-destroy-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/rebuild-mcp-phase.test.ts
|
Pushed the narrow growth-guard follow-up in
Fresh exact-head CI and automated review are now running. |
|
Advisor follow-up on exact head
No code change is justified by the exhausted advisor failure. |
|
Blocking exact-head security review at f1c5643 found two pre-delete gaps. First, generic or gateway-level NotFound output, including signal-terminated partial output, can be treated as positive proof that the sandbox was deleted. Require a completed sandbox-specific absence response tied to the requested sandbox and reject gateway/provider/schema/transport ambiguity. Second, the host-recovery snapshot validates entries individually but not cross-entry credential, provider, and policy uniqueness, so collisions can surface only after destructive deletion. Re-establish those whole-snapshot invariants before delete and add negative tests for both boundaries. I am preparing the narrow fixes now. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer remediation is now pushed at exact head 21695d1.
Validation: rebuild destroy phase 16/16, MCP lifecycle 40/40, CLI type-check, and diff whitespace check all pass. An independent security audit found no remaining blocker. All three commits are signed off, GitHub Verified, append-only, and retain kagura-agent as co-author. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the final transport-symmetry review finding in 5293739. Forced rebuild now proves both SSH-backed and OpenShell sandbox-exec MCP preparation paths before any live mutation; either mismatch selects the fail-closed host-side recovery path, with Docker fallback disabled. Comments now accurately scope gateway recovery/selection and the registry-only final edge check. Validation: independent security re-audit PASS; 61 targeted tests PASS; CLI typecheck PASS; Biome and diff checks PASS. The commit is DCO-signed, original-author credited, and GitHub Verified. Monitoring exact-head CI/advisor feedback now. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
CI follow-up 6340766 linearizes the transport mock flagged by the codebase growth guard; behavior and coverage are unchanged. The 40-test lifecycle file, Biome, and diff checks pass, and the signed co-authored commit is GitHub Verified. Exact-head CI has restarted. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the exact-head advisor blocker in b83a56b. The destructive sandbox delete now resolves the persisted gateway before MCP preparation and passes it explicitly with -g at the delete boundary; recorded-gateway post-delete reconciliation remains unchanged. Added a regression with ambient OPENSHELL_GATEWAY=nemoclaw-29080 and persisted gateway nemoclaw-19080, proving deletion targets only nemoclaw-19080. Validation: 17/17 focused tests, CLI typecheck, Biome, git diff check, and an independent security audit all pass. Commit is signed, DCO-compliant, co-authored, and GitHub Verified. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: cli
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Fixed the newly actionable CLI timeout cascade in signed commit 59bac28. The shared rebuild fixtures now model the exact recorded-gateway deletion probe instead of falling through to the full convergence wait. Exact-head local evidence: 66 shared-harness tests, 64 focused CLI tests, and 40 MCP lifecycle integration tests pass; source-shape, Biome, commit hooks, CLI typecheck, and pre-push checks pass. The replacement CI run is now authoritative; the prior failed jobs are superseded. |
Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the exact-head shard-8 failure in signed commit 280c81e. Root cause: the safe default exact-gateway fixture response ran before explicit per-test OpenShell behavior, so the delete-failure regression could not model an intact sandbox. Explicit handled calls now take precedence, while unhandled calls fall through to the safe default. Exact-head local evidence: targeted regressions 3/3 and full rebuild-flow suite 86/86; Biome, source-shape, commit hooks, CLI typecheck, and pre-push gates pass. Prior CI is superseded by the replacement run. |
|
Exact-head E2E retry follow-up for |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head E2E follow-up for |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the exact-head PRA-1 delete-convergence blocker in signed commit 61834f2. The successful-delete convergence path now uses the same strict named-sandbox classifier as nonzero-delete reconciliation and rejects signal-terminated probes, other sandbox names, gateway/provider NotFound output, and mixed diagnostics. Five adversarial cases verify that NIM stop, registry removal, and recreation signaling do not occur; all 26 destroy-phase tests, Biome, CLI build/type-check, and normal commit/push hooks pass. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Follow-up fix-forward: signed commit |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Closes two destructive-delete safety gaps discovered after #7196 merged. Rebuild now rereads the sandbox registry immediately before deletion, verifies the canonical sandbox and recorded gateway binding still match the prepared target, and treats an accepted but unconfirmed deletion as ambiguous so shields are not relocked prematurely. ## Related Issue Follow-up to #7196 and #7062. ## Changes - Reread and resolve the canonical sandbox target and recorded gateway at the delete edge. - Abort deletion, restore MCP state, and relock shields when the sandbox identity or gateway binding drifts after MCP preparation. - Mark an accepted but unconfirmed delete as ambiguous so the outer finally path does not claim a locked posture. - Recognize OpenShell's exact structured sandbox-absence response while leaving gateway, provider, transport, and mixed diagnostics ambiguous. - Add regressions for sandbox-name drift, gateway-binding drift, MCP restoration, ambiguous deletion, and structured absence classification. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This correction restores the delete-edge identity and ambiguous-deletion behavior already documented in the rebuild, MCP, and trusted-boundary guides. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: The correction narrows the destructive action by revalidating the exact target and gateway at the delete edge, fails closed on drift, restores MCP before relocking, and only classifies the exact structured sandbox-not-found response as absence. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Existing canonical docs already require delete-edge target/gateway revalidation and ambiguous-deletion handling; the structured response classifier is an internal compatibility correction that does not change commands, configuration, supported surfaces, or operator guidance. - Agent: Codex Desktop <!-- docs-review-head-sha: a3d5882 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable — normal hooks passed for the maintainer follow-up commit; the original exact head passed `npm run check:diff`. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 46 focused CLI tests passed for the current exact head; CLI typecheck, build, source-shape, test-title, project-membership, and formatting checks passed. The original change also passed its 88 changed tests and 52 adjacent integration tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable; the correction is isolated to the rebuild destroy phase and its focused tests. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented sandbox deletion when gateway/registry details change during rebuild preparation by re-checking the delete target right before execution. * Improved gateway pinning so the delete targets the intended gateway even if ambient gateway selection shifts. * Treated deletion as ambiguous when transport errors block confirmation, triggering the ambiguous-delete flow rather than “deleted” handling. * Expanded handling of structured “sandbox not found” responses for destructive reconciliation. * **Tests** * Added/extended coverage for gateway mismatch scenarios, ambiguous deletion behavior, and structured absence variants. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
3 participants
Summary
An explicit
rebuild --forcecan now preserve registered managed MCP intent when the old sandbox cannot execute a pre-mutation no-op. The recovery does not reinterpret arbitrary live MCP preparation failures as exec-relay loss, and it preserves the original contribution from @kagura-agent with co-author credit.Related Issue
Closes #7062.
Supersedes #7195 because its published commit lacks the required DCO sign-off and cannot be repaired append-only.
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-needede92c7f060896fe3ea5fb84cc4876ddba26c8fcf9, then independently reviewed the exact follow-up delta through768e87c20a6f676e51bf6890884d724d76565a3a. The follow-up changes only three integration-test diagnostics to name sandboxmy-assistant, aligning intended success fixtures with the already-documented strict delete-convergence rule. No command, flag, output contract, recovery step, runtime behavior, or user action changed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable768e87c20a6f676e51bf6890884d724d76565a3a: 12 remaining integration fixture tests, 26 owning destroy-phase tests, and 96 earlier affected fixture-consumer tests passed; Biome, the CLI build, CLI type-check, normal commit hooks, and normal push hooks passed. The earlier branch head retained its broader 90 owning rebuild-flow, 32 adjacent CLI, and 45 MCP lifecycle integration test evidence.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable to this narrow branch; exact-head PR CI will run the repository gates.npm run docsbuilds without warnings (doc changes only) — the prior head's documentation validation passed with 0 errors and 2 Fern warnings; the exact-head follow-up changes no documentation.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Documentation
rebuild --forcemanaged-MCP warning with clearer guidance on when recovery falls back to a host-side, fail-closed path, plus safer post-delete verification and reconciliation outcomes.Bug Fixes
Tests
forcebehavior through the rebuild destroy phase.