fix(sandbox): preserve structured MCP destroy failures (#8103) - #8324
fix(sandbox): preserve structured MCP destroy failures (#8103)#8324cv wants to merge 20 commits into
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: Carlos Villela <cvillela@nvidia.com>
cv
added
area: sandbox
📝 WalkthroughWalkthroughThe destroy flow now structures typed MCP bridge failures, skips deletion for already-absent sandboxes, and propagates deletion status to cleanup logic. Tests cover redaction, exit codes, state preservation, and successful reruns. ChangesSandbox destruction error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DestroyFlow
participant MCPBridge
participant SandboxState
DestroyFlow->>MCPBridge: Prepare MCP bridge
MCPBridge-->>DestroyFlow: McpBridgeError or prepared state
DestroyFlow->>SandboxState: Delete sandbox when present
DestroyFlow->>MCPBridge: Finalize MCP bridge
MCPBridge-->>DestroyFlow: Success or McpBridgeError
DestroyFlow-->>SandboxState: Apply cleanup from structured result
Possibly related PRs
Suggested reviewers: 🚥 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 df6acd1 in the TypeScript / code-coverage/cliThe overall coverage in commit df6acd1 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: 1
🤖 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 `@test/helpers/destroy-flow-test-assertions.ts`:
- Around line 232-237: Update expectMcpFinalizeBridgeErrorReturnsFailure to
assert that the sandbox deletion operation for “alpha” was called and that
finalizeMcpBridgesAfterSandboxDeleteSpy occurred after that deletion. Preserve
the existing assertions that finalization ran and registry cleanup did not
occur.
🪄 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: 2cb4ecb6-0768-4ccd-8209-69227eddcf34
📒 Files selected for processing (4)
src/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 — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
Blockers
|
|
Maintainer review of commit At that commit, PR #8324 was not approval-ready.
At that review, neither #8313 nor #8324 was eligible. Comparator scores were 13.5/16 for #8313 and 12.5/16 for #8324. This comment records the historical review of commit |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> (cherry picked from commit 5934ec1)
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> (cherry picked from commit adf49fd)
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> (cherry picked from commit 5934ec1)
There was a problem hiding this comment.
Actionable comments posted: 1
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/destroy-flow.test.ts (1)
377-397: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity Test Coverage (CWE-532): Insertion of Sensitive Information into Log File
Exploitability: Theoretical
Cover redaction for MCP preparation errors.
prepareMcpBridgeErrorhas no secret marker, so the test does not prove redaction for preparation failures. Add a credential-shaped marker and assert that it is absent from all captured error arguments. The finalization path currently emits each message as one argument, so the later-argument concern applies only if that output contract changes.🤖 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/destroy-flow.test.ts` around lines 377 - 397, Update the MCP preparation failure test using createDestroyHarness and expectMcpPrepareBridgeErrorAborts to include a credential-shaped secret marker in prepareMcpBridgeError, then assert the marker is absent from every captured error argument. Keep the existing finalization test and its single-argument output contract unchanged.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.
Inline comments:
In `@src/lib/actions/sandbox/destroy-flow.test.ts`:
- Around line 408-418: Update the destroySandbox test around
harness.destroySandbox to assert through the public runOpenshell boundary that
the command for deleting sandbox alpha is invoked exactly once after the retry
path completes. Keep the existing absent-sandbox preparation and MCP bridge
assertions, and ensure the assertion distinguishes the initial delete from any
retry.
---
Outside diff comments:
In `@src/lib/actions/sandbox/destroy-flow.test.ts`:
- Around line 377-397: Update the MCP preparation failure test using
createDestroyHarness and expectMcpPrepareBridgeErrorAborts to include a
credential-shaped secret marker in prepareMcpBridgeError, then assert the marker
is absent from every captured error argument. Keep the existing finalization
test and its single-argument output contract unchanged.
🪄 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: 94e6cc2b-321a-4b83-a6a5-9ce89fa8c13f
📒 Files selected for processing (4)
src/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
🚧 Files skipped from review as they are similar to previous changes (2)
- test/helpers/destroy-flow-test-assertions.ts
- src/lib/actions/sandbox/destroy-execution.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com> (cherry picked from commit d312465) Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Superseded by #8313, merged in commit ac24de0. The merged PR carries the structured MCP destroy error handling for #8103 and preserves the contributor history from Yanyun Liao, Apurv Kumaria, and Carlos Villela. The remaining #8324 changes affect the path used when the sandbox is already absent and are not required for that fix. |
3 participants
Summary
destroynow returns the existing structured agent adapter error when adapter preparation or finalization fails. Previously, the command exposed an uncaught stack trace. This PR preserves Yanyun Liao's contributor commit and Apurv Kumaria's two remediation commits with cherry-pick provenance. It supersedes #8298; #8103 continues to track separate rebuild authority drift.Related Issue
Refs #8103
Changes
McpBridgeErrorduring MCP preparation and return before OpenShell sandbox deletion.McpBridgeErrorafter confirmed sandbox deletion, redact the finalization error, and retain the sandbox registry entry and gateway.openshell sandbox deleteagain.4f0f7ccdunchanged.adf49fd8and5934ec1a.2b90d18ce.Type of Change
Quality Gates
df6acd128against base commit2b90d18ce. The merge changed no PR-touched file.installer-integrationfailed and the requiredE2E / PR Gateis still running for commitdf6acd128.Documentation Writer Review
no-docs-neededDGX 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 unavailabledf6acd128.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: GitHub Actions is running for commitdf6acd128;installer-integrationmust be resolved.npm run docsbuilds without warnings (doc changes only)Sensitive-path Security Review
McpBridgeErrorboundary is handled.openshell sandbox deletecall across the initial destroy and its rerun. GitHub Actions must validate commitdf6acd128.Signed-off-by: Carlos Villela cvillela@nvidia.com