fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103) by yanyunl1991 · Pull Request #8298 · NVIDIA/NemoClaw

yanyunl1991 · 2026-08-05T04:40:49Z

Summary

Sandbox destruction now returns the existing structured MCP bridge error when bridge preparation or finalization fails. Before this change, the command exposed an uncaught stack trace. This PR repairs the destroy recovery path; #8103 continues to track the separate rebuild authority-drift failure.

Related Issue

Refs #8103

Changes

Catch McpBridgeError before destructive work starts and return its redacted failure result.
Catch McpBridgeError after sandbox deletion and preserve the retry state.
Rethrow other error types and add tests for both MCP bridge failure stages.

Type of Change

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

Tests added or updated for changed behavior
Existing tests cover changed behavior — justification:
Tests not applicable — justification:
Docs updated for user-facing behavior changes
Docs not applicable — justification: Existing sandbox lifecycle documentation already describes pre-delete failure preservation, post-delete retry, registry cleanup after confirmed deletion, and managed MCP ownership checks. This change returns the documented structured error instead of an uncaught stack trace.
Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: The nine-category security review passed with no findings: fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103) #8298 (comment)
Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

Documentation writer subagent reviewed the completed changes
Result: no-docs-needed
Evidence: docs/manage-sandboxes/manage-mcp-servers.mdx already covers MCP preparation, recovery-state preservation, post-deletion cleanup, and rerunning destroy. docs/reference/commands.mdx already covers unreachable gateways and managed MCP ownership. The change adds no command, option, workflow, lifecycle outcome, or recovery action.
Agent: Codex Desktop

DGX Station Hardware Evidence

Tested on DGX Station
Tested commit: Not applicable; scripts/prepare-dgx-station-host.sh is unchanged.
Station profile/scenario: Not applicable.
Result: Not applicable.
Supporting evidence: Not applicable.

Verification

PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project cli src/lib/actions/sandbox/destroy-flow.test.ts passed 27 tests.
Applicable broad gate passed — Fresh GitHub CI and E2E are running for revision 0b1eeb726.
Quality Gates section completed with required justifications or waivers
No secrets, API keys, or credentials committed
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: yanyunl1991 mzhong@nvidia.com

Summary by CodeRabbit

Bug Fixes
- Sandbox destruction now reports structured failures when bridge preparation or finalization encounters a recognized bridge error.
- Preparation failures stop the process before deletion or cleanup.
- Finalization failures preserve the sandbox registry and prevent unnecessary gateway cleanup.
Tests
- Added coverage for bridge errors during sandbox destruction and cleanup.

