fix(rebuild): fall back to host-side MCP recovery when --force and exec relay unavailable - #7195
Closed
kagura-agent wants to merge 1 commit into
Closed
Conversation
…ec relay unavailable When a sandbox reports Ready but its exec relay is broken, `rebuild --force` now falls back to prepareMcpBridgesForAbsentSandboxRebuild instead of bailing. This provides a recovery path for registered-but-unreachable sandboxes with managed MCP state. Closes NVIDIA#7062
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe rebuild pipeline now propagates ChangesForced MCP recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant rebuildPipeline as rebuild-pipeline
participant rebuildDestroyPhase as rebuild-destroy-phase
participant prepareMcpForRebuild
participant hostRecovery as prepareMcpBridgesForAbsentSandboxRebuild
rebuildPipeline->>rebuildDestroyPhase: pass normalized.force
rebuildDestroyPhase->>prepareMcpForRebuild: pass force
prepareMcpForRebuild->>hostRecovery: attempt host-side recovery after error
hostRecovery-->>prepareMcpForRebuild: recovery result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review 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
Blockers
|
22 tasks
Collaborator
|
Superseded by #7196. The replacement preserves this implementation with explicit Co-authored-by credit to @kagura-agent, narrows host-side recovery to a failed pre-mutation sandbox no-op, adds fail-closed regressions and docs, and provides a compliant signed DCO history. Closing this duplicate so review and CI converge on the replacement. |
Contributor
Author
|
Thanks for the credit and the improved approach — the pre-mutation exec probe is much cleaner than my catch-all fallback. Noted the DCO sign-off requirement for future PRs. |
2 tasks
cv
pushed a commit
that referenced
this pull request
Jul 26, 2026
## Summary An explicit `rebuild --force` can 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 - Thread the normalized force choice through the rebuild destroy phase. - Probe sandbox execution before any MCP teardown mutation. - Use the existing host-side absent-sandbox recovery only when that probe fails under explicit force. - Keep policy drift, ambiguous ownership, invalid targets, provider failures, and errors after a successful probe fail-closed. - Require delete convergence to prove explicit absence for the named sandbox; reject other-resource, mixed, and signal-terminated diagnostics. - Document the data-loss and managed-MCP recovery boundary in the command reference and rebuild recovery guide. - Cover force propagation, unavailable execution, live-path safety failures, ambiguous host-side state, non-force behavior, and established stale recovery. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: maintainer review narrowed recovery to a pre-mutation no-op failure and added fail-closed regressions. - [ ] 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: The writer reviewed the cumulative behavior through `e92c7f060896fe3ea5fb84cc4876ddba26c8fcf9`, then independently reviewed the exact follow-up delta through `768e87c20a6f676e51bf6890884d724d76565a3a`. The follow-up changes only three integration-test diagnostics to name sandbox `my-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. - Agent: Codex Desktop <!-- docs-review-head-sha: 768e87c --> <!-- docs-review-agents-blob-sha: be20a09 --> ## 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — exact head `768e87c20a6f676e51bf6890884d724d76565a3a`: 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. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this narrow branch; exact-head PR CI will run the repository gates. - [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) — the prior head's documentation validation passed with 0 errors and 2 Fern warnings; the exact-head follow-up changes no documentation. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded the `rebuild --force` managed-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** * Improved forced managed-MCP rebuild safety by probing in-sandbox when enabled, using stricter read-only ownership/policy checks when exec is unavailable, handling ambiguous delete states correctly, and delaying local NIM stopping until after deletion is confirmed. * **Tests** * Added broader coverage for forced rebuild MCP prep, exec-unavailable recovery, policy/ownership drift detection, and correct `force` behavior through the rebuild destroy phase. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a registered sandbox with managed MCP state has its exec relay permanently unavailable (sandbox reports
Readybutopenshell sandbox execfails),rebuild --forcecannot recover becauseprepareMcpForRebuildstill requires the live exec relay to scrub the MCP adapter before deletion.This leaves all supported recovery paths blocked:
rebuild --yes— cannot back up through the failed relayrebuild --force— cannot scrub the managed MCP adapter through the failed relayonboard --recreate-sandbox— refuses a sandbox that owns managed MCP serversmcp remove --force— cannot prove complete cleanupCloses #7062
Solution
Thread the
forceflag from the rebuild pipeline intoprepareMcpForRebuild. When--forceis set and the live MCP bridge preparation fails (because the exec relay is unavailable), fall back toprepareMcpBridgesForAbsentSandboxRebuild— the host-side-only recovery path that doesn't require sandbox exec.This follows the existing pattern where
--forcealready skips backup failures, extending it to also recover from exec relay failures during MCP preparation.Changes
rebuild-mcp-phase.tsforce: booleanparam; catch live-path failure and fall back to absent-sandbox path whenforce=truerebuild-destroy-phase.tsforce?: booleantoRebuildDestroyPhaseInput; thread toprepareMcpForRebuildrebuild-pipeline.tsnormalized.forceintorunRebuildDestroyPhaserebuild-destroy-phase.test.tsforce=trueis threaded toprepareMcpForRebuildTesting
npx vitest run src/lib/actions/sandbox/rebuild-destroy-phase.test.ts— 2/2 passnpx tsc --noEmit— clean (no new errors)Summary by CodeRabbit
New Features
Tests