Repository navigation
Conversation
…omplete destroy state Fixes NVIDIA#6376. Two gaps let an incomplete MCP destroy transaction strand a sandbox with no diagnosis and no non-destructive recovery: 1. The rebuild CLI path's bail() discarded its message and exited bare (exit 1 with no output). It now prints the redacted failure reason to stderr before exiting, so the destroy-marker refusal (and every other bail) is visible. 2. assertMcpDestroyNotPending fired before the --force handling in mcp remove, so the documented forced cleanup was itself blocked and the only way out was full sandbox destruction. A forced remove now adopts the incomplete destroy transaction's cleanup manifest for the named server, and the durable destroy markers are cleared once no bridge entries remain. The guard's error message now names the forced-remove recovery command. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: sauravdev <saurava@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds detection and clearing of incomplete MCP destroy-transaction markers, wires forced ChangesMCP destroy marker recovery
Rebuild bail failure reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts (1)
61-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove issue reference to individual test titles.
The
(#6376)reference is on thedescribeblock rather than eachittitle. As per coding guidelines,**/*.test.tsfiles should "Write behavior-oriented test titles, and put local issue references in a final(#1234)suffix" — i.e., on each test title.✏️ Proposed fix
-describe("rebuild bail reporting (`#6376`)", () => { - it("prints the failure reason on stderr before exiting in CLI mode", () => { +describe("rebuild bail reporting", () => { + it("prints the failure reason on stderr before exiting in CLI mode (`#6376`)", () => { const errors: string[] = []; const errorSpy = vi.spyOn(console, "error").mockImplementation((message: unknown) => { errors.push(String(message)); }); const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { throw new Error(`exit:${code}`); }) as never); const { bail } = createRebuildCommandContext({}, {}); expect(() => bail("Failed to preserve MCP bridges before rebuild: destroy pending")).toThrow( "exit:1", ); expect(errors.join("\n")).toContain("Failed to preserve MCP bridges before rebuild"); expect(exitSpy).toHaveBeenCalledWith(1); errorSpy.mockRestore(); }); - it("keeps the throwing bail contract when throwOnError is set", () => { + it("keeps the throwing bail contract when throwOnError is set (`#6376`)", () => { const { bail } = createRebuildCommandContext({}, { throwOnError: true }); expect(() => bail("boom")).toThrow("boom"); }); });🤖 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/rebuild-preflight-confirmation.test.ts` around lines 61 - 84, The issue is that the issue reference is attached to the describe block instead of the individual test names. Update the rebuild bail reporting tests in rebuild-preflight-confirmation.test.ts so each it(...) title includes the local issue suffix as a final "(`#6376`)" while keeping the describe block behavior-oriented and free of the reference; use the existing test titles around createRebuildCommandContext and bail to apply the change consistently.Source: Coding guidelines
test/mcp-destroy-lifecycle.test.ts (1)
741-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey the policy-state mock per server.
After the first removal, this mock reports every policy as
"absent", so the secondslackcleanup can pass without exercising its own policy cleanup path. Make the mock depend on the policy name instead.Suggested test tightening
- testState.getPresetContentGatewayState.mockImplementation(() => - removedPolicies.size > 0 ? "absent" : "match", - ); + testState.getPresetContentGatewayState.mockImplementation( + (_sandbox: string, policyName: string) => + removedPolicies.has(policyName) ? "absent" : "match", + );As per path instructions, “Review tests for behavioral confidence rather than implementation lock-in” and flag “broad mocks that bypass the behavior under test.”
🤖 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 `@test/mcp-destroy-lifecycle.test.ts` around lines 741 - 743, The policy-state mock in the destroy lifecycle test is too broad because it keys only off removedPolicies, so the second slack cleanup can skip its own policy cleanup path. Update the mock in the test setup around getPresetContentGatewayState to vary by server/policy identity instead of global removal count, using the existing server-specific identifiers in the test so each cleanup path is exercised independently.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.
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts`:
- Around line 61-84: The issue is that the issue reference is attached to the
describe block instead of the individual test names. Update the rebuild bail
reporting tests in rebuild-preflight-confirmation.test.ts so each it(...) title
includes the local issue suffix as a final "(`#6376`)" while keeping the describe
block behavior-oriented and free of the reference; use the existing test titles
around createRebuildCommandContext and bail to apply the change consistently.
In `@test/mcp-destroy-lifecycle.test.ts`:
- Around line 741-743: The policy-state mock in the destroy lifecycle test is
too broad because it keys only off removedPolicies, so the second slack cleanup
can skip its own policy cleanup path. Update the mock in the test setup around
getPresetContentGatewayState to vary by server/policy identity instead of global
removal count, using the existing server-specific identifiers in the test so
each cleanup path is exercised independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 758521ed-39b9-446b-92db-d13af91aaf5b
📒 Files selected for processing (5)
src/lib/actions/sandbox/mcp-bridge-remove.tssrc/lib/actions/sandbox/mcp-bridge-state.tssrc/lib/actions/sandbox/rebuild-preflight-confirmation.test.tssrc/lib/actions/sandbox/rebuild-preflight-confirmation.tstest/mcp-destroy-lifecycle.test.ts
Collaborator
|
Closing as fully superseded by #6377, merged as Exact-head/current-
Thank you @sauravdev for the focused alternative implementation, verification, and test coverage. No code from this branch was copied into #6377 or a replacement commit: the safe behavior had already landed independently, while the only unique behavior is the pending-state widening described above. Accordingly, there is no replacement commit requiring a co-author trailer. |
wscurran
added
area: cli
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
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.
Summary
Fixes the two operator-facing gaps reported in #6376:
nemoclaw <name> rebuildexited 1 with no message when an incomplete MCP destroy transaction was present, and the documented recovery (mcp remove <server> --force) was itself blocked by the same guard, leaving full sandbox destruction as the only way out.Related Issue
Fixes #6376
Changes
src/lib/actions/sandbox/rebuild-preflight-confirmation.ts: the CLI-modebail()previously discarded its message and calledprocess.exit(code)bare. It now prints the redacted failure reason to stderr (Rebuild failed: …) before exiting, so the destroy-marker refusal — and every other rebuild bail — is diagnosable.src/lib/actions/sandbox/mcp-bridge-state.ts:assertMcpDestroyNotPendingnow names the forced-remove recovery command (with a real server name when one is registered); addedhasMcpDestroyMarkersandclearMcpDestroyMarkersIfNoBridgeshelpers.src/lib/actions/sandbox/mcp-bridge-remove.ts:mcp remove <server> --forcenow runs under destroy markers, adopting the incomplete transaction's cleanup manifest for that server (announced on stdout). The durable markers are cleared once no bridge entries remain, including the forced-remove paths for apreparedadd and for a server that is no longer registered.test/mcp-destroy-lifecycle.test.ts(plain remove still refused with the recovery hint; forced remove of the last entry clears markers; markers persist while other entries remain; bare markers with no entries are cleared), plus 2 new cases inrebuild-preflight-confirmation.test.tspinning the bail message-printing and throwing contracts.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification detail:
npm run build:cliclean;test/mcp-destroy-lifecycle.test.ts24/24,rebuild-preflight-confirmation.test.ts11/11, plusrebuild-flow,rebuild-destroy-phase,mcp-provider-ownership,mcp-add-crash-consistency,mcp-bridge-status-removal, andhermes-mcp-shields-orderall green (140 tests across the touched surface).Signed-off-by: sauravdev saurava@nvidia.com
Summary by CodeRabbit