…ze in destroy path (#8103) `executeSandboxDestroy` called `prepareMcpDestroy` and `finalizeMcpDestroy` with no `McpBridgeError` guard. When a managed MCP server is present and the gateway becomes unreachable, `inspectExactMcpDestroyProvider` throws `McpBridgeError("Could not inspect OpenShell provider…")` which propagated uncaught, crashing `destroy --yes` with a stack trace instead of a clean exit-1 message. The same uncaught escape existed for `finalizeMcpDestroy`'s internal re-throw after post-delete cleanup fails. Both call sites now catch `McpBridgeError` and return `{ ok: false, … }`, letting the existing failure-path rendering in `destroy.ts` surface the error cleanly and exit with the error's own `exitCode`. Signed-off-by: yanyunl1991 <yanyunl@nvidia.com>

coderabbitai · 2026-08-05T04:41:09Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5e1aa9bc-07c1-4b58-97ec-28abc2eb9e34

📥 Commits

Reviewing files that changed from the base of the PR and between 4f0f7cc and 331c168.

📒 Files selected for processing (1)

ci/source-architecture-budget.json

📝 Walkthrough

Walkthrough

Changes

Sandbox destroy MCP errors

Layer / File(s)	Summary
Handle MCP bridge failures `src/lib/actions/sandbox/destroy-execution.ts`	`executeSandboxDestroy` returns structured failure results for `McpBridgeError` during preparation and finalization. Other errors continue to propagate.
Validate destroy failure paths `test/helpers/destroy-flow-test-harness.ts`, `test/helpers/destroy-flow-test-assertions.ts`, `src/lib/actions/sandbox/destroy-flow.test.ts`	The test harness injects preparation and finalization bridge errors. Tests verify exit codes and skipped cleanup operations.

Architecture budget updates

Layer / File(s)	Summary
Update architecture thresholds `ci/source-architecture-budget.json`	The configuration lowers fan-in, fan-out, and maximum root-file thresholds for selected source paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: catching McpBridgeError in the sandbox destroy MCP bridge path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/mcp-bridge-destroy-error-8103

_{Comment @coderabbitai help to get the list of available commands.}

github-code-quality · 2026-08-05T04:41:31Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 331c168 in the fix/mcp-bridge-destr... branch remains at 96%, unchanged from commit 3a39ff3 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 331c168 in the fix/mcp-bridge-destr... branch remains at 81%, unchanged from commit 89e17bf in the main branch.

Show a code coverage summary of the most impacted files.

File	`main` 89e17bf	`fix/mcp-bridge-destr...` 331c168	+/-
`src/lib/shields/index.ts`	74%	73%	-1%
`src/lib/onboard.ts`	31%	31%	0%
`src/lib/inferen...ving/catalog.ts`	88%	88%	0%
`src/lib/inferen...er-discovery.ts`	87%	88%	+1%
`src/lib/actions...oy-execution.ts`	89%	92%	+3%
`src/lib/inferen...f-cache-plan.ts`	0%	78%	+78%
`src/lib/onboard...ization-deps.ts`	0%	83%	+83%
`src/lib/inferen...ache-receipt.ts`	0%	97%	+97%
`src/lib/onboard...e-port-reuse.ts`	0%	100%	+100%
`src/lib/onboard...-composition.ts`	0%	100%	+100%

_{Updated August 05, 2026 08:56 UTC}

github-actions · 2026-08-05T04:45:33Z

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

1 semantic terminology decision

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

replace — MCP bridge at src/lib/actions/sandbox/destroy-flow.test.ts:377: Use “agent adapter” in explanatory test titles. Preserve literal identifiers such as McpBridgeError.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: onboard-repair, onboard-resume

2 optional E2E recommendations

sandbox-operations
mcp-bridge

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

apurvvkumaria · 2026-08-05T07:56:08Z

Security review: PASS — no findings

Reviewed revision 4f0f7ccd0731 against current main across the repository’s nine security categories.

Secrets and credentials: no credential access or persistence changes; returned MCP errors continue through redactDestroyError.
Input handling: no new user-controlled parsing, interpolation, or command construction.
Authorization and ownership: existing exact MCP provider ownership checks remain intact and still fail closed.
Dependencies and supply chain: no dependency, workflow, or build-input changes.
Logging and error disclosure: replaces uncaught stack-trace behavior with the existing redacted structured failure path; no new sensitive output.
Cryptography: no cryptographic behavior changes.
Configuration and deployment: no policy, network, image, or runtime configuration changes.
Security testing: the focused destroy-flow suite passes all 27 tests, including the new prepare and finalize failure cases; hosted CodeQL, CodeRabbit, ordinary CI, and E2E are green.
Lifecycle safety: prepare failures return before destructive work; finalize failures occur only after confirmed sandbox deletion and preserve registry cleanup state for retry; non-McpBridgeError exceptions still propagate.

The remaining failed Nemotron advisor lane is a model-protocol failure after a partial second-opinion review. The primary advisor completed with high confidence and published merge_as_is with no required follow-up.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

apurvvkumaria · 2026-08-05T08:24:03Z

Security review — PASS

Reviewed revision 0b1eeb726 against main revision 89e17bfe9. The current comparison contains the same four implementation and test files as the prior review. The current-main merge introduced no additional PR behavior.

No security findings were identified.

Category	Verdict	Evidence
Secrets and credentials	PASS	The change adds no secret, credential source, or credential log. Returned messages pass through the existing full redaction function.
Input validation and sanitization	PASS	The change adds no new input parser. It handles the existing typed `McpBridgeError` boundary.
Authentication and authorization	PASS	Existing managed MCP ownership checks remain unchanged. Preparation failures stop before sandbox deletion.
Dependencies	PASS	The change adds no dependency or package source.
Error handling and logging	PASS	Typed bridge failures now return the structured redacted failure instead of an uncaught stack trace. Other error types still propagate.
Cryptography and data protection	PASS	Not applicable; the change adds no cryptographic operation or data format.
Configuration and secure defaults	PASS	The change adds no configuration, network exposure, container permission, or security-header behavior.
Security testing	PASS	Tests prove that preparation failure prevents deletion and that finalization failure preserves registry and gateway cleanup state.
Holistic security posture	PASS	The change preserves fail-closed ownership behavior before deletion and preserves retry authority after deletion.

Validation:

destroy-flow.test.ts: 27 tests passed.
Plugin build, CLI build, and CLI type-check passed.
Diff checks passed.
The merge commit is signed and GitHub reports it as verified.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

copy-pr-bot · 2026-08-05T08:44:52Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

cv · 2026-08-05T09:07:19Z

Compliance hold: do not merge this PR. Commit 331c168f4854b5944605f97fc76621f719d93466 is signed off but GitHub reports it as unverified, so the published history cannot satisfy the repository commit-verification gate. A fresh replacement branch preserves the original contributor commit unchanged and uses verified commits only; its required broad validation is still running. I will link the replacement PR here before closing this one as superseded.

apurvvkumaria · 2026-08-05T09:20:29Z

Replacement: #8313

Evidence that #8313 supersedes this PR:

Both branches apply the same effective five-file change to the same main revision. Git reports the same stable patch comparison for both branches.
The replacement preserves Yanyun Liao as the implementation author and Carlos Villela as the architecture-budget update author.
GitHub reports both replacement commits as verified.
The focused 27-test destroy suite, CLI type-check, source-shape check, and diff-scoped hooks pass on the replacement branch.
The replacement includes the documentation-writer receipt and a fresh nine-category security review.

This PR cannot satisfy the commit-verification gate because its published history contains an unverified commit. #8313 now owns the remaining CI and review work. Closing this PR as superseded avoids duplicate review and merge activity.

cv · 2026-08-05T09:51:04Z

Replacement: #8324. It preserves the original verified contributor commit and effective destroy fix, refreshes onto current protected main, and uses only GitHub-verified commits. This closed PR remains unsuitable for merge because its published history contains an unverified maintenance commit.

…#8313)  ## Summary This replacement preserves the implementation and contributor attribution from #8298 while removing an unverified commit from the published history. Sandbox destruction now returns a structured, redacted failure when MCP bridge preparation or finalization fails instead of exposing an uncaught stack trace. ## Related Issue Refs #8103 Supersedes #8298 after this replacement passes all repository gates. ## Changes - Catch `McpBridgeError` before destructive work starts and return its redacted failure result. - Catch `McpBridgeError` after sandbox deletion and preserve the registry state required for retry. - Rethrow other error types and add negative-path tests for both MCP bridge failure stages. - Preserve Yanyun Liao as the implementation author and Carlos Villela as the architecture-budget update author. - Ratchet the affected source architecture budgets to the values measured by the replacement branch. ## 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: Existing sandbox lifecycle documentation covers MCP preparation failures, retry-state preservation, post-deletion cleanup, and rerunning destroy. This change returns the documented structured failure instead of an uncaught stack trace. - [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 nine-category maintainer security review passed with no findings: #8313 (comment) - [ ] 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: `docs/security/best-practices.mdx` already defines the CLI redaction contract, and `docs/manage-sandboxes/manage-mcp-servers.mdx` already documents preserved recovery state and rerunning `destroy`. The change adds no command, option, configuration, lifecycle boundary, or supported user procedure. - Agent: Codex Desktop   ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## 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 validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/actions/sandbox/destroy-flow.test.ts` passed 27 tests. - [x] Applicable broad gate passed — `npm run typecheck:cli`, `npm run source-shape:check`, and `npx prek run --from-ref origin/main --to-ref HEAD` passed. - [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: yanyunl1991 <mzhong@nvidia.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>  ## Summary by CodeRabbit * **Bug Fixes** * Sandbox destruction now handles MCP bridge failures gracefully instead of exposing unhandled errors. * Preparation failures stop deletion and cleanup safely. * Finalization failures return clear failure results, redact sensitive details, and preserve required sandbox and gateway state. * Retrying after a finalization failure now completes cleanup successfully. * **Tests** * Added coverage for MCP bridge failures during sandbox destruction, including exit codes, redaction, retries, and state preservation.  --------- Signed-off-by: yanyunl1991 <yanyunl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Yanyun Liao <yanyunl@nvidia.com> Co-authored-by: Carlos Villela <cv@lixo.org>

apurvvkumaria added the v0.0.103 Release target label Aug 5, 2026

apurvvkumaria self-assigned this Aug 5, 2026

merge: refresh PR #8298 with current main

0b1eeb7

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

cv and others added 2 commits August 5, 2026 01:44

Merge branch 'main' into fix/mcp-bridge-destroy-error-8103

9d3b52d

chore(architecture): ratchet source budgets

331c168

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

apurvvkumaria mentioned this pull request Aug 5, 2026

fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103) #8313

