Resolve restore targets from live process identity - #9384
Merged
Merged
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestore now resolves the caller’s terminal binding before selecting a surface. It validates delivery targets, retries transient failures, and applies protocol and TTY fallbacks. TTY metadata restoration clears stale device identity. Tests cover routing, failure, timeout, and ordered socket responses. ChangesRestore surface resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreCommand
participant TerminalBindingResolver
participant UnixSocketResponder
RestoreCommand->>TerminalBindingResolver: resolve current restore surface
TerminalBindingResolver->>UnixSocketResponder: request PID delivery target
UnixSocketResponder-->>TerminalBindingResolver: return target metadata
TerminalBindingResolver-->>RestoreCommand: return validated surface or fallback
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning, 1 inconclusive)
✅ Passed checks (20 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
austinywang
changed the title
![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 `@cmuxTests/CMUXCLIErrorOutputRegressionTests.swift`:
- Around line 646-656: The test fixture in the caller-target response does not
exercise duplicate-TTY handling because currentRestoreSurfaceID only uses the
top-level source, pid_resolution, workspace_id, and surface_id fields. Either
remove the unused terminals row and rename the assertion to reflect top-level
surface precedence, or change the mocked response to an actual
ambiguity/not_found case and assert that currentRestoreSurfaceID falls back to
CMUX_SURFACE_ID.
- Line 657: Rename the local String constants named restoreResponse in the
affected test cases to a distinct name, and update each corresponding responder
initializer to use the renamed constant while preserving the
restoreResponse(result:) helper method calls.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: ac624011-0161-419d-8184-1ceb205eeed9
📒 Files selected for processing (2)
CLI/CMUXCLI+Restore.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/CMUXCLI+Restore.swift (1)
182-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMalformed-success handling relies on incidental catch routing.
The guard at Lines 199-206 throws
currentRestoreSurfaceUnknownError()from inside thedoblock. That error is then caught bycatch let error as CLIErrorat Line 208 and reachesdefault:only because the helper'sv2Codeisnil. The result is correct today, but the mechanism is indirect: a later change that assigns av2CodetocurrentRestoreSurfaceUnknownError(), or a newcaseadded to the switch, can silently redirect this path (for example into themethod_not_foundlegacy fallback) instead of failing closed.Close and throw directly at the guard site instead of routing the local error back through the switch.
♻️ Proposed fix to close and throw directly on malformed success
guard payload["source"] as? String == "pid", payload["pid_resolution"] as? String == resolution, let workspaceID = normalizedHandleValue(payload["workspace_id"] as? String), isUUID(workspaceID), let surfaceID = normalizedHandleValue(payload["surface_id"] as? String), isUUID(surfaceID) else { - throw currentRestoreSurfaceUnknownError() + client.close() + throw currentRestoreSurfaceUnknownError() } return surfaceID } catch let error as CLIError { switch error.v2Code { case "not_found": client.close() throw currentRestoreSurfaceUnknownError() case "method_not_found", "unrecognized_method": // These protocol replies were consumed in full, so the socket // remains synchronized for the legacy discovery request. return legacyRestoreSurfaceID(client: client) default: client.close() throw error }🤖 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 `@CLI/CMUXCLI`+Restore.swift around lines 182 - 225, Update the malformed-success guard in currentRestoreSurfaceID so it closes the client and throws currentRestoreSurfaceUnknownError() directly when the response fields are invalid. Keep protocol-error handling through the existing CLIError switch unchanged, ensuring malformed successful responses cannot reach legacyRestoreSurfaceID through catch routing.
🤖 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.
Outside diff comments:
In `@CLI/CMUXCLI`+Restore.swift:
- Around line 182-225: Update the malformed-success guard in
currentRestoreSurfaceID so it closes the client and throws
currentRestoreSurfaceUnknownError() directly when the response fields are
invalid. Keep protocol-error handling through the existing CLIError switch
unchanged, ensuring malformed successful responses cannot reach
legacyRestoreSurfaceID through catch routing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d2ac8e3-e17c-41f8-b95e-b19a29c81842
📒 Files selected for processing (3)
CLI/CMUXCLI+ClaudeHookWorkspaceRouting.swiftCLI/CMUXCLI+Restore.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
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 `@CLI/CMUXCLI`+Restore.swift:
- Around line 191-231: Remove the retryDelays/retryIndex loop and make the
resolve_delivery_target path fail closed on the first not_found response. Move
the readiness ordering guarantee into the server handler or ensure shell
integration completes registration before returning control to the caller, while
preserving legacy fallback for method_not_found/unrecognized_method and closing
the client for other failures.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 6aeacd6d-0932-40a5-bfa9-7173bdab56f6
📒 Files selected for processing (5)
CLI/CMUXCLI+Restore.swiftSources/AgentDeliveryTargetResolution.swiftSources/Workspace.swiftcmuxTests/AgentNotificationMutationBoundaryTests.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
…-nothing-to-restore
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
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
1 participant
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
cmux restoretargets from the CLI process's live controlling TTY instead of inheritedCMUX_SURFACE_IDstate.Investigation
Hypothesis 2 was confirmed. A controlled stale
CMUX_SURFACE_IDreproduced the exact falsenothing to restoreresult: the old restore path queried the inherited surface even though the terminal running the command belonged to another surface with a valid Pi binding. Issue #8672 corrected Pi hook dispatch and its stale-target circuit breaker, but it did not change restore-command routing. Conda activation,cd, and nested shells did not independently make the variable stale during current testing.Hypothesis 1 did not explain the report. Pi discovery runs on the 8 second autosave cadence after a 0.65 second typing quiet period. A fresh local Pi launch bound after about 8.5 seconds. For the reported session, the checkpoint was persisted at 04:10:35, the app quit at 04:10:49, and it relaunched at 04:10:51. The restore binding therefore existed before the reported command.
Review exposed additional routing-boundary defects. Ended remote terminals retained current-runtime TTY provenance. Disconnect also left authenticated relay provenance live, and a delayed
report_ttycould recreate proof for an ended terminal. Nonpersistent reconnects did not retire the previous remote PTY's report. PID routing repeatedly allocated TTY strings and calledstat. Workspace transfer restored TTY proof before destination lifecycle initialization cleared it. Persistent PTY bridge retries were mistaken for new remote shells even though the report-once shell hook does not run again. A relay-authenticated remote terminal moved into a new ordinary workspace fell outside the original workspace's candidate scan. A second ordinary-workspace move dropped its remote classification, and a persistent retry after the first move consulted the ordinary workspace instead of the configuration attached to the transferred terminal.Final review found three trust-boundary gaps. The relay did not stamp its authenticated owner onto
surface.report_tty, so that method could retain a caller-supplied owner marker. Remote registration checked the live origin but not the current terminal lifecycle and attach attempt. Local and transferred TTY proof was not scoped to the Ghostty runtime generation, so proof could outlive runtime replacement.A follow-up concern about local detached
report_ttyordering did not reproduce. A worst-case harness invoked the real zsh integration and built restore CLI immediately after_cmux_report_tty_once. The report connection arrived first in 200 of 200 runs, with a 1.668 ms minimum lead. Interactive use adds the prompt-to-command interval. No polling was restored without contrary evidence.A later review found two remote readiness gaps. Existing tmux shells kept stale lifecycle and attach-attempt values after a new cmux attach, so the server correctly rejected their TTY reports. Fish and the initial remote bootstrap also launched work before
surface.report_ttycompleted. These were distinct from Pi local discovery cadence and did not change the confirmed root cause for #9380.Fix
Local implicit restore calls
agent.resolve_delivery_targetwith the CLI PID andpid_resolution: controlling_tty. The app matches the kernel controlling-device identifier against live local terminals. It ignores ambientCMUX_SURFACE_ID,TTY, andSSH_TTYclaims for this decision.TerminalSurfacecaptures its Ghostty TTY name and device identifier once per runtime lifecycle. Ghostty starts the PTY asynchronously, so a missed eager lookup retries lazily only until the PTY exists. Later probes use the in-memory device identifier. The cache is cleared whenever the runtime pointer changes.Remote reports remain scoped to the authenticated relay workspace and follow a terminal into its current Workspace or Dock owner. The relay overwrites
surface.report_ttyprovenance with its authenticated workspace. Every remote TTY report carries the terminal lifecycle and attach attempt through bootstrap, zsh, bash, and fish paths, and registration requires all three identities to match the live terminal. Mosh creates the attempt before staging its bootstrap and registers it before launch. Authenticated origin and attempt identity are preserved across detach and attach, including repeated moves through ordinary workspaces. Destination lifecycle tracking initializes before transferred proof is restored. The transfer remains classified as remote while live relay provenance exists, and retry policy reads the configuration attached to that transferred terminal. New terminal lifecycles, disconnect, lifecycle end, and nonpersistent attach attempts retire old proof. Local and transferred TTY proof is also tied to the current Ghostty runtime generation. Persistent transport retries retain proof because they reattach the same remote PTY and shell.Existing tmux sessions now receive current
CMUX_TERMINAL_LIFECYCLE_IDandCMUX_SSH_ATTEMPT_IDvalues on attach, and zsh, bash, and fish pull those keys from tmux. Fish waits for relay acknowledgement before latching its one-time TTY report, and the remote bootstrap completessurface.report_ttybefore starting the interactive shell.surface.ports_kickremains asynchronous, and no polling was added.A complete
not_founddelivery-target reply is authoritative and fails closed immediately. The CLI does not poll for shell registration. Older apps still use the unique, workspace-scoped legacy match aftermethod_not_foundorunrecognized_method. Ambiguous, malformed, timed-out, or unscoped results fail closed.Red and green proof
942469d093adds a failing regression through the realmoveSurfaceToNewWorkspacepath. Relay resolution returnednot_foundafter the move.ca95cc4185preserves authenticated report provenance and resolves the terminal at its current live owner. The new regression passes.2c625bde77adds failing tests for workspace transfer and persistent Workspace and Dock bridge retries. All three returned no delivery target before the fix.413d5a487drestores transferred proof after destination lifecycle setup and distinguishes persistent bridge attempts from new remote PTYs.d2759b3b24adds failing tests for single-reply local and relay failure plus Workspace and Dock reconnect provenance.0a9d22111eremoves readiness polling and makes the first completenot_foundauthoritative.5db73900a1adds failing tests for ended Workspace and Dock TTY reuse plus repeated native TTY reads.c32a53fcf1invalidates ended-lifecycle runtime proof and caches the PTY device.13f3b1e7a2adds failing tests showing disconnect leaves the old TTY resolvable and a delayed report revives an ended terminal.b1d61ad219clears authenticated relay provenance on disconnect and terminal end, then requires that live provenance for remote TTY registration.f85da04bf5adds failing regressions for a second ordinary-workspace move and a persistent retry after the first move.01494812d6keeps remote classification tied to live relay provenance and reads retry policy from the terminal's transferred configuration.d78041b2f7adds failing regressions for relay owner spoofing, stale attach attempts, and local TTY proof surviving runtime replacement.c482c7a54astamps relay ownership, validates lifecycle and attempt identity, propagates that identity through every remote report path, and scopes proof to the runtime generation.1dd6ab0944extends the shared live-process target regression to Grok without changing Grok discovery code.48157de9f1adds failing regressions for stale tmux lifecycle identity, unacknowledged fish registration, and bootstrap ordering. Before the fix, zsh, bash, and fish retained stale IDs, fish latched after an unsuccessful call, and bootstrap returnedraced.e693ac2cfarebinds lifecycle identity through tmux and waits for TTY registration acknowledgement before readiness.Verification
cmux-unit: the two disconnect and delayed-report regressions failed beforeb1d61ad219and passed afterward.cmux-unit: the three trust-boundary regressions failed beforec482c7a54aand passed afterward.cmux-unit: the complete 22-case transfer, repeated-move, lifecycle, reconnect, retry, restore, relay-scoping, and trust-boundary matrix passed on final HEAD.cmux-unit: three focused zsh and bash relay payload tests passed.cmux-unit:testTerminalEndClearsReadinessPendingRemoteConfigurationpassed on final HEAD.cmux-unit: 27 targeted restore and routing tests passed before the final lifecycle review fix.CmuxFoundation: 14 focused Mosh and remote bootstrap staging tests passed.CmuxControlSocket: 17 focused surface coordinator tests passed.CmuxTerminalTests:controllingTTYNameIsReadOncePerRuntimeLifecyclepassed with one native read.cmux-unit: 27 focused shell integration and remote bootstrap tests passed on final HEAD.CmuxFoundation: four focused remote tmux session tests passed under arm64.zsh -n,bash -n, andfish -n: all three shell integration files passed syntax validation../scripts/lint-pbxproj-test-wiring.sh: passed for 648 test files../scripts/check-pbxproj.sh: passed.python3 scripts/check-workspace-package-groups.py --check: passed.python3 scripts/check-package-resolved-policy.py: passed.git diff --check origin/main...HEAD: passed.python3 scripts/swift_warning_budget.py --log /tmp/cmux-app-host-xcodebuild-untagged-attempt-1.log: passed with zero cmux-owned Swift warnings.scripts/swift_file_length_budget.pyand.github/swift-file-length-budget.tsvare absent from this checkout andorigin/main. No budget TSV was changed, and all newly added or previously untracked Swift files remain below 500 lines.e693ac2cfa; its isolated socket responded.Closes #9380