fix(sandbox): catch McpBridgeError in destroy MCP bridge path (#8103) - #8313
Conversation
…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> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review: PASS, no findings Updated after the review-feedback changes on the current branch.
Validation:
Contributor attribution remains preserved for Yanyun Liao and Carlos Villela. |
📝 WalkthroughWalkthroughSandbox destruction now handles ChangesSandbox destruction error handling
Architecture budget updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant destroySandbox
participant MCPBridge
participant SandboxState
destroySandbox->>MCPBridge: Prepare MCP state
MCPBridge-->>destroySandbox: Return McpBridgeError
destroySandbox-->>SandboxState: Retain state and return failure
destroySandbox->>MCPBridge: Finalize MCP state
MCPBridge-->>destroySandbox: Return redacted failure
destroySandbox-->>SandboxState: Preserve retryable state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit ec94feb in the TypeScript / code-coverage/cliThe overall coverage in commit ec94feb in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/lib/actions/sandbox/destroy-execution.ts`:
- Around line 343-357: Update finalizeMcpDestroy error logging to pass the error
through redactDestroyError before logging its details, ensuring secret markers
never reach console.error. Add a regression test for the destroy flow that
triggers an MCP cleanup failure and asserts console.error receives the redacted
output rather than the secret marker.
In `@test/helpers/destroy-flow-test-assertions.ts`:
- Around line 232-237: Update expectMcpFinalizeBridgeErrorReturnsFailure to
capture the invocation index of the sandbox delete spy and assert that
finalizeMcpBridgesAfterSandboxDeleteSpy was called afterward. Preserve the
existing assertions that finalization occurs and cleanup spies are not called.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d7ab7629-02d2-49b0-93a5-3360bd612b78
📒 Files selected for processing (5)
ci/source-architecture-budget.jsonsrc/lib/actions/sandbox/destroy-execution.tssrc/lib/actions/sandbox/destroy-flow.test.tstest/helpers/destroy-flow-test-assertions.tstest/helpers/destroy-flow-test-harness.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed PRA-1 in the latest branch revision. A new two-attempt destroy-flow test makes MCP finalization fail after deletion, verifies the retained local state, marks the remote sandbox absent, retries destroy, and confirms finalization plus registry, onboarding-session, and applicable gateway cleanup converge. The focused destroy-flow suite passes all 28 tests. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer merge-train review of PR commit This PR is not approval-ready.
Compared with #8324, this branch is closer to approval because it already covers redacted finalization errors, delete-before-finalize ordering, and retry retirement. Its automated-review threads are also resolved. Neither PR satisfies the broader rebuild, recreate, and recorded-or-live drift acceptance criteria in #8103, so keeping After the refresh and fixes, rerun the focused destroy-flow tests and repository PR validation. Refresh the documentation writer and security reviews for the resulting PR commit. Then let the required checks complete. |
|
Addressed the CLI shard-4 failure in revision The retry-convergence test asserted that the shared gateway was cleaned up, but its second destroy call did not request gateway cleanup. That passed in an interactive local environment because the test prompt defaulted to “yes”; CI correctly preserves the gateway in non-interactive mode unless cleanup is explicit. The second call now sets |
|
The completed CLI run has two independent failures:
I am not rerunning the full failed job set while the deterministic shard-8 base failure remains. After #8327 merges, the branch can refresh and run one clean gate that also supersedes the transient timeout result. |
|
The one permitted rerun of the Terra advisor lane also failed in the advisor runtime while verifying its analysis outcome. The Nemotron lane and publisher passed, and the branch has 39 passing checks with no pending checks or unresolved review threads. I will not rerun the same transient lane again without new evidence. The documented external advisor failure and independent approval remain the merge blockers. |
4 participants
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
McpBridgeErrorbefore destructive work starts and return its redacted failure result.McpBridgeErrorafter sandbox deletion and preserve the registry state required for retry.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/security/best-practices.mdxalready defines the CLI redaction contract, anddocs/manage-sandboxes/manage-mcp-servers.mdxalready documents preserved recovery state and rerunningdestroy. The change adds no command, option, configuration, lifecycle boundary, or supported user procedure.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/destroy-flow.test.tspassed 27 tests.npm run typecheck:cli,npm run source-shape:check, andnpx prek run --from-ref origin/main --to-ref HEADpassed.npm run docsbuilds without warnings (doc changes 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
Tests