Merged

23 tasks

apurvvkumaria closed this Aug 5, 2026

cv mentioned this pull request Aug 5, 2026

fix(sandbox): preserve structured MCP destroy failures (#8103) #8324

Closed

23 tasks

Navigation Menu

Uh oh!

fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103) - #8298

fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103)#8298
yanyunl1991 wants to merge 4 commits into
mainfrom
fix/mcp-bridge-destroy-error-8103

yanyunl1991 commented Aug 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

github-code-quality Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

apurvvkumaria commented Aug 5, 2026

Uh oh!

apurvvkumaria commented Aug 5, 2026

Uh oh!

copy-pr-bot Bot commented Aug 5, 2026

Uh oh!

cv commented Aug 5, 2026

Uh oh!

apurvvkumaria commented Aug 5, 2026

Uh oh!

cv commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

yanyunl1991 commented Aug 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Type of Change

Quality Gates

Documentation Writer Review

DGX Station Hardware Evidence

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-code-quality Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor — No blocking findings reported

Model lanes

E2E guidance

Uh oh!

apurvvkumaria commented Aug 5, 2026

Uh oh!

apurvvkumaria commented Aug 5, 2026

Security review — PASS

Uh oh!

copy-pr-bot Bot commented Aug 5, 2026

Uh oh!

cv commented Aug 5, 2026

Uh oh!

apurvvkumaria commented Aug 5, 2026

Uh oh!

cv commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yanyunl1991 commented Aug 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

github-code-quality Bot commented Aug 5, 2026 •

edited

Loading

github-actions Bot commented Aug 5, 2026 •

edited

Loading