Repository navigation
Control socket: never present resume approval UI from surface.resume.set (#13369) - #13704
Merged
Merged
Conversation
…ting (#13369) Drives `surface.resume.set` through the real v2 socket dispatcher with a command no approval record matches, and expects the reply to carry `approval_required: true` with the binding stored untrusted. On main the socket path resolves approval by presenting a blocking NSAlert, so the reply has no such field and this test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`surface.resume.set` over the socket ran `NSAlert.runModal()` inside the command's main-actor job when the proposed command had no approval record, parking the command coordinator until the alert closed; the socket stopped answering and new clients saw EPIPE. The socket entry now proposes with `.controlSocket` origin: a binding that still needs approval is stored without resume trust and the reply reports `approval_required: true`. Only the terminal's Resume Commands context menu (`.userInterface`) may still show the approval prompt. The trust policy is split out as `SurfaceResumeApprovalStore.proposalNeedsApproval`, and the approval helpers move to TerminalController+SurfaceResumeApproval.swift to keep file length budgets. Smaller alternative to #13397 (@austinywang), covering only its "never park a socket command on modal UI" half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
teamleaderleo
enabled auto-merge (squash)
September 22, 2026 16:30
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Sep 22, 2026
Main's #13704 fixed the modal half of #13369: surface.resume.set never presents approval UI and reports approval_required. Take that design as-is (its files, tests, and docs), drop this branch's queued-sheet prompter and its test, and keep the second half #13704 left open: bounded main-actor hops, the blocking worker lane, pending-job expiry, overloaded replies, and the accept-drop hook. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 30, 2026
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.
Fixes #13369. This is a smaller alternative to #13397 by @austinywang. It takes only the "never park a socket command on modal UI" half of that PR.
Problem
surface.resume.setover the control socket calledNSAlert.runModal()inside the command's main-actor job whenever the proposed command had no approval record (controlSurfaceResumeSet→surfaceResumeBindingWithApproval→surfacePromptForResumeApproval). The hang sample in #13369 shows this stack. While the alert was up, the command coordinator was parked on the main actor, so the socket stopped answering and new clients gotBroken pipe.Resulting behavior
surface.resume.setnever presents approval UI. A binding that still needs approval is stored without resume trust (auto_resume: false, no approval record). It stays available for manual restore, which is already the default for socket-created commands.approval_required(true/false). It tells the caller whether a person still has to approve the command in cmux. It appears only onsurface.resume.setreplies.The decision is made where the request enters.
controlSurfaceResumeSet(the socket entry) passes.controlSocket, and the context menu passes.userInterface.SurfaceResumeApprovalStore.proposalNeedsApprovalis now the trust policy alone. The main-thread and XCTest checks stay inshouldPromptForProposal.Why this option and not an async prompt
The socket policy (
skills/cmux-socket-policy) says a socket command must not steal focus or raise windows unless focus is what it is for. An app-modal approval alert does both, so the socket path should not present one at all.Replying first and then showing the alert from a main-queue job does not fix the wedge. A nested
runModalstarted from a main-queue job does not drain the main queue, so every other socket command's main hop still starves while the alert is up. Fixing that needs a non-modal, queued, coalescing prompter, and that is most of #13397's 2,600 lines.The cost: a script that proposes a command can no longer surface the approval prompt. To approve it, the user sets the command from the context menu or changes the signed prefix in Settings > Terminal > Resume Commands.
docs/agent-hooks.mdnow says so.Validation
e4ab87a) adds onlycmuxTests/SurfaceResumeSocketApprovalTests.swift, which is wired inproject.pbxproj. The test drivessurface.resume.setthrough the real v2 dispatcher (TerminalController.handleSocketLine) with a command that no approval record matches. It expectsapproval_required: trueand the binding stored untrusted. On main the reply has no such field, so the test fails. A second case checks that a trustedagent-hookbinding reportsapproval_required: false.swift testinPackages/macOS/CmuxControlSocketpassed 463 tests in 59 suites locally, before the new payload tests moved into their own file. After the move, the twoControlCommandCoordinatorSurface*suites passed 30 tests, including the newControlCommandCoordinatorSurfaceResumeApprovalTests(theapproval_requiredfield on set, and its absence on get).cmux-unitbuild-for-testingin a tagged DerivedData directory failed withNo space left on device: the Mac had about 1 GiB free, shared with other agents' builds. The app andcmuxTestscompile check is left to this PR's CI. The package changes compiled and passed their tests (above)../scripts/lint-pbxproj-test-wiring.shpasses. Every touched Swift file stays within its merge-base line budget. The approval helpers moved into a newSources/TerminalController+SurfaceResumeApproval.swiftfor this.Remaining gap
docs/agent-hooks.md.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes #13369.
surface.resume.setover the control socket calledNSAlert.runModal()inside the command's main-actor job when the proposed command had no approval record, parking the command coordinator on the main actor and stopping the socket from answering (new clients gotBroken pipe). The socket path now never presents approval UI.surface.resume.setstores a binding that still needs approval without resume trust (auto_resume: false, no approval record); it stays available for manual restore.approval_required(true/false) onsurface.resume.set, telling the caller whether a person still has to approve the command in cmux..controlSocket, the context menu passes.userInterface.SurfaceResumeApprovalStore.proposalNeedsApprovalis now the pure trust policy, and the approval helpers moved intoSources/TerminalController+SurfaceResumeApproval.swift.docs/agent-hooks.mdnow notes that CLI and socket requests never show the prompt and that approval is done in the app.Written for commit 965fa5d. Summary will update on new commits.