Repository navigation
iOS: registry-driven auto-attach (sign in → connected) - #6044
Closed
lawrencecchen wants to merge 9 commits into
Closed
lawrencecchen wants to merge 9 commits into
lawrencecchen wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThis PR implements registry-driven auto-attach for iOS: when a user signs in without a previously stored paired Mac, the system automatically selects and connects to a reachable Mac from the device registry using deterministic rules (presence-based filtering, recency-based selection, ambiguity detection) protected by generation-based cancellation to prevent stale attempts from interfering with user pairing actions. ChangesiOS Registry-Driven Auto-Attach Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileShellComposite
participant TargetSelector
participant Registry
participant ConnectPath
participant Presence
User->>MobileShellComposite: sign in (no stored Mac)
MobileShellComposite->>MobileShellComposite: reconnectActiveMacIfAvailable()
MobileShellComposite->>Registry: load team-scoped devices
Registry-->>MobileShellComposite: RegistryDevice[]
MobileShellComposite->>Presence: onlineDeviceIDs() async
Presence-->>MobileShellComposite: Set<String>? or nil
MobileShellComposite->>TargetSelector: selectTarget(devices, routes, presence)
alt exactly one online device (presence available)
TargetSelector-->>MobileShellComposite: Candidate
else zero/multiple online OR no presence
TargetSelector->>TargetSelector: use strict recency
TargetSelector-->>MobileShellComposite: Candidate or nil
end
alt Candidate selected
MobileShellComposite->>ConnectPath: connectToRegistryInstance(device, instance, rejectLoopback, supersedeAutoAttach: false)
ConnectPath->>ConnectPath: select best route (skip loopback if physical device)
ConnectPath-->>MobileShellComposite: connected ✓
MobileShellComposite->>MobileShellComposite: persist paired-Mac, resolve restoring gate
else No candidate
MobileShellComposite->>MobileShellComposite: resolve restoring gate
MobileShellComposite-->>User: present add-device pairing screen
end
User->>MobileShellComposite: tap to pair device manually
MobileShellComposite->>MobileShellComposite: supersedeInFlightAutoAttach()
MobileShellComposite->>ConnectPath: connectToRegistryInstance(device, instance, supersedeAutoAttach: true)
ConnectPath-->>MobileShellComposite: connected ✓
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 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 |
Contributor
Greptile SummaryAdds registry-driven auto-attach so a signed-in phone with no stored pairing can connect to its team's single obvious Mac without a QR scan, turning onboarding into "sign in → connected". The feature reuses
Confidence Score: 5/5Safe to merge. The auto-attach path is correctly gated (DEBUG-only in Release), bounded by the existing 6 s restoring deadline, and falls through to the pair screen on any ambiguity or failure — it cannot strand the user. All new code is well-guarded with per-attempt generation tokens, explicit supersession at every user-initiated entry point, and a conservative selector that returns nil on any ambiguity. The feature is off by default in Release builds, so production exposure during dogfood is intentionally minimal. The two comments left are documentation-quality nits that do not affect runtime behaviour. MobileShellComposite.swift has a merged doc-comment block attaching the entire section overview and cancelAutoAttach description to supersedeInFlightAutoAttach, and a nil-identityProvider edge case in the stillCurrent account-switch guard — both are doc/defensive-programming nits with no production-runtime impact. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[reconnectActiveMacIfAvailable] --> B{stored Mac?}
B -- yes --> C[normal stored-Mac reconnect]
B -- no --> D{autoAttachEnabled?}
D -- no --> E[setHasKnownPairedMac false
fall through to pair screen]
D -- yes --> F{autoAttachInFlight?}
F -- yes --> G[return false
dedupe: gate already owned]
F -- no --> H[runAutoAttachOwningRestoringGate
claims generation, sets RestoringSessionView]
H --> I[performAutoAttach
loadRegistryDevices fresh .ok only]
I --> J{registry .ok?}
J -- transient failure --> K[loadRegistryDevices UI cache
return false → pair screen]
J -- ok --> L[read presenceMap / autoAttachPresence]
L --> M[MobileAutoAttachTargetSelector.selectTarget]
M --> N{candidate?}
N -- nil ambiguous/no route --> O[return false → pair screen]
N -- single target --> P[connectToRegistryInstance
supersedeAutoAttach: false
rejectLoopback: isPhysicalDevice]
P --> Q{connected?}
Q -- yes --> R[pairedMacStore.upsert
loopback routes stripped
next launch: normal reconnect]
Q -- no --> O
H --> S{generation still current?}
S -- yes --> T[resolve restoring gate
hasKnownPairedMac = false if miss]
S -- no --> U[superseded: gate already resolved
by cancelAutoAttach or newer attempt]
V[User pairing / sign-out] --> W[supersedeInFlightAutoAttach
or cancelAutoAttach]
W --> X[bump autoAttachGeneration
clear running marker
resolve gate if owned]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[reconnectActiveMacIfAvailable] --> B{stored Mac?}
B -- yes --> C[normal stored-Mac reconnect]
B -- no --> D{autoAttachEnabled?}
D -- no --> E[setHasKnownPairedMac false
fall through to pair screen]
D -- yes --> F{autoAttachInFlight?}
F -- yes --> G[return false
dedupe: gate already owned]
F -- no --> H[runAutoAttachOwningRestoringGate
claims generation, sets RestoringSessionView]
H --> I[performAutoAttach
loadRegistryDevices fresh .ok only]
I --> J{registry .ok?}
J -- transient failure --> K[loadRegistryDevices UI cache
return false → pair screen]
J -- ok --> L[read presenceMap / autoAttachPresence]
L --> M[MobileAutoAttachTargetSelector.selectTarget]
M --> N{candidate?}
N -- nil ambiguous/no route --> O[return false → pair screen]
N -- single target --> P[connectToRegistryInstance
supersedeAutoAttach: false
rejectLoopback: isPhysicalDevice]
P --> Q{connected?}
Q -- yes --> R[pairedMacStore.upsert
loopback routes stripped
next launch: normal reconnect]
Q -- no --> O
H --> S{generation still current?}
S -- yes --> T[resolve restoring gate
hasKnownPairedMac = false if miss]
S -- no --> U[superseded: gate already resolved
by cancelAutoAttach or newer attempt]
V[User pairing / sign-out] --> W[supersedeInFlightAutoAttach
or cancelAutoAttach]
W --> X[bump autoAttachGeneration
clear running marker
resolve gate if owned]
Reviews (9): Last reviewed commit: "iOS tests: stop route-fallback test help..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+58
to
+65
| public static func selectTarget( | ||
| devices: [RegistryDevice], | ||
| supportedRouteKinds: [CmxAttachTransportKind], | ||
| presenceOnlineDeviceIDs: Set<String> = [], | ||
| presenceAvailable: Bool = false, | ||
| rejectLoopback: Bool = false, | ||
| now: Date = Date() | ||
| ) -> Candidate? { |
Contributor
There was a problem hiding this comment.
now parameter is declared in the public signature and passed by every call site (including performAutoAttach, which supplies runtime?.now() ?? Date()), but it is never forwarded to candidate(for:supportedRouteKinds:rejectLoopback:) or mostRecentUnambiguous(_:). Callers therefore believe they are influencing recency-based selection — e.g. an age cutoff for stale devices — when the value has no effect at all. If absolute-recency filtering is deferred, the parameter should either be removed now or annotated clearly so future implementors know it is intentionally a no-op.
Suggested change
| public static func selectTarget( | |
| devices: [RegistryDevice], | |
| supportedRouteKinds: [CmxAttachTransportKind], | |
| presenceOnlineDeviceIDs: Set<String> = [], | |
| presenceAvailable: Bool = false, | |
| rejectLoopback: Bool = false, | |
| now: Date = Date() | |
| ) -> Candidate? { | |
| public static func selectTarget( | |
| devices: [RegistryDevice], | |
| supportedRouteKinds: [CmxAttachTransportKind], | |
| presenceOnlineDeviceIDs: Set<String> = [], | |
| presenceAvailable: Bool = false, | |
| rejectLoopback: Bool = false | |
| ) -> Candidate? { |
Comment on lines
+1691
to
+1694
| analytics.capture("ios_auto_attach_attempt", [ | ||
| "candidate_device_count": .int(registryDevices.count), | ||
| "presence_available": .bool(presenceOnline != nil), | ||
| ]) |
Contributor
There was a problem hiding this comment.
loadRegistryDevices() has been called, so registryDevices.count reflects the UI cache from the previous load, not the freshDevices list that selectTarget just used to find a candidate. On a cold-start with no prior cache this reports 0; after a prior refresh it may report a count that doesn't match the set actually evaluated. Using freshDevices.count gives the accurate cardinality for this event.
Suggested change
| analytics.capture("ios_auto_attach_attempt", [ | |
| "candidate_device_count": .int(registryDevices.count), | |
| "presence_available": .bool(presenceOnline != nil), | |
| ]) | |
| analytics.capture("ios_auto_attach_attempt", [ | |
| "candidate_device_count": .int(freshDevices.count), | |
| "presence_available": .bool(presenceOnline != nil), | |
| ]) |
Comment on lines
+290
to
+294
| @Test func sequentialRetryAfterFailureIsAllowed() async throws { | ||
| // After an attempt finishes without connecting, the in-flight flag is | ||
| // cleared, so a later trigger may retry (the flag is not a permanent | ||
| // one-shot latch). First call: empty registry → no candidate → false. | ||
| // Then the same store, given a candidate registry, connects on retry. |
Contributor
There was a problem hiding this comment.
FakeRegistry is constructed with mac-A from the start and #expect(first) asserts true — the first call connects successfully. The comment appears to be left over from an earlier test design and now contradicts both the fixture and the assertion.
Suggested change
| @Test func sequentialRetryAfterFailureIsAllowed() async throws { | |
| // After an attempt finishes without connecting, the in-flight flag is | |
| // cleared, so a later trigger may retry (the flag is not a permanent | |
| // one-shot latch). First call: empty registry → no candidate → false. | |
| // Then the same store, given a candidate registry, connects on retry. | |
| @Test func sequentialRetryAfterFailureIsAllowed() async throws { | |
| // After an attempt finishes (with or without connecting), the in-flight | |
| // flag is cleared, so a later trigger may retry (the flag is not a | |
| // permanent one-shot latch). First call: registry has mac-A → connects | |
| // → true. Second call: already connected → short-circuits via the top | |
| // guard → false, proving the flag did not latch permanently. |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59bc671610
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Registry unreachable/unauthorized: degrade to manual. Still refresh | ||
| // the UI cache (it honors the same outcome semantics) so the device | ||
| // tree reflects the latest known state. | ||
| await loadRegistryDevices() |
There was a problem hiding this comment.
In the .authRejected / .transientFailure branch this awaits loadRegistryDevices(), which calls deviceRegistry.listDevices() again. When /api/devices is down or timing out, a fresh-install auto-attach pays the failed registry request twice before returning to the pair flow, keeping the auto-attach attempt in-flight and deduping other triggers after this code has already decided it will not connect. Refresh the device tree asynchronously or reuse the failed outcome instead of blocking the fallback path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1448-1711: Extract the entire auto-attach state machine (the logic
and state around
autoAttachGeneration/autoAttachRunningGeneration/autoAttachOwnsRestoringGate/autoAttachInFlight,
and the methods supersedeInFlightAutoAttach, cancelAutoAttach,
runAutoAttachOwningRestoringGate, attemptAutoAttachIfEligible,
beginAutoAttachGeneration, endAutoAttachGeneration, and performAutoAttach) into
a new helper class/struct AutoAttachCoordinator inside Packages/CmuxMobileShell;
move all generation/gate/deadline/connect orchestration there and keep
MobileShellComposite as a thin delegate that forwards calls and exposes only
simple lifecycle APIs (e.g. coordinator.supersedeInFlightAutoAttach(),
coordinator.attemptAutoAttachIfEligible(stackUserID:),
coordinator.runOwningRestoringGate(stackUserID:)). Ensure the new coordinator
receives required dependencies via initializer (deviceRegistry, analytics,
identityProvider, runtime, autoAttachPresence, functions or closures for
connectToRegistryInstance, cancelRemoteOperationTasks, loadRegistryDevices, and
accessors/mutators for
connectionState/connectionGeneration/hasKnownPairedMac/isSignedIn/isReconnectingStoredMac/didFinishStoredMacReconnectAttempt)
and preserves main-actor guarantees, generation guards, task cancellation, and
behavior (including restoringDeadline, presence handling, rejectLoopback logic,
and analytics events); update MobileShellComposite to call into the coordinator
and remove the moved state and methods from the composite.
- Around line 1592-1640: The auto-attach flow can start while a user-initiated
pairing/connect is already in flight, which lets the background attach overwrite
pairingAttemptID/connectionGeneration; fix by refusing to start auto-attach when
a user pairing is active: add a guard in attemptAutoAttachIfEligible (and/or at
the top of performAutoAttach's stillCurrent) that returns false if a user
pairing is in progress (check the existing pairingAttemptID or the
pairing-in-flight boolean used by connect/connectManualHost), so auto-attach
only proceeds when pairingAttemptID == nil (or pairing-in-flight == false) and
thus cannot supersede a user pairing attempt.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swift`:
- Around line 290-314: The test comment is incorrect about the first attempt:
the registry is initialized with a device (registry = FakeRegistry(devices:
[device(id: "mac-A", ...)])), so the first call to
sequentialRetryAfterFailureIsAllowed's store.attemptAutoAttachIfEligible(...)
actually succeeds (first == true). Update the comment above the test (the
paragraph starting "After an attempt finishes..." / the line "First call: ...")
to state that the registry contains a candidate so the first attempt connects
(expect true), and that the second sequential call is a no-op when already
connected (expect false); no code logic changes needed.
- Line 79: The count() method accesses the actor-isolated property macs but
isn’t marked async; change the signature func count() -> Int to func count()
async -> Int (or func count() async -> Int { macs.count }) and then update all
call sites (e.g., tests or helpers that invoke count()) to await the call and
make those callers async or wrap the call in Task/await, ensuring actor
isolation is preserved; reference: count() and the actor-isolated macs property.
🪄 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
Run ID: 8cafceaf-8d85-43d4-bb59-4f8a38eddcc0
📒 Files selected for processing (9)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileAutoAttachPresenceProviding.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAttachRoutePriority.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachFlag.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftplans/feat-ios-auto-attach/DESIGN.md
Comment on lines
+1448
to
+1711
| // MARK: - Registry-driven auto-attach | ||
|
|
||
| /// Registry-driven auto-attach: the "sign in → connected" onboarding path. | ||
| /// | ||
| /// When a signed-in phone has no stored pairing and is disconnected, this | ||
| /// loads the team-scoped device registry, picks the single obvious Mac (the | ||
| /// one online Mac, else the single most-recently-active Mac with a reachable | ||
| /// route), and connects to it via the same path the device-tree tap uses | ||
| /// (`connectToRegistryInstance`), which mints the attach ticket | ||
| /// Stack-authenticated on the Mac and persists the pairing. The persist means | ||
| /// the *next* launch takes the ordinary stored-mac reconnect path and never | ||
| /// re-runs auto-attach. | ||
| /// | ||
| /// Conservative and bounded by construction: | ||
| /// - Same-account only: the registry is scoped to the signed-in user's team, | ||
| /// and the Mac authorizes the mint on matching Stack account, so a | ||
| /// different-account Mac never appears and could never be connected. | ||
| /// Cancel any in-flight auto-attach attempt and supersede it by bumping the | ||
| /// generation, so its still-alive awaits discard their results and its | ||
| /// runner/deadline can no longer resolve the restoring gate or drive a | ||
| /// connect. Called on sign-out and on every user-initiated pairing, so a stale | ||
| /// background auto-attach never overrides the new account or the user's manual | ||
| /// pairing. | ||
| /// | ||
| /// When the superseded attempt was holding the launch restoring gate, this | ||
| /// resolves the gate here: the superseded runner's own cleanup is generation- | ||
| /// guarded and now a no-op, so without this a manual pairing that later fails | ||
| /// would leave RestoringSessionView stuck up. | ||
| /// Supersede any in-flight auto-attach on a USER-initiated pairing. | ||
| /// | ||
| /// Beyond `cancelAutoAttach` (which bumps the generation so a parked attempt's | ||
| /// awaits discard their results), this also invalidates the active pairing | ||
| /// attempt when an auto-attach is in flight. That covers the case where | ||
| /// auto-attach has ALREADY entered its own `connectManualHost` and minted a | ||
| /// `pairingAttemptID`: bumping the generation alone would not stop that | ||
| /// in-flight connect (its `isCurrentPairingAttempt` checks key off the | ||
| /// attempt id, not the generation), so a user's invalid manual submission that | ||
| /// returns before its own `beginPairingAttempt` could still be overridden by | ||
| /// the background connect. Regenerating the attempt id makes auto-attach's | ||
| /// in-flight connect bail at its next `isCurrentPairingAttempt` check. | ||
| /// | ||
| /// Guarded on `autoAttachInFlight` so it never clobbers an unrelated live | ||
| /// pairing attempt when no auto-attach is running. | ||
| private func supersedeInFlightAutoAttach() { | ||
| let wasInFlight = autoAttachInFlight | ||
| cancelAutoAttach() | ||
| guard wasInFlight else { return } | ||
| // Auto-attach may already be inside its own `connect()`, which installs the | ||
| // live client guarded by `connectionGeneration` (not `pairingAttemptID`), | ||
| // and whose `isCurrentPairingAttempt` check runs only AFTER it has set | ||
| // `connectionState = .connected`. Invalidating the pairing attempt alone | ||
| // would not stop that in-flight connect. Bump the connection generation and | ||
| // cancel in-flight remote work — the same supersession `beginPairingAttempt` | ||
| // performs — so auto-attach's running connect discards its result at its | ||
| // `generation == connectionGeneration` guard instead of landing over the | ||
| // user's explicit pairing. This runs BEFORE the validation guards in | ||
| // `connectManualHost`, so even an invalid user submission supersedes it. | ||
| invalidatePairingAttempt() | ||
| connectionGeneration = UUID() | ||
| cancelRemoteOperationTasks() | ||
| } | ||
|
|
||
| private func cancelAutoAttach() { | ||
| autoAttachGeneration &+= 1 | ||
| autoAttachRunningGeneration = nil | ||
| if autoAttachOwnsRestoringGate { | ||
| autoAttachOwnsRestoringGate = false | ||
| isReconnectingStoredMac = false | ||
| didFinishStoredMacReconnectAttempt = true | ||
| } | ||
| } | ||
|
|
||
| /// Runs an auto-attach attempt that owns the launch restoring gate | ||
| /// (``RestoringSessionView``) for its whole lifetime, then resolves it. | ||
| /// | ||
| /// Used from the no-stored-Mac reconnect branch. It holds the gate up while | ||
| /// auto-attach runs (so onboarding shows "Restoring session…" instead of a QR | ||
| /// flash), caps that window with the same bounded deadline the stored-Mac path | ||
| /// uses, and resolves the gate based on the per-attempt generation it owns — | ||
| /// not the stored-Mac reconnect generation, which a concurrent trigger may | ||
| /// bump. A newer auto-attach (or `cancelAutoAttach`) supersedes this one by | ||
| /// advancing ``autoAttachGeneration``, so a stale runner/deadline becomes a | ||
| /// no-op and cannot strand the gate or run a connect. | ||
| /// | ||
| /// - Returns: `true` only when a live connection landed under this attempt. | ||
| private func runAutoAttachOwningRestoringGate(stackUserID: String?) async -> Bool { | ||
| let generation = beginAutoAttachGeneration() | ||
| isReconnectingStoredMac = true | ||
| // Mark that an auto-attach runner holds the gate, so a user-initiated | ||
| // pairing that supersedes this attempt via `cancelAutoAttach` resolves the | ||
| // gate instead of leaving it stranded. | ||
| autoAttachOwnsRestoringGate = true | ||
| // Cap the restoring-gate window: a single stale/offline registry candidate | ||
| // makes the manual connect hang on its timeout, so without this deadline | ||
| // the gate would stay up for the whole registry + connect timeout and the | ||
| // fresh-install path would look hung instead of falling through to the | ||
| // pair sheet. The connect keeps running in the background, so a later | ||
| // success still flips to the workspaces; this only resolves the visible | ||
| // gate. Guarded by the per-attempt generation, so a superseded attempt's | ||
| // deadline can never clear a newer attempt's gate. Bounded and cancellable | ||
| // (not a poll) — cancelled the instant the attempt returns below. | ||
| let restoringDeadline = Task { [weak self] in | ||
| try? await ContinuousClock().sleep( | ||
| for: .seconds(Self.storedMacReconnectRestoringDeadlineSeconds) | ||
| ) | ||
| guard let self, !Task.isCancelled, | ||
| generation == self.autoAttachGeneration, | ||
| self.connectionState != .connected else { return } | ||
| self.isReconnectingStoredMac = false | ||
| self.didFinishStoredMacReconnectAttempt = true | ||
| } | ||
| let attached = await performAutoAttach(stackUserID: stackUserID, generation: generation) | ||
| restoringDeadline.cancel() | ||
| endAutoAttachGeneration(generation) | ||
| // Resolve the gate only if we are still the current attempt: a superseding | ||
| // attempt (or cancel) now owns the gate and will resolve it itself (the | ||
| // cancel path already did so via `cancelAutoAttach`). | ||
| if generation == autoAttachGeneration { | ||
| autoAttachOwnsRestoringGate = false | ||
| isReconnectingStoredMac = false | ||
| didFinishStoredMacReconnectAttempt = true | ||
| // This runner is the authoritative determiner for the no-stored-Mac | ||
| // path: it definitively found no stored Mac AND no auto-attach target. | ||
| // Clear the negative hint HERE (keyed on the auto-attach generation, | ||
| // which we still own) rather than relying on the caller's | ||
| // stored-mac-generation-guarded write, which a concurrent duplicate | ||
| // reconnect trigger can supersede — leaving `pairedMacHintUndetermined` | ||
| // unresolved and the restoring gate stuck on a fresh install. | ||
| if !attached, isSignedIn, connectionState != .connected { | ||
| hasKnownPairedMac = false | ||
| } | ||
| } | ||
| return attached | ||
| } | ||
|
|
||
| /// Public/test entry point for a one-shot auto-attach attempt that does NOT | ||
| /// own the restoring gate. | ||
| /// | ||
| /// Dedupes against any attempt already in flight: if one is running, returns | ||
| /// `false` immediately rather than racing a second registry load / destructive | ||
| /// connect, satisfying the one-attempt-at-a-time contract for every caller. | ||
| /// | ||
| /// - Returns: `true` only when a live connection landed under this attempt. | ||
| @discardableResult | ||
| func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool { | ||
| guard autoAttachEnabled, isSignedIn, connectionState != .connected else { return false } | ||
| guard !autoAttachInFlight else { return false } | ||
| let generation = beginAutoAttachGeneration() | ||
| defer { endAutoAttachGeneration(generation) } | ||
| return await performAutoAttach(stackUserID: stackUserID, generation: generation) | ||
| } | ||
|
|
||
| /// Claim the next auto-attach generation and mark it the in-flight attempt. | ||
| /// Synchronous (no await), so a concurrent caller observes `autoAttachInFlight` | ||
| /// true at its next main-actor hop and dedupes. | ||
| private func beginAutoAttachGeneration() -> Int { | ||
| autoAttachGeneration &+= 1 | ||
| let generation = autoAttachGeneration | ||
| autoAttachRunningGeneration = generation | ||
| return generation | ||
| } | ||
|
|
||
| /// Clear the in-flight marker when `generation` is still current, so a | ||
| /// superseded attempt does not erase a newer attempt's running marker. | ||
| private func endAutoAttachGeneration(_ generation: Int) { | ||
| guard generation == autoAttachGeneration else { return } | ||
| autoAttachRunningGeneration = nil | ||
| } | ||
|
|
||
| /// The shared auto-attach flow: load the registry, pick the single obvious | ||
| /// Mac, and connect to it via the proven registry connect path. | ||
| /// | ||
| /// Guarded by the per-attempt `generation`: after every suspension it bails | ||
| /// unless this is still the current attempt AND the same signed-in account, so | ||
| /// a task left alive by a sign-out/account switch or superseded by a newer | ||
| /// attempt can never resume and drive a destructive connect. This is the | ||
| /// equivalent of the stored-Mac path owning a pairing attempt id before its | ||
| /// connect, and mirrors the account-switch guard in ``loadPairedMacs`` / | ||
| /// ``loadRegistryDevices``. | ||
| /// | ||
| /// - Returns: `true` only when a live connection landed under this attempt. | ||
| private func performAutoAttach(stackUserID: String?, generation: Int) async -> Bool { | ||
| // Capture the requesting account; after any suspension the result is | ||
| // discarded unless this is still the same signed-in user and the current | ||
| // attempt. | ||
| let requestingUserID = stackUserID ?? identityProvider?.currentUserID | ||
| func stillCurrent() -> Bool { | ||
| generation == autoAttachGeneration | ||
| && isSignedIn | ||
| && connectionState != .connected | ||
| && identityProvider?.currentUserID == requestingUserID | ||
| } | ||
| guard autoAttachEnabled, stillCurrent(), let deviceRegistry else { return false } | ||
| // Auto-attach decides from a FRESHLY-confirmed registry list, not the | ||
| // store-wide `registryDevices` cache. `loadRegistryDevices()` deliberately | ||
| // keeps the prior cache on a transient failure (so a UI blip never blanks | ||
| // the device tree), but auto-attach must NOT connect off a stale list | ||
| // during a registry outage — the contract is to degrade to manual. So we | ||
| // read the outcome directly and proceed only on `.ok`; `.authRejected` / | ||
| // `.transientFailure` fall through to the pair screen. The UI cache is | ||
| // refreshed separately below so the tree still benefits from this load. | ||
| let outcome = await deviceRegistry.listDevices() | ||
| // The load suspended the main actor; bail if the user connected, signed | ||
| // out, switched accounts, or a newer attempt superseded this one. | ||
| guard stillCurrent() else { return false } | ||
| guard case let .ok(freshDevices) = outcome else { | ||
| // Registry unreachable/unauthorized: degrade to manual. Still refresh | ||
| // the UI cache (it honors the same outcome semantics) so the device | ||
| // tree reflects the latest known state. | ||
| await loadRegistryDevices() | ||
| return false | ||
| } | ||
| // Keep the UI device tree in sync with this fresh list. | ||
| await loadRegistryDevices() | ||
| guard stillCurrent() else { return false } | ||
|
|
||
| let presenceOnline = await autoAttachPresence?.onlineDeviceIDs() | ||
| guard stillCurrent() else { return false } | ||
|
|
||
| // On a physical phone, reject loopback routes: a `127.0.0.1` route names | ||
| // the phone itself, not the Mac, and loopback is Stack-auth-trusted, so | ||
| // auto-dialing it would fail and could hand the bearer to a phone-local | ||
| // listener. The simulator (where 127.0.0.1 IS the host Mac) keeps loopback. | ||
| let rejectLoopback = Self.isPhysicalDevice | ||
| guard let target = MobileAutoAttachTargetSelector.selectTarget( | ||
| devices: freshDevices, | ||
| supportedRouteKinds: runtime?.supportedRouteKinds ?? [], | ||
| presenceOnlineDeviceIDs: presenceOnline ?? [], | ||
| presenceAvailable: presenceOnline != nil, | ||
| rejectLoopback: rejectLoopback, | ||
| now: runtime?.now() ?? Date() | ||
| ) else { | ||
| return false | ||
| } | ||
|
|
||
| // Final guard immediately before the destructive connect: if a manual | ||
| // pairing began (which calls `cancelAutoAttach`), the user connected, or | ||
| // the account changed since the last await, do NOT start | ||
| // connectToRegistryInstance — that would invalidate the user's manual | ||
| // pairing attempt. `selectTarget` is synchronous, so this covers the | ||
| // window up to the connect. | ||
| guard stillCurrent() else { return false } | ||
|
|
||
| analytics.capture("ios_auto_attach_attempt", [ | ||
| "candidate_device_count": .int(registryDevices.count), | ||
| "presence_available": .bool(presenceOnline != nil), | ||
| ]) | ||
| // Reuse the proven registry connect path: it mints Stack-authenticated, | ||
| // rolls back to the previous active Mac on failure, and persists the | ||
| // pairing into the store on success. `supersedeAutoAttach: false` marks | ||
| // this as auto-attach's OWN connect (an explicit per-call parameter, not a | ||
| // shared marker), so the `beginPairingAttempt` inside it does not cancel | ||
| // auto-attach. A concurrent USER pairing during this connect carries | ||
| // `supersedeAutoAttach: true` on its own call and still supersedes us. | ||
| await connectToRegistryInstance( | ||
| device: target.device, | ||
| instance: target.instance, | ||
| rejectLoopback: rejectLoopback, | ||
| supersedeAutoAttach: false | ||
| ) | ||
| let connected = connectionState == .connected | ||
| analytics.capture("ios_auto_attach_result", ["connected": .bool(connected)]) | ||
| return connected | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract the new auto-attach state machine out of this store before it grows further.
This PR adds another large orchestration block to a production Swift file that is already far past the repo’s size threshold and already mixes connection lifecycle, registry caching, pairing, drafts, notifications, and terminal liveness. Please move the auto-attach generation/gate/connect flow into a dedicated helper/coordinator inside Packages/CmuxMobileShell and keep MobileShellComposite as the composition surface.
As per coding guidelines, production Swift files over 800 lines should be flagged when a PR adds more than 250 lines without extracting mixed responsibilities.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1448 - 1711, Extract the entire auto-attach state machine (the
logic and state around
autoAttachGeneration/autoAttachRunningGeneration/autoAttachOwnsRestoringGate/autoAttachInFlight,
and the methods supersedeInFlightAutoAttach, cancelAutoAttach,
runAutoAttachOwningRestoringGate, attemptAutoAttachIfEligible,
beginAutoAttachGeneration, endAutoAttachGeneration, and performAutoAttach) into
a new helper class/struct AutoAttachCoordinator inside Packages/CmuxMobileShell;
move all generation/gate/deadline/connect orchestration there and keep
MobileShellComposite as a thin delegate that forwards calls and exposes only
simple lifecycle APIs (e.g. coordinator.supersedeInFlightAutoAttach(),
coordinator.attemptAutoAttachIfEligible(stackUserID:),
coordinator.runOwningRestoringGate(stackUserID:)). Ensure the new coordinator
receives required dependencies via initializer (deviceRegistry, analytics,
identityProvider, runtime, autoAttachPresence, functions or closures for
connectToRegistryInstance, cancelRemoteOperationTasks, loadRegistryDevices, and
accessors/mutators for
connectionState/connectionGeneration/hasKnownPairedMac/isSignedIn/isReconnectingStoredMac/didFinishStoredMacReconnectAttempt)
and preserves main-actor guarantees, generation guards, task cancellation, and
behavior (including restoringDeadline, presence handling, rejectLoopback logic,
and analytics events); update MobileShellComposite to call into the coordinator
and remove the moved state and methods from the composite.
Source: Coding guidelines
Comment on lines
+1592
to
+1640
| func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool { | ||
| guard autoAttachEnabled, isSignedIn, connectionState != .connected else { return false } | ||
| guard !autoAttachInFlight else { return false } | ||
| let generation = beginAutoAttachGeneration() | ||
| defer { endAutoAttachGeneration(generation) } | ||
| return await performAutoAttach(stackUserID: stackUserID, generation: generation) | ||
| } | ||
|
|
||
| /// Claim the next auto-attach generation and mark it the in-flight attempt. | ||
| /// Synchronous (no await), so a concurrent caller observes `autoAttachInFlight` | ||
| /// true at its next main-actor hop and dedupes. | ||
| private func beginAutoAttachGeneration() -> Int { | ||
| autoAttachGeneration &+= 1 | ||
| let generation = autoAttachGeneration | ||
| autoAttachRunningGeneration = generation | ||
| return generation | ||
| } | ||
|
|
||
| /// Clear the in-flight marker when `generation` is still current, so a | ||
| /// superseded attempt does not erase a newer attempt's running marker. | ||
| private func endAutoAttachGeneration(_ generation: Int) { | ||
| guard generation == autoAttachGeneration else { return } | ||
| autoAttachRunningGeneration = nil | ||
| } | ||
|
|
||
| /// The shared auto-attach flow: load the registry, pick the single obvious | ||
| /// Mac, and connect to it via the proven registry connect path. | ||
| /// | ||
| /// Guarded by the per-attempt `generation`: after every suspension it bails | ||
| /// unless this is still the current attempt AND the same signed-in account, so | ||
| /// a task left alive by a sign-out/account switch or superseded by a newer | ||
| /// attempt can never resume and drive a destructive connect. This is the | ||
| /// equivalent of the stored-Mac path owning a pairing attempt id before its | ||
| /// connect, and mirrors the account-switch guard in ``loadPairedMacs`` / | ||
| /// ``loadRegistryDevices``. | ||
| /// | ||
| /// - Returns: `true` only when a live connection landed under this attempt. | ||
| private func performAutoAttach(stackUserID: String?, generation: Int) async -> Bool { | ||
| // Capture the requesting account; after any suspension the result is | ||
| // discarded unless this is still the same signed-in user and the current | ||
| // attempt. | ||
| let requestingUserID = stackUserID ?? identityProvider?.currentUserID | ||
| func stillCurrent() -> Bool { | ||
| generation == autoAttachGeneration | ||
| && isSignedIn | ||
| && connectionState != .connected | ||
| && identityProvider?.currentUserID == requestingUserID | ||
| } | ||
| guard autoAttachEnabled, stillCurrent(), let deviceRegistry else { return false } |
There was a problem hiding this comment.
Block auto-attach while a user pairing attempt is already in flight.
This entrypoint only gates on sign-in/connection/auto-attach state. If a QR/manual pairing is already awaiting connect(), a later reconnect/foreground trigger can still enter auto-attach. Once that background flow reaches its own connectManualHost(... supersedeAutoAttach: false), it overwrites pairingAttemptID and connectionGeneration, so the user-started pairing is deterministically superseded by the background attach.
Suggested fix
`@discardableResult`
func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool {
- guard autoAttachEnabled, isSignedIn, connectionState != .connected else { return false }
+ guard autoAttachEnabled,
+ isSignedIn,
+ connectionState != .connected,
+ pairingAttemptMethod == nil else { return false }
guard !autoAttachInFlight else { return false }
let generation = beginAutoAttachGeneration()
defer { endAutoAttachGeneration(generation) }
return await performAutoAttach(stackUserID: stackUserID, generation: generation)
}
@@
func stillCurrent() -> Bool {
generation == autoAttachGeneration
&& isSignedIn
&& connectionState != .connected
+ && pairingAttemptMethod == nil
&& identityProvider?.currentUserID == requestingUserID
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool { | |
| guard autoAttachEnabled, isSignedIn, connectionState != .connected else { return false } | |
| guard !autoAttachInFlight else { return false } | |
| let generation = beginAutoAttachGeneration() | |
| defer { endAutoAttachGeneration(generation) } | |
| return await performAutoAttach(stackUserID: stackUserID, generation: generation) | |
| } | |
| /// Claim the next auto-attach generation and mark it the in-flight attempt. | |
| /// Synchronous (no await), so a concurrent caller observes `autoAttachInFlight` | |
| /// true at its next main-actor hop and dedupes. | |
| private func beginAutoAttachGeneration() -> Int { | |
| autoAttachGeneration &+= 1 | |
| let generation = autoAttachGeneration | |
| autoAttachRunningGeneration = generation | |
| return generation | |
| } | |
| /// Clear the in-flight marker when `generation` is still current, so a | |
| /// superseded attempt does not erase a newer attempt's running marker. | |
| private func endAutoAttachGeneration(_ generation: Int) { | |
| guard generation == autoAttachGeneration else { return } | |
| autoAttachRunningGeneration = nil | |
| } | |
| /// The shared auto-attach flow: load the registry, pick the single obvious | |
| /// Mac, and connect to it via the proven registry connect path. | |
| /// | |
| /// Guarded by the per-attempt `generation`: after every suspension it bails | |
| /// unless this is still the current attempt AND the same signed-in account, so | |
| /// a task left alive by a sign-out/account switch or superseded by a newer | |
| /// attempt can never resume and drive a destructive connect. This is the | |
| /// equivalent of the stored-Mac path owning a pairing attempt id before its | |
| /// connect, and mirrors the account-switch guard in ``loadPairedMacs`` / | |
| /// ``loadRegistryDevices``. | |
| /// | |
| /// - Returns: `true` only when a live connection landed under this attempt. | |
| private func performAutoAttach(stackUserID: String?, generation: Int) async -> Bool { | |
| // Capture the requesting account; after any suspension the result is | |
| // discarded unless this is still the same signed-in user and the current | |
| // attempt. | |
| let requestingUserID = stackUserID ?? identityProvider?.currentUserID | |
| func stillCurrent() -> Bool { | |
| generation == autoAttachGeneration | |
| && isSignedIn | |
| && connectionState != .connected | |
| && identityProvider?.currentUserID == requestingUserID | |
| } | |
| guard autoAttachEnabled, stillCurrent(), let deviceRegistry else { return false } | |
| func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool { | |
| guard autoAttachEnabled, | |
| isSignedIn, | |
| connectionState != .connected, | |
| pairingAttemptMethod == nil else { return false } | |
| guard !autoAttachInFlight else { return false } | |
| let generation = beginAutoAttachGeneration() | |
| defer { endAutoAttachGeneration(generation) } | |
| return await performAutoAttach(stackUserID: stackUserID, generation: generation) | |
| } | |
| /// Claim the next auto-attach generation and mark it the in-flight attempt. | |
| /// Synchronous (no await), so a concurrent caller observes `autoAttachInFlight` | |
| /// true at its next main-actor hop and dedupes. | |
| private func beginAutoAttachGeneration() -> Int { | |
| autoAttachGeneration &+= 1 | |
| let generation = autoAttachGeneration | |
| autoAttachRunningGeneration = generation | |
| return generation | |
| } | |
| /// Clear the in-flight marker when `generation` is still current, so a | |
| /// superseded attempt does not erase a newer attempt's running marker. | |
| private func endAutoAttachGeneration(_ generation: Int) { | |
| guard generation == autoAttachGeneration else { return } | |
| autoAttachRunningGeneration = nil | |
| } | |
| /// The shared auto-attach flow: load the registry, pick the single obvious | |
| /// Mac, and connect to it via the proven registry connect path. | |
| /// | |
| /// Guarded by the per-attempt `generation`: after every suspension it bails | |
| /// unless this is still the current attempt AND the same signed-in account, so | |
| /// a task left alive by a sign-out/account switch or superseded by a newer | |
| /// attempt can never resume and drive a destructive connect. This is the | |
| /// equivalent of the stored-Mac path owning a pairing attempt id before its | |
| /// connect, and mirrors the account-switch guard in ``loadPairedMacs`` / | |
| /// ``loadRegistryDevices``. | |
| /// | |
| /// - Returns: `true` only when a live connection landed under this attempt. | |
| private func performAutoAttach(stackUserID: String?, generation: Int) async -> Bool { | |
| // Capture the requesting account; after any suspension the result is | |
| // discarded unless this is still the same signed-in user and the current | |
| // attempt. | |
| let requestingUserID = stackUserID ?? identityProvider?.currentUserID | |
| func stillCurrent() -> Bool { | |
| generation == autoAttachGeneration | |
| && isSignedIn | |
| && connectionState != .connected | |
| && pairingAttemptMethod == nil | |
| && identityProvider?.currentUserID == requestingUserID | |
| } | |
| guard autoAttachEnabled, stillCurrent(), let deviceRegistry else { return false } |
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1592 - 1640, The auto-attach flow can start while a user-initiated
pairing/connect is already in flight, which lets the background attach overwrite
pairingAttemptID/connectionGeneration; fix by refusing to start auto-attach when
a user pairing is active: add a guard in attemptAutoAttachIfEligible (and/or at
the top of performAutoAttach's stillCurrent) that returns false if a user
pairing is in progress (check the existing pairingAttemptID or the
pairing-in-flight boolean used by connect/connectManualHost), so auto-attach
only proceeds when pairingAttemptID == nil (or pairing-in-flight == false) and
thus cannot supersede a user pairing attempt.
|
|
||
| func removeAll() async throws { macs.removeAll() } | ||
|
|
||
| func count() -> Int { macs.count } |
There was a problem hiding this comment.
Make count() async to match actor isolation.
The count() method accesses the actor-isolated macs property but is not marked async. Actor-isolated methods must be async to maintain thread safety.
🔧 Proposed fix
- func count() -> Int { macs.count }
+ func count() async -> Int { macs.count }🤖 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
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swift`
at line 79, The count() method accesses the actor-isolated property macs but
isn’t marked async; change the signature func count() -> Int to func count()
async -> Int (or func count() async -> Int { macs.count }) and then update all
call sites (e.g., tests or helpers that invoke count()) to await the call and
make those callers async or wrap the call in Task/await, ensuring actor
isolation is preserved; reference: count() and the actor-isolated macs property.
Comment on lines
+290
to
+314
| @Test func sequentialRetryAfterFailureIsAllowed() async throws { | ||
| // After an attempt finishes without connecting, the in-flight flag is | ||
| // cleared, so a later trigger may retry (the flag is not a permanent | ||
| // one-shot latch). First call: empty registry → no candidate → false. | ||
| // Then the same store, given a candidate registry, connects on retry. | ||
| let clock = TestClock() | ||
| let route = try loopbackRoute() | ||
| let pairedStore = InMemoryPairedMacStore() | ||
| let registry = FakeRegistry(devices: [device(id: "mac-A", lastSeen: clock.now, route: route)]) | ||
| let store = makeStore( | ||
| devices: [], | ||
| pairedStore: pairedStore, | ||
| registry: registry, | ||
| clock: clock, | ||
| router: LivenessHostRouter(), | ||
| box: TransportBox() | ||
| ) | ||
|
|
||
| let first = await store.attemptAutoAttachIfEligible(stackUserID: "user-1") | ||
| #expect(first) | ||
| // A second sequential call when already connected is a no-op via the top | ||
| // guard, proving the in-flight flag did not latch permanently. | ||
| let second = await store.attemptAutoAttachIfEligible(stackUserID: "user-1") | ||
| #expect(!second) | ||
| } |
There was a problem hiding this comment.
Comment doesn't match test logic.
The comment at Line 293 states "First call: empty registry → no candidate → false", but the registry is initialized with mac-A at Line 298, so the first attempt succeeds (Line 309 expects first == true).
The test appears to validate that sequential calls are allowed when already connected (not retry-after-failure). Either update the comment to match the actual behavior, or change the test to match the comment's intent (create registry with empty devices, call once expecting false, then populate it and retry expecting true).
🤖 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
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swift`
around lines 290 - 314, The test comment is incorrect about the first attempt:
the registry is initialized with a device (registry = FakeRegistry(devices:
[device(id: "mac-A", ...)])), so the first call to
sequentialRetryAfterFailureIsAllowed's store.attemptAutoAttachIfEligible(...)
actually succeeds (first == true). Update the comment above the test (the
paragraph starting "After an attempt finishes..." / the line "First call: ...")
to state that the registry contains a candidate so the first attempt connects
(expect true), and that the second sequential call is a no-op when already
connected (expect false); no code logic changes needed.
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
…d mobileAutoAttach flag, #6044)
lawrencecchen
force-pushed
the
feat-ios-auto-attach
branch
from
June 14, 2026 07:13
59bc671 to
2161953
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 216195340c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+72
to
+73
| let online = candidates.filter { presenceOnlineDeviceIDs.contains($0.device.deviceId) } | ||
| if online.count == 1 { return online[0] } |
There was a problem hiding this comment.
When presence is available for a Mac that has multiple tagged cmux instances, this only filters by deviceId after candidate(for:) has already collapsed the device to the freshest registry instance. If the stable tag is currently online but a newer dev tag is stale/offline, the device is treated as the one online Mac and auto-attach attempts the stale dev route (or falls through to manual) instead of the live instance; pass/tag-filter per-instance presence before selecting the instance.
Useful? React with 👍 / 👎.
lawrencecchen
force-pushed
the
feat-ios-auto-attach
branch
from
June 14, 2026 07:19
2161953 to
adeecb5
Compare
lawrencecchen
force-pushed
the
feat-ios-auto-attach
branch
from
June 14, 2026 07:22
adeecb5 to
6b5fbad
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1596-1644:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuto-attach still ignores an active user pairing flow.
These guards only look at sign-in/connection state. The QR path now enters
beginPairingValidationAttempt()before approval/connect, so a foreground/recovery trigger can still start auto-attach while the user is sitting in validation or on the version-warning prompt, and that background path can then overwrite the user-owned attempt with its ownbeginPairingAttempt(...).Please gate auto-attach on a dedicated “user pairing in progress” state that covers validation, warning approval, and connect phases;
pairingAttemptMethodalone is too late for the split QR flow.🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1596 - 1644, The auto-attach guards in `attemptAutoAttachIfEligible` and the `stillCurrent()` check within `performAutoAttach` do not account for an active user-initiated pairing flow. Add a dedicated state variable to track when user pairing is in progress (covering validation, warning approval, and connect phases), and update the guard clauses in both `attemptAutoAttachIfEligible` and the `stillCurrent()` function to return false when this pairing state is active. This prevents the background auto-attach from overwriting a user-owned pairing attempt while the user is actively engaged in the QR validation or approval flow.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1653-1666: The code redundantly calls listDevices() twice in the
success path: once explicitly and then again inside loadRegistryDevices().
Extract the shared "sort and assign" logic from loadRegistryDevices() into a
separate helper function, then in the success path after obtaining freshDevices
from the guard case where .ok succeeds, apply freshDevices directly to
registryDevices using this new helper instead of calling loadRegistryDevices().
Keep loadRegistryDevices() for the error path only to maintain the fallback
refresh behavior. This eliminates the second registry round-trip in the critical
auto-attach path.
- Around line 1495-1511: The `supersedeInFlightAutoAttach()` function
invalidates the pairing attempt and rotates the `connectionGeneration` to
prevent stale auto-attach connects from landing over a user's explicit pairing
action, but it does not rotate the `connectionAttemptGeneration`. This means the
background connect can still pass the `isCurrentConnectionAttempt(...)` check
with a stale value if auto-attach is superseded before reaching its own
`beginPairingAttempt(...)`. Add a statement to rotate
`connectionAttemptGeneration` (assign it a new UUID, similar to the existing
`connectionGeneration = UUID()` line) in the `supersedeInFlightAutoAttach()`
function to ensure stale connection attempts are properly discarded.
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 123-129: The setHoldWorkspaceList method incorrectly drains the
shared heldContinuations queue when hold is false, resuming all held RPCs
instead of only workspace.list ones. This causes unrelated held continuations
for mobile.host.status and mobile.events.subscribe to be released prematurely.
Fix this by maintaining a separate tracking mechanism specifically for
workspace.list continuations (such as a dedicated workspaceListContinuations
array or similar) instead of using the shared heldContinuations queue. When
setHoldWorkspaceList(false) is called, only resume the continuations from this
dedicated workspace.list-specific collection, leaving other held continuations
intact.
---
Duplicate comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1596-1644: The auto-attach guards in `attemptAutoAttachIfEligible`
and the `stillCurrent()` check within `performAutoAttach` do not account for an
active user-initiated pairing flow. Add a dedicated state variable to track when
user pairing is in progress (covering validation, warning approval, and connect
phases), and update the guard clauses in both `attemptAutoAttachIfEligible` and
the `stillCurrent()` function to return false when this pairing state is active.
This prevents the background auto-attach from overwriting a user-owned pairing
attempt while the user is actively engaged in the QR validation or approval
flow.
🪄 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
Run ID: 65d533fc-ed3f-4d1b-9689-3d7733164943
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileAutoAttachPresenceProviding.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/PresenceMapTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAttachRoutePriority.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachFlag.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swift
Comment on lines
+1495
to
+1511
| private func supersedeInFlightAutoAttach() { | ||
| let wasInFlight = autoAttachInFlight | ||
| cancelAutoAttach() | ||
| guard wasInFlight else { return } | ||
| // Auto-attach may already be inside its own `connect()`, which installs the | ||
| // live client guarded by `connectionGeneration` (not `pairingAttemptID`), | ||
| // and whose `isCurrentPairingAttempt` check runs only AFTER it has set | ||
| // `connectionState = .connected`. Invalidating the pairing attempt alone | ||
| // would not stop that in-flight connect. Bump the connection generation and | ||
| // cancel in-flight remote work — the same supersession `beginPairingAttempt` | ||
| // performs — so auto-attach's running connect discards its result at its | ||
| // `generation == connectionGeneration` guard instead of landing over the | ||
| // user's explicit pairing. This runs BEFORE the validation guards in | ||
| // `connectManualHost`, so even an invalid user submission supersedes it. | ||
| invalidatePairingAttempt() | ||
| connectionGeneration = UUID() | ||
| cancelRemoteOperationTasks() |
There was a problem hiding this comment.
Invalidate connectionAttemptGeneration when superseding auto-attach.
connect(ticket:) now drops stale late results via isCurrentConnectionAttempt(...), but this helper only rotates connectionGeneration. If a manual/QR/device-tree action supersedes auto-attach before it reaches its own beginPairingAttempt(...), the background connect can still pass the current guard and install itself over the user’s action.
Suggested fix
invalidatePairingAttempt()
connectionGeneration = UUID()
+ connectionAttemptGeneration = UUID()
cancelRemoteOperationTasks()🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1495 - 1511, The `supersedeInFlightAutoAttach()` function
invalidates the pairing attempt and rotates the `connectionGeneration` to
prevent stale auto-attach connects from landing over a user's explicit pairing
action, but it does not rotate the `connectionAttemptGeneration`. This means the
background connect can still pass the `isCurrentConnectionAttempt(...)` check
with a stale value if auto-attach is superseded before reaching its own
`beginPairingAttempt(...)`. Add a statement to rotate
`connectionAttemptGeneration` (assign it a new UUID, similar to the existing
`connectionGeneration = UUID()` line) in the `supersedeInFlightAutoAttach()`
function to ensure stale connection attempts are properly discarded.
Comment on lines
+1653
to
+1666
| let outcome = await deviceRegistry.listDevices() | ||
| // The load suspended the main actor; bail if the user connected, signed | ||
| // out, switched accounts, or a newer attempt superseded this one. | ||
| guard stillCurrent() else { return false } | ||
| guard case let .ok(freshDevices) = outcome else { | ||
| // Registry unreachable/unauthorized: degrade to manual. Still refresh | ||
| // the UI cache (it honors the same outcome semantics) so the device | ||
| // tree reflects the latest known state. | ||
| await loadRegistryDevices() | ||
| return false | ||
| } | ||
| // Keep the UI device tree in sync with this fresh list. | ||
| await loadRegistryDevices() | ||
| guard stillCurrent() else { return false } |
There was a problem hiding this comment.
Avoid the second registry round-trip in the auto-attach critical path.
After listDevices() succeeds, this path immediately awaits loadRegistryDevices(), which performs another deviceRegistry.listDevices() before selection/connect. On a flow explicitly bounded by a 6s restoring window, that extra RTT can turn a reachable cold-start attach into a fall-through to the pair screen.
Apply freshDevices to registryDevices through a shared “sort + assign” helper and keep loadRegistryDevices() for the failure path only.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1653 - 1666, The code redundantly calls listDevices() twice in the
success path: once explicitly and then again inside loadRegistryDevices().
Extract the shared "sort and assign" logic from loadRegistryDevices() into a
separate helper function, then in the success path after obtaining freshDevices
from the guard case where .ok succeeds, apply freshDevices directly to
registryDevices using this new helper instead of calling loadRegistryDevices().
Keep loadRegistryDevices() for the error path only to maintain the fallback
refresh behavior. This eliminates the second registry round-trip in the critical
auto-attach path.
Comment on lines
+123
to
+129
| func setHoldWorkspaceList(_ hold: Bool) { | ||
| holdWorkspaceList = hold | ||
| if !hold { | ||
| let continuations = heldContinuations | ||
| heldContinuations = [] | ||
| for continuation in continuations { continuation.resume() } | ||
| } |
There was a problem hiding this comment.
setHoldWorkspaceList(false) currently releases unrelated held RPCs.
On Line 126, this drains the shared heldContinuations queue, so unholding workspace.list can also resume intentionally held mobile.host.status / mobile.events.subscribe requests. That breaks the test harness contract for dead-stream scenarios and can mask real regressions.
Suggested fix
actor LivenessHostRouter {
@@
- private var heldContinuations: [CheckedContinuation<Void, Never>] = []
+ private var heldContinuations: [CheckedContinuation<Void, Never>] = []
+ private var heldWorkspaceListContinuations: [CheckedContinuation<Void, Never>] = []
@@
func setHoldWorkspaceList(_ hold: Bool) {
holdWorkspaceList = hold
if !hold {
- let continuations = heldContinuations
- heldContinuations = []
+ let continuations = heldWorkspaceListContinuations
+ heldWorkspaceListContinuations = []
for continuation in continuations { continuation.resume() }
}
}
@@
- if holdWorkspaceList {
- await park()
+ if holdWorkspaceList {
+ await parkWorkspaceList()
}
@@
+ private func parkWorkspaceList() async {
+ await withCheckedContinuation { continuation in
+ heldWorkspaceListContinuations.append(continuation)
+ }
+ }
+
private func park() async {
await withCheckedContinuation { continuation in
heldContinuations.append(continuation)
}
}
@@
func releaseAllHeld() {
holdSubscribe = false
heldHostStatusRequestNumbers = []
heldSubscribeRequestNumbers = []
let continuations = heldContinuations
+ let workspaceContinuations = heldWorkspaceListContinuations
heldContinuations = []
+ heldWorkspaceListContinuations = []
for continuation in continuations {
continuation.resume()
}
+ for continuation in workspaceContinuations {
+ continuation.resume()
+ }
}
}🤖 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
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`
around lines 123 - 129, The setHoldWorkspaceList method incorrectly drains the
shared heldContinuations queue when hold is false, resuming all held RPCs
instead of only workspace.list ones. This causes unrelated held continuations
for mobile.host.status and mobile.events.subscribe to be released prematurely.
Fix this by maintaining a separate tracking mechanism specifically for
workspace.list continuations (such as a dedicated workspaceListContinuations
array or similar) instead of using the shared heldContinuations queue. When
setHoldWorkspaceList(false) is called, only resume the continuations from this
dedicated workspace.list-specific collection, leaving other held continuations
intact.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b5fbad385
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !supported.isEmpty, !supported.contains(route.kind) { | ||
| continue | ||
| } | ||
| if rejectLoopback, MobileShellRouteAuthPolicy.routeIsLoopback(route) { |
There was a problem hiding this comment.
When rejectLoopback is true on a physical phone, this calls the narrower MobileShellRouteAuthPolicy.routeIsLoopback, which misses loopback spellings that the shared CmxLoopbackHost classifier handles (for example a registry route whose host is [::1]). In that case auto-attach can still select the route; connectManualHost then normalizes it to ::1 and treats it as Stack-auth-trusted, so the phone can dial its own localhost instead of the Mac. Use the same loopback classifier as the ticket/QR rejection path here (and for the later persisted-route filter) so all self-dialing routes are skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift (1)
123-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setHoldWorkspaceList(false)releases unrelated held RPCs.On Line 126, draining
heldContinuationsresumes holds for non-workspace.listrequests too (mobile.host.status/mobile.events.subscribe), which breaks dead-stream test isolation.Suggested minimal fix
actor LivenessHostRouter { @@ private var heldContinuations: [CheckedContinuation<Void, Never>] = [] + private var heldWorkspaceListContinuations: [CheckedContinuation<Void, Never>] = [] @@ func setHoldWorkspaceList(_ hold: Bool) { holdWorkspaceList = hold if !hold { - let continuations = heldContinuations - heldContinuations = [] + let continuations = heldWorkspaceListContinuations + heldWorkspaceListContinuations = [] for continuation in continuations { continuation.resume() } } } @@ case "workspace.list", "mobile.workspace.list": if holdWorkspaceList { - await park() + await parkWorkspaceList() } @@ + private func parkWorkspaceList() async { + await withCheckedContinuation { continuation in + heldWorkspaceListContinuations.append(continuation) + } + } + private func park() async { await withCheckedContinuation { continuation in heldContinuations.append(continuation) } } @@ func releaseAllHeld() { @@ let continuations = heldContinuations + let workspaceContinuations = heldWorkspaceListContinuations heldContinuations = [] + heldWorkspaceListContinuations = [] for continuation in continuations { continuation.resume() } + for continuation in workspaceContinuations { + continuation.resume() + } } }🤖 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 `@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift` around lines 123 - 129, The setHoldWorkspaceList(_:) method currently drains all held continuations when hold is set to false, but it should only resume continuations that are specifically for workspace.list requests. To fix this, you need to separate the tracking of workspace.list continuations from other held continuations (like mobile.host.status and mobile.events.subscribe). Modify the data structure to maintain a separate storage for workspace.list continuations (you may need to add a new property similar to heldContinuations but specifically for workspace.list requests), and then in the setHoldWorkspaceList method, only iterate over and resume the workspace.list-specific continuations rather than all held continuations.
🤖 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
`@Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swift`:
- Line 20: Replace all force-try statements (try!) in the test fixture setup
code with proper error handling to improve test reliability and diagnostics.
When initializing CmxAttachRoute and other fixtures that can fail, either wrap
the try calls in a do-catch block and call preconditionFailure with the error
details if setup fails, or convert the affected test functions to throwing
functions (marked with throws) to allow errors to propagate and fail the tests
appropriately. This approach prevents silent crashes during test setup and
provides better error messages when fixtures fail to initialize.
---
Duplicate comments:
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 123-129: The setHoldWorkspaceList(_:) method currently drains all
held continuations when hold is set to false, but it should only resume
continuations that are specifically for workspace.list requests. To fix this,
you need to separate the tracking of workspace.list continuations from other
held continuations (like mobile.host.status and mobile.events.subscribe). Modify
the data structure to maintain a separate storage for workspace.list
continuations (you may need to add a new property similar to heldContinuations
but specifically for workspace.list requests), and then in the
setHoldWorkspaceList method, only iterate over and resume the
workspace.list-specific continuations rather than all held continuations.
🪄 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
Run ID: 45001d1f-b97a-4a2e-accc-b56dca0b6a67
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (11)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileAutoAttachPresenceProviding.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/PresenceMapTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAttachRoutePriority.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachFlag.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftplans/feat-ios-auto-attach/DESIGN.md
| port: Int = 58_465, | ||
| priority: Int = 0 | ||
| ) -> CmxAttachRoute { | ||
| try! CmxAttachRoute( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no force-try remains in this test file after applying the fix.
rg -n '\btry!' Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftRepository: manaflow-ai/cmux
Length of output: 337
🏁 Script executed:
find .github/review-bot-rules -type f -name "*.md" | head -20Repository: manaflow-ai/cmux
Length of output: 954
🏁 Script executed:
rg -l "force.try|try!" .github/review-bot-rules/ 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cat Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swift | head -80Repository: manaflow-ai/cmux
Length of output: 2862
🏁 Script executed:
cat .github/review-bot-rules/swift-concurrency-modernization.md | head -100Repository: manaflow-ai/cmux
Length of output: 1703
🏁 Script executed:
cat .github/review-bot-rules/swift-logging.mdRepository: manaflow-ai/cmux
Length of output: 1856
🏁 Script executed:
ls -la .github/review-bot-rules/ | grep -i swiftRepository: manaflow-ai/cmux
Length of output: 843
🏁 Script executed:
rg "force.try" . --type md 2>/dev/null | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg "try!" .github/review-bot-rules/ --type mdRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cat .github/review-bot-rules/README.mdRepository: manaflow-ai/cmux
Length of output: 1442
🏁 Script executed:
wc -l Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftRepository: manaflow-ai/cmux
Length of output: 168
🏁 Script executed:
cat Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swift | sed -n '170,220p'Repository: manaflow-ai/cmux
Length of output: 2050
🏁 Script executed:
cat .github/review-bot-rules/*.md | grep -i "force\|try!" -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 703
🏁 Script executed:
grep -r "force_try\|force-try" .github --include="*.md" --include="*.json" --include="*.yaml"Repository: manaflow-ai/cmux
Length of output: 42
Replace force-tries in tests with proper error handling for better fixture reliability.
Test fixtures currently use try! at lines 20, 64, 177, 205, and 211, which can crash setup if route initialization fails and makes test failures harder to diagnose. Convert these to proper error handling with preconditionFailure or make test functions throws.
Suggested patch
@@
private func route(
@@
) -> CmxAttachRoute {
- try! CmxAttachRoute(
- id: id,
- kind: kind,
- endpoint: .hostPort(host: host, port: port),
- priority: priority
- )
+ do {
+ return try CmxAttachRoute(
+ id: id,
+ kind: kind,
+ endpoint: .hostPort(host: host, port: port),
+ priority: priority
+ )
+ } catch {
+ preconditionFailure("Invalid test route fixture: \(error)")
+ }
}
@@
- `@Test` func noCandidateWhenNoReachableRoute() {
+ `@Test` func noCandidateWhenNoReachableRoute() throws {
@@
- let wsRoute = try! CmxAttachRoute(id: "ws", kind: .websocket, endpoint: .url("wss://x"))
+ let wsRoute = try CmxAttachRoute(id: "ws", kind: .websocket, endpoint: .url("wss://x"))
@@
- `@Test` func rejectLoopbackSkipsLoopbackOnlyDevicesForPhysicalPhone() {
+ `@Test` func rejectLoopbackSkipsLoopbackOnlyDevicesForPhysicalPhone() throws {
@@
- let loopback = try! CmxAttachRoute(
+ let loopback = try CmxAttachRoute(
id: "loop",
kind: .debugLoopback,
endpoint: .hostPort(host: "127.0.0.1", port: 56_584)
)
@@
- `@Test` func rejectLoopbackStillPicksTailscaleRouteForPhysicalPhone() {
+ `@Test` func rejectLoopbackStillPicksTailscaleRouteForPhysicalPhone() throws {
@@
- let loopback = try! CmxAttachRoute(
+ let loopback = try CmxAttachRoute(
id: "loop",
kind: .debugLoopback,
endpoint: .hostPort(host: "127.0.0.1", port: 56_584),
priority: 0
)
- let tailscale = try! CmxAttachRoute(
+ let tailscale = try CmxAttachRoute(
id: "ts",
kind: .tailscale,
endpoint: .hostPort(host: "100.71.0.5", port: 56_584),
priority: 1
)🧰 Tools
🪛 SwiftLint (0.63.3)
[Error] 20-20: Force tries should be avoided
(force_try)
🤖 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
`@Packages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swift`
at line 20, Replace all force-try statements (try!) in the test fixture setup
code with proper error handling to improve test reliability and diagnostics.
When initializing CmxAttachRoute and other fixtures that can fail, either wrap
the try calls in a do-catch block and call preconditionFailure with the error
details if setup fails, or convert the affected test functions to throwing
functions (marked with throws) to allow errors to propagate and fail the tests
appropriately. This approach prevents silent crashes during test setup and
provides better error messages when fixtures fail to initialize.
Source: Linters/SAST tools
lawrencecchen
force-pushed
the
feat-ios-auto-attach
branch
from
June 14, 2026 08:42
6b5fbad to
bf96d10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf96d105a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // user's explicit pairing. This runs BEFORE the validation guards in | ||
| // `connectManualHost`, so even an invalid user submission supersedes it. | ||
| invalidatePairingAttempt() | ||
| connectionGeneration = UUID() |
There was a problem hiding this comment.
When an auto-attach has already passed into connect() and a user action supersedes it but then returns before starting its own connect (for example, tapping a registry instance with no reachable route), this path only changes connectionGeneration. The pending workspace.list in connect() is guarded by isCurrentConnectionAttempt(...) / connectionAttemptGeneration, which remains the auto-attach generation unless a later validation failure calls clearRemoteConnectionContext(), so the background attach can still install its client and connect over the user's explicit no-op. Bump connectionAttemptGeneration here as well (or use the same context clear as beginPairingAttempt).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (4)
1495-1511:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRotate
connectionAttemptGenerationhere too.Line 1510 only bumps
connectionGeneration.connect(ticket:)drops stale late results throughisCurrentConnectionAttempt(...), which still keys offconnectionAttemptGeneration, so an auto-attach already insideconnect()can still land after a user pairing supersedes it.Proposed fix
invalidatePairingAttempt() connectionGeneration = UUID() + connectionAttemptGeneration = UUID() cancelRemoteOperationTasks()🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1495 - 1511, In the supersedeInFlightAutoAttach() method, add rotation of the connectionAttemptGeneration property in addition to the existing connectionGeneration rotation. The connect(ticket:) method validates results using isCurrentConnectionAttempt(...) which checks connectionAttemptGeneration, so rotating only connectionGeneration is insufficient to prevent in-flight auto-attach results from landing after a user pairing supersedes it. Add a line after connectionGeneration = UUID() to also set connectionAttemptGeneration to a new UUID() value, ensuring all generation checks properly invalidate stale auto-attach operations.
1596-1601:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock auto-attach while a user pairing attempt is active.
Line 1597 only gates on sign-in/connected state. Once a manual/QR pairing reaches its first await, a later reconnect or foreground trigger can still start auto-attach, and the auto-attach path's
beginPairingAttempt(...)then overwrites the explicit user pairing state. Add the pairing-in-flight guard at the entry point and instillCurrent()so background attach cannot supersede an active user connect.Proposed fix
`@discardableResult` func attemptAutoAttachIfEligible(stackUserID: String?) async -> Bool { - guard autoAttachEnabled, isSignedIn, connectionState != .connected else { return false } + guard autoAttachEnabled, + isSignedIn, + connectionState != .connected, + pairingAttemptMethod == nil else { return false } guard !autoAttachInFlight else { return false } let generation = beginAutoAttachGeneration() defer { endAutoAttachGeneration(generation) } return await performAutoAttach(stackUserID: stackUserID, generation: generation) } @@ func stillCurrent() -> Bool { generation == autoAttachGeneration && isSignedIn && connectionState != .connected + && pairingAttemptMethod == nil && identityProvider?.currentUserID == requestingUserID }Also applies to: 1638-1643
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1596 - 1601, The attemptAutoAttachIfEligible function allows auto-attach to proceed even when a manual or QR pairing attempt is active, causing the auto-attach path's beginPairingAttempt call to overwrite the explicit user pairing state. Add a guard clause after line 1597 that returns false if a pairing attempt is already in flight (check for a pairingInFlight or similar flag), preventing auto-attach from starting when a user-initiated pairing is active. Apply the same guard addition at the other affected location noted at lines 1638-1643, which is likely in the stillCurrent method, to ensure background auto-attach cannot supersede an active user connect attempt.
1653-1666:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the second registry round-trip in the auto-attach success path.
Lines 1653-1666 call
deviceRegistry.listDevices()and then immediately awaitloadRegistryDevices(), which performs anotherlistDevices()before selection/connect continues. On a flow explicitly bounded by the 6-second restoring window, that extra RTT can turn a reachable cold-start attach into a fall-through to pairing. ApplyfreshDevicestoregistryDevicesthrough a shared sort/assign helper, and keeploadRegistryDevices()only for the failure path.🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1653 - 1666, The code performs two consecutive registry calls: first via deviceRegistry.listDevices() to obtain freshDevices, then again via loadRegistryDevices(). In the success path where freshDevices is available, extract the sort/assign logic from loadRegistryDevices() into a shared helper function, apply freshDevices to registryDevices directly using this helper (removing the redundant loadRegistryDevices() call after the guard case .ok check), and keep loadRegistryDevices() only for the error path where the registry is unreachable or unauthorized. This eliminates the unnecessary second round-trip while maintaining the same update semantics for the UI device tree.
1452-1727: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the auto-attach coordinator out of
MobileShellComposite.This PR adds another large orchestration block to a production Swift file that is already far past the repo’s size threshold and already mixes connection lifecycle, registry syncing, pairing, notifications, and terminal state. Please move the auto-attach generation/gate/connect flow into a dedicated helper/coordinator and keep
MobileShellCompositeas the composition surface.As per coding guidelines, production Swift files over 800 lines should be flagged when a PR adds more than 250 lines without extracting mixed responsibilities.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1452 - 1727, Extract the auto-attach generation and connection orchestration logic from MobileShellComposite into a dedicated coordinator class. Create a new helper/coordinator that manages the auto-attach lifecycle: move the methods supersedeInFlightAutoAttach(), cancelAutoAttach(), runAutoAttachOwningRestoringGate(), attemptAutoAttachIfEligible(), beginAutoAttachGeneration(), endAutoAttachGeneration(), and performAutoAttach() from MobileShellComposite into this coordinator, along with their associated state properties (autoAttachGeneration, autoAttachRunningGeneration, autoAttachOwnsRestoringGate, etc.). Then update MobileShellComposite to delegate auto-attach operations to an instance of this coordinator, keeping the file focused on composition rather than mixing connection lifecycle, registry syncing, pairing, notifications, and terminal state management.Source: Coding guidelines
🤖 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
`@Packages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swift`:
- Around line 58-65: The `selectTarget` function in
MobileAutoAttachTargetSelector.swift declares an unused `now: Date = Date()`
parameter that is never referenced in the function body. Remove this parameter
from the function signature. Additionally, update the corresponding call site in
MobileShellComposite.swift (line 1691) that passes `runtime?.now() ?? Date()` as
the `now:` argument to remove that argument entirely, since it is no longer
accepted by the function.
---
Duplicate comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1495-1511: In the supersedeInFlightAutoAttach() method, add
rotation of the connectionAttemptGeneration property in addition to the existing
connectionGeneration rotation. The connect(ticket:) method validates results
using isCurrentConnectionAttempt(...) which checks connectionAttemptGeneration,
so rotating only connectionGeneration is insufficient to prevent in-flight
auto-attach results from landing after a user pairing supersedes it. Add a line
after connectionGeneration = UUID() to also set connectionAttemptGeneration to a
new UUID() value, ensuring all generation checks properly invalidate stale
auto-attach operations.
- Around line 1596-1601: The attemptAutoAttachIfEligible function allows
auto-attach to proceed even when a manual or QR pairing attempt is active,
causing the auto-attach path's beginPairingAttempt call to overwrite the
explicit user pairing state. Add a guard clause after line 1597 that returns
false if a pairing attempt is already in flight (check for a pairingInFlight or
similar flag), preventing auto-attach from starting when a user-initiated
pairing is active. Apply the same guard addition at the other affected location
noted at lines 1638-1643, which is likely in the stillCurrent method, to ensure
background auto-attach cannot supersede an active user connect attempt.
- Around line 1653-1666: The code performs two consecutive registry calls: first
via deviceRegistry.listDevices() to obtain freshDevices, then again via
loadRegistryDevices(). In the success path where freshDevices is available,
extract the sort/assign logic from loadRegistryDevices() into a shared helper
function, apply freshDevices to registryDevices directly using this helper
(removing the redundant loadRegistryDevices() call after the guard case .ok
check), and keep loadRegistryDevices() only for the error path where the
registry is unreachable or unauthorized. This eliminates the unnecessary second
round-trip while maintaining the same update semantics for the UI device tree.
- Around line 1452-1727: Extract the auto-attach generation and connection
orchestration logic from MobileShellComposite into a dedicated coordinator
class. Create a new helper/coordinator that manages the auto-attach lifecycle:
move the methods supersedeInFlightAutoAttach(), cancelAutoAttach(),
runAutoAttachOwningRestoringGate(), attemptAutoAttachIfEligible(),
beginAutoAttachGeneration(), endAutoAttachGeneration(), and performAutoAttach()
from MobileShellComposite into this coordinator, along with their associated
state properties (autoAttachGeneration, autoAttachRunningGeneration,
autoAttachOwnsRestoringGate, etc.). Then update MobileShellComposite to delegate
auto-attach operations to an instance of this coordinator, keeping the file
focused on composition rather than mixing connection lifecycle, registry
syncing, pairing, notifications, and terminal state management.
🪄 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
Run ID: 9db5b599-8ef8-48e1-a74c-2c9b43c45ac7
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (11)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileAutoAttachPresenceProviding.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/PresenceMapTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAttachRoutePriority.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachFlag.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftplans/feat-ios-auto-attach/DESIGN.md
Comment on lines
+58
to
+65
| public static func selectTarget( | ||
| devices: [RegistryDevice], | ||
| supportedRouteKinds: [CmxAttachTransportKind], | ||
| presenceOnlineDeviceIDs: Set<String> = [], | ||
| presenceAvailable: Bool = false, | ||
| rejectLoopback: Bool = false, | ||
| now: Date = Date() | ||
| ) -> Candidate? { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove unused now parameter or document its purpose.
The now: Date = Date() parameter is declared but never referenced in the function body. The caller in MobileShellComposite.swift (line 1691) passes runtime?.now() ?? Date(), but the function performs no time-based logic. Either remove the parameter entirely, or if it's reserved for future timestamp-based filtering, add a comment explaining why it's currently unused.
♻️ Remove unused parameter
public static func selectTarget(
devices: [RegistryDevice],
supportedRouteKinds: [CmxAttachTransportKind],
presenceOnlineDeviceIDs: Set<String> = [],
presenceAvailable: Bool = false,
- rejectLoopback: Bool = false,
- now: Date = Date()
+ rejectLoopback: Bool = false
) -> Candidate? {Note: This also requires updating the call site in MobileShellComposite.swift to remove the now: argument.
🤖 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
`@Packages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swift`
around lines 58 - 65, The `selectTarget` function in
MobileAutoAttachTargetSelector.swift declares an unused `now: Date = Date()`
parameter that is never referenced in the function body. Remove this parameter
from the function signature. Additionally, update the corresponding call site in
MobileShellComposite.swift (line 1691) that passes `runtime?.now() ?? Date()` as
the `now:` argument to remove that argument entirely, since it is no longer
accepted by the function.
A signed-in phone whose team has one reachable Mac now connects on the cold-start path with no QR scan or manual host entry. The attach ticket is route-discovery only; the Mac authorizes the mint purely on matching Stack account, so a registry route + Stack token is sufficient (no prior pairing). - MobileAutoAttachTargetSelector: pure target picker (online-preferred, else single most-recently-seen with a reachable route; ambiguity → nil → manual). - MobileAttachRoutePriority: shared route-priority helper (single source of truth for reconnect, switch, device-tree tap, auto-attach). - MobileAutoAttachFlag: mobileAutoAttach flag, DEBUG on / Release off, override via cmux.mobile.autoAttach.enabled. - attemptAutoAttachIfEligible chains from the no-stored-mac reconnect branch; bounded, cancellable, one attempt per generation; reuses connectToRegistryInstance (Stack-authenticated mint + paired-mac persist) so the next launch takes the normal reconnect path. - Presence seam optional (MobileAutoAttachPresenceProviding); degrades to recency until presence (#5792) lands. Tests: pure selector (online>recency, no-candidate, ambiguity) + composite behavior (connects+persists, fall-through, one-attempt-per-generation, flag off, already-connected). swift test green for CmuxMobileShellModel and CmuxMobileShell; iOS simulator arm64 build succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerated from the post-rebase tree so the CI guard reflects real counts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Main's iOS package-conventions lint now flags all-static public types (both caseless namespace enums and static-only structs) repo-wide. MobileAutoAttachFlag, MobileAttachRoutePriority, and MobileAutoAttachTargetSelector are pure, stateless policy namespaces: deterministic transforms over values passed per call, no I/O and no injected dependencies, so there is nothing to instantiate. Carry the sanctioned inline lint:allow justification rather than forcing an artificial instance type onto pure functions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-auto-attach
branch
from
June 14, 2026 09:33
bf96d10 to
ed632e5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed632e5e3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false | ||
| } | ||
| // Keep the UI device tree in sync with this fresh list. | ||
| await loadRegistryDevices() |
There was a problem hiding this comment.
Even after deviceRegistry.listDevices() has already returned .ok(freshDevices), this awaits loadRegistryDevices(), which issues another /api/devices request before target selection. If the first call succeeds with a usable Mac but the second request is slow or times out, fresh-install auto-attach waits behind a redundant network fetch and can drop the restoring gate/fall back to QR even though it already had a fresh candidate; update the UI cache from freshDevices or refresh it asynchronously before continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift (1)
123-129:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
setHoldWorkspaceList(false)currently releases unrelated held RPCs.On Line 126, this drains the shared
heldContinuationsqueue, so unholdingworkspace.listcan also resume intentionally heldmobile.host.status/mobile.events.subscriberequests. That breaks the test harness contract for dead-stream scenarios and can mask real regressions.The past review comment's suggested fix (separate
heldWorkspaceListContinuationsarray + dedicatedparkWorkspaceList()method) remains the correct approach.🤖 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 `@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift` around lines 123 - 129, The setHoldWorkspaceList method currently drains the shared heldContinuations queue when hold is false, which unintentionally resumes unrelated RPC continuations for mobile.host.status and mobile.events.subscribe. Create a separate heldWorkspaceListContinuations array dedicated to tracking only workspace.list continuations, and modify the setHoldWorkspaceList method to drain and resume only from this workspace-specific array instead of the shared heldContinuations queue. Consider adding a dedicated parkWorkspaceList method to handle parking workspace list continuations specifically.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1294-1295: The issue is that returning false when
autoAttachInFlight is true at lines 1294-1295 causes the caller
recoverMobileConnection() to treat this as a failure and set
connectionRecoveryFailed = true, even though the auto-attach is still in flight
and may succeed. Fix this by either returning a distinct outcome value (not
false) to signal "already in progress" rather than failure, or modify
recoverMobileConnection() at lines 1099-1103 to detect when the early exit was
due to autoAttachInFlight and skip the failure reporting in that case. The
solution should prevent false failure signals while the first gate-owning
auto-attach is still executing.
- Around line 1188-1191: The auto-attach flow currently records pairing events
as "manual" which pollutes user-intent analytics. Thread an analytics mode
parameter through the call chain to distinguish auto-attach from user-initiated
manual pairing. Add a parameter to connectManualHost (or the function containing
line 1191) to accept an analytics mode or isUserInitiated flag, then pass this
through to beginPairingAttempt so it can suppress or distinctly label the
ios_pairing_started/succeeded/failed events rather than recording them as manual
user actions. Update the call site in performAutoAttach to pass the appropriate
flag indicating this is a background auto-attach attempt, not a user-initiated
pairing.
---
Duplicate comments:
In
`@Packages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift`:
- Around line 123-129: The setHoldWorkspaceList method currently drains the
shared heldContinuations queue when hold is false, which unintentionally resumes
unrelated RPC continuations for mobile.host.status and mobile.events.subscribe.
Create a separate heldWorkspaceListContinuations array dedicated to tracking
only workspace.list continuations, and modify the setHoldWorkspaceList method to
drain and resume only from this workspace-specific array instead of the shared
heldContinuations queue. Consider adding a dedicated parkWorkspaceList method to
handle parking workspace list continuations specifically.
🪄 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
Run ID: c4ee719a-4891-464e-86a3-d1268f90e9fe
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (11)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileAutoAttachPresenceProviding.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileAutoAttachTests.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/PresenceMapTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAttachRoutePriority.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachFlag.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileAutoAttachTargetSelector.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileAutoAttachTargetSelectorTests.swiftplans/feat-ios-auto-attach/DESIGN.md
Comment on lines
+1188
to
+1191
| // Propagate the supersede intent: a user submission already cancelled | ||
| // auto-attach above; auto-attach's own connect (supersedeAutoAttach=false) | ||
| // must not cancel itself here either. | ||
| let attemptID = beginPairingAttempt(method: "manual", supersedeAutoAttach: supersedeAutoAttach) |
There was a problem hiding this comment.
Keep auto-attach out of the manual pairing funnel.
performAutoAttach() already emits ios_auto_attach_attempt/result, but Line 1191 still routes the background registry attach through beginPairingAttempt(method: "manual", ...). That means a successful auto-attach also records ios_pairing_started/succeeded/failed as if the user manually entered a host, which will pollute the manual first-pair funnel and blur explicit user-intent metrics. Thread a non-user analytics mode through connectManualHost so the auto-attach call site can suppress or distinctly label the ios_pairing_* events.
Suggested direction
-func connectManualHost(name: String, host: String, port: Int, supersedeAutoAttach: Bool) async {
+func connectManualHost(
+ name: String,
+ host: String,
+ port: Int,
+ supersedeAutoAttach: Bool,
+ pairingMethod: String? = "manual"
+) async {
if supersedeAutoAttach {
supersedeInFlightAutoAttach()
}
...
- let attemptID = beginPairingAttempt(method: "manual", supersedeAutoAttach: supersedeAutoAttach)
+ let attemptID = beginPairingAttempt(method: pairingMethod, supersedeAutoAttach: supersedeAutoAttach) await connectToRegistryInstance(
device: target.device,
instance: target.instance,
rejectLoopback: rejectLoopback,
supersedeAutoAttach: false
) await connectManualHost(
name: device.displayName ?? host,
host: host,
port: port,
- supersedeAutoAttach: false
+ supersedeAutoAttach: false,
+ pairingMethod: nil
)🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1188 - 1191, The auto-attach flow currently records pairing events
as "manual" which pollutes user-intent analytics. Thread an analytics mode
parameter through the call chain to distinguish auto-attach from user-initiated
manual pairing. Add a parameter to connectManualHost (or the function containing
line 1191) to accept an analytics mode or isUserInitiated flag, then pass this
through to beginPairingAttempt so it can suppress or distinctly label the
ios_pairing_started/succeeded/failed events rather than recording them as manual
user actions. Update the call site in performAutoAttach to pass the appropriate
flag indicating this is a background auto-attach attempt, not a user-initiated
pairing.
Comment on lines
+1294
to
+1295
| if autoAttachInFlight { return false } | ||
| let attached = await runAutoAttachOwningRestoringGate(stackUserID: stackUserID) |
There was a problem hiding this comment.
Don’t report a deduped auto-attach as a failed reconnect.
Returning false on Lines 1294-1295 feeds straight into recoverMobileConnection()’s failure path on Lines 1099-1103, which flips connectionRecoveryFailed = true even though the first gate-owning auto-attach is still in flight and may still connect successfully. This branch needs an explicit “already in progress” outcome, or the caller needs to skip failure reporting when autoAttachInFlight caused the early exit.
🤖 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 `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1294 - 1295, The issue is that returning false when
autoAttachInFlight is true at lines 1294-1295 causes the caller
recoverMobileConnection() to treat this as a failure and set
connectionRecoveryFailed = true, even though the auto-attach is still in flight
and may succeed. Fix this by either returning a distinct outcome value (not
false) to signal "already in progress" rather than failure, or modify
recoverMobileConnection() at lines 1099-1103 to detect when the early exit was
due to autoAttachInFlight and skip the failure reporting in that case. The
solution should prevent false failure signals while the first gate-owning
auto-attach is still executing.
lawrencecchen
added a commit
that referenced
this pull request
Jun 15, 2026
# Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8fb69ed. Configure here.
# Conflicts: # .github/swift-file-length-budget.tsv
…test process waitForWorkspaceListRequestCount could time out and return a short array, which callers then index directly (workspaceLists[0]/[1]). A short array made that subscript trap with `Index out of range` (ContiguousArrayBuffer:691), crashing the entire xctest process and failing every concurrently-running Swift Testing test, including attachTicketFallsBackToNextRouteWhenPreferredRouteFails. Require the count so a timed-out wait fails just this test with a readable message instead of crashing the process. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
2 participants
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.
The gap
A fresh install (or a re-install with no SQLite paired-Mac row) lands on the pair/QR screen after sign-in.
reconnectStoredMacIfNeeded()only reconnects whenMobilePairedMacStorealready has an active row, which only exists after a prior QR/manual pairing. The team-scoped device registry already knows every Mac the signed-in user's team owns and their current attach routes, but nothing consults it on the cold-start path. So a brand-new phone with a perfectly reachable team Mac still dead-ends on "scan a QR".This adds registry-driven auto-attach: a signed-in phone whose team has one reachable Mac connects with no QR scan or manual host entry. Onboarding becomes "sign in → connected".
Key finding: can the phone mint from a registry route without prior QR pairing?
Yes. The attach ticket is route-discovery + workspace-selection only; it carries no authorization secret. The Mac authorizes the mobile data plane solely on same-Stack-account matching (
MobileHostAuthorizationPolicy.authorizeStackUserinSources/Mobile/MobileHostService.swift:guard localUserID == remoteUserID else { throw .accountMismatch }), not on any pairing record.mobile.attach_ticket.createmints from routes + a bearer token with no prior pairing. So a signed-in phone can mint a valid ticket from a registry route purely because it owns the same Stack account that owns the Mac. The registry is team-scoped, so a different-account Mac never appears, and a stray cross-account route would be rejected at mint byaccount_mismatch. Auto-attach therefore reuses the existingconnectToRegistryInstancepath (pick route, mint Stack-authenticated, persist intoMobilePairedMacStore).Mechanism
Registry + presence + same-account mint. On the no-stored-Mac branch of
reconnectActiveMacIfAvailable, auto-attach loads a freshly-confirmed.okregistry list, picks the single obvious Mac via the pureMobileAutoAttachTargetSelector(presence-online preferred, else the single most-recently-seen with a reachable route; any ambiguity → fall through), and connects viaconnectToRegistryInstance, persisting the pairing so the next launch takes the normal reconnect path.A presence seam (
MobileAutoAttachPresenceProviding) is optional and degrades to recency until presence (#5792) lands, which is correct for the dominant single-Mac team.Flag + default
mobileAutoAttachviaMobileAutoAttachFlag(keycmux.mobile.autoAttach.enabled): DEBUG on, Release off until dogfooded. Read once at the composition root and injected as aBool, so the shell stays testable. An explicitUserDefaults/settings override wins over the build default.Fallback / multi-Mac
No candidate, registry/presence outage (transient failure degrades to manual rather than connecting off a stale cache), connect failure, or ambiguous multi-online → fall through to today's pair screen. Bounded and cancellable: the gate-owning runner caps the restoring window with the same 6s deadline the stored-Mac path uses, and one attempt runs at a time. Never auto-attaches to a different-account Mac.
Concurrency is handled with a per-attempt generation (not a shared boolean) plus explicit per-call supersede intent: every user-initiated pairing entry point (manual host top before validation, QR/code, device-tree tap top before the no-route guard) supersedes any in-flight auto-attach before its own early-returns, invalidating the pairing attempt and bumping the connection generation so an auto-attach already inside its own
connect()discards its result. Auto-attach's own connect passessupersedeAutoAttach: falseso it never cancels itself. Sign-out supersedes the in-flight attempt before resetting the restoring-gate flags so the next account's sign-in starts clean.On a physical phone, loopback routes are rejected (a
127.0.0.1route names the phone itself, not the Mac, and loopback is Stack-auth-trusted):selectTargetandfirstReachableHostPortskip them, andconnectToRegistryInstancestrips loopback from the routes it persists, so the next stored-Mac reconnect can never pick localhost. The simulator (where127.0.0.1is the host Mac) keeps loopback.Tests
MobileAutoAttachTargetSelectorTests, 13): online > recency, no-candidate, ambiguity (multi-online / equally-recent), per-device instance tie, loopback rejection for physical phones.MobileAutoAttachTests, 23 via injected registry/paired-store/identity doubles + scripted transport): connects + persists, fall-through on no candidate / transient registry failure, one-attempt dedupe, flag off, already-connected, same-account guard (stale attempt after account switch), manual/QR/no-route-tap/invalid-host supersession (including auto-attach mid-connect), sign-out-during-gate clean handoff, loopback not persisted.swift testgreen forCmuxMobileShellModel(60) andCmuxMobileShell(138); iOS arm64 simulator build (cmux-ios) succeeds. Structured autoreview clean.Localization audit
No new user-facing strings. Auto-attach reuses the existing
RestoringSessionViewcopy and pairing error strings; noResources/Localizable.xcstringschanges. The onlyL10n.stringlines touched are pre-existing validation strings I reordered.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches mobile connection and pairing orchestration with destructive connects and account-switch races, mitigated by conservative target selection, fresh registry reads, loopback stripping, and extensive regression tests.
Overview
Adds registry-driven auto-attach so a signed-in phone with no stored paired Mac can connect to the team’s single obvious Mac without QR or manual host entry, then persist the pairing for normal reconnect on later launches.
When
MobileAutoAttachFlagis on (DEBUG default, Release off,UserDefaultsoverride), the no-stored-Mac branch ofreconnectActiveMacIfAvailableruns a bounded attempt: fresh.okregistry list (not stale UI cache),MobileAutoAttachTargetSelector(online via presence when available, else strict recency; ambiguity → pair screen), then existingconnectToRegistryInstance. A generation-based in-flight model owns the restoring gate, dedupes concurrent reconnects, and is cancelled on sign-out.User pairing paths (
connectManualHost, QR, device-tree tap) now supersede in-flight auto-attach up front—including invalid submissions and no-route taps—and can bump connection generation so a connect already in progress cannot land over the user. Physical devices skip and do not persist loopback routes via sharedMobileAttachRoutePriority/rejectLoopback.New model pieces:
MobileAutoAttachTargetSelector,MobileAutoAttachFlag,MobileAttachRoutePriority, optionalMobileAutoAttachPresenceProviding, andPresenceMap.onlineDeviceIDs(). Broad unit/integration tests plusplans/feat-ios-auto-attach/DESIGN.md.Reviewed by Cursor Bugbot for commit 4425260. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds registry-driven auto-attach on iOS so, after sign-in, a phone connects to its team’s single reachable Mac without QR or manual host entry. Now prefers live-online Macs using the device-tree presence stream; falls back to recency and shows the pair screen if ambiguous or unreachable.
New Features
connectToRegistryInstanceand persists toMobilePairedMacStore.PresenceMap.onlineDeviceIDs()orMobileAutoAttachPresenceProviding); else picks the single most-recent reachable; ambiguity → manual pair.MobileAutoAttachTargetSelectorand sharedMobileAttachRoutePriorityunify route ordering across reconnect, switcher, device-tree tap, and auto-attach; loopback skipped on physical devices.mobileAutoAttachviaMobileAutoAttachFlag: DEBUG on, Release off; override withUserDefaultskeycmux.mobile.autoAttach.enabled.Bug Fixes
connectPairingURLResult(before validation), so even invalid codes pause the background attempt.waitForWorkspaceListRequestCountnow requires the expected count to avoid xctest crashes on timeouts, failing the test locally with a clear message.Written for commit 4425260. Summary will update on new commits.
Summary by CodeRabbit