Minimal pairing QR: routes-only payload, loopback refusal, full-width render, Copy IP/Port, no expiry - #5872
Merged
Merged
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:
📝 WalkthroughWalkthroughIntroduces a minimal v2 pairing QR codec (route-only, no expiry/identity), time-independent attach-ticket validation and optional expiry, loopback detection/rejection for physical devices, host-status verification/adoption with serialized persistence, QR bitmap and frame-selection improvements, transport waiting fast-fail handling, manual-entry UI/copy, and extensive tests. ChangesPairing QR & lifecycle
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e2907ab. Configure here.
![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: e2907ab93c
ℹ️ 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".
|
|
||
| /// The route id the Mac's route resolver mints for the route at `index` | ||
| /// (`tailscale` for the first, `tailscale_N` after). | ||
| private func synthesizedRouteID(index: Int) -> String { |
There was a problem hiding this comment.
For mobile-owned changes, .github/workflows/test-ios.yml runs ./scripts/lint-ios-package-conventions.sh in the package-conventions-lint job; running that script reports ERROR free-function for this helper and the other private helpers added in this commit, and the script exits nonzero on any ERROR. Please move these helpers under CmxPairingQRCode/their owning types or add a sanctioned suppression so the iOS CI can pass.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryReplaces the pairing QR payload with a minimal v2 URL grammar (
Confidence Score: 5/5Safe to merge — the trust boundaries (loopback rejection, Stack-gated identity, limiter-capped DoS surface) are all correctly implemented and broadly tested. The loopback classifier, verification limiter slot management, serialized paired-Mac write chain, and identity-recovery guards all check out. The only finding is a minor diagnostic logging inconsistency in No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Mac as Mac (MobileHostService)
participant QR as Pairing QR (v2)
participant iOS as iOS Shell (MobileShellComposite)
participant Stack as Stack Auth
Mac->>QR: "encode(ticket) → cmux-ios://attach?v=2&r=host:port"
Note over QR: No token, no expiry, no display name
iOS->>QR: scan
QR-->>iOS: CmxAttachTicket(macDeviceID: "", routes: [...])
iOS->>iOS: reject loopback routes (CmxLoopbackHost)
iOS->>Mac: connect via Tailscale route
iOS->>Mac: mobile.host.status + Stack token (opportunistic)
Mac->>Stack: verifiedStackCaller (cached or network, limiter-capped)
Stack-->>Mac: "verified = true/false"
Mac-->>iOS: identityStatusPayload OR publicStatusPayload
iOS->>iOS: applyHostReportedIdentity (device id + display name)
iOS->>iOS: persistPairedMacFromTicket (serialized write chain)
Reviews (6): Last reviewed commit: "Deflake renderGridTerminalInputWaitsForL..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1661
to
+1679
| guard activeTicket?.macDeviceID.isEmpty == true else { return } | ||
| hostIdentityAdoptionTask?.cancel() | ||
| hostIdentityAdoptionTask = Task { @MainActor [weak self] in | ||
| guard let self, !Task.isCancelled, self.remoteClient === client else { return } | ||
| let data: Data | ||
| do { | ||
| data = try await client.sendRequest( | ||
| MobileCoreRPCClient.requestData(method: "mobile.host.status", params: [:]) | ||
| ) | ||
| } catch { | ||
| // The connection (or a reconnect) re-schedules adoption; a | ||
| // failed status here means the connection itself is in | ||
| // trouble and its own recovery paths take over. | ||
| mobileShellLog.error("host identity status request failed: \(String(describing: error), privacy: .private)") | ||
| return | ||
| } | ||
| guard !Task.isCancelled, | ||
| let payload = try? MobileHostStatusResponse.decode(data) else { return } | ||
| await self.applyHostReportedIdentity( |
Contributor
There was a problem hiding this comment.
try? CmxAttachTicket(...) discards any validation error without logging. If the Mac-reported mac_device_id ever fails ticket validation (e.g., an unexpectedly long or malformed string from a new Mac version), the phone connects but is never persisted to the paired-Mac store — reconnect-on-launch and the host switcher silently lose the entry, with zero signal that anything went wrong. A mobileShellLog.error on the discarded error would make this diagnosable without changing the non-blocking intent of the adoption path.
Comment on lines
+184
to
+188
| case .loopbackRejected: | ||
| return L10n.string( | ||
| "mobile.pairing.loopbackRejected", | ||
| defaultValue: "This code points at the Mac itself (localhost), so your iPhone can't use it. Set up Tailscale on the Mac, then scan a fresh code." | ||
| ) |
Contributor
There was a problem hiding this comment.
The defaultValue says "your iPhone can't use it", but this surface is also shown on iPad devices. When the xcstrings catalog key uses the defaultValue (e.g., falls back because the localized string is missing or for an unsupported locale), iPad users see the incorrect device name. "your device" or "your iPhone or iPad" would be device-neutral here.
Suggested change
| case .loopbackRejected: | |
| return L10n.string( | |
| "mobile.pairing.loopbackRejected", | |
| defaultValue: "This code points at the Mac itself (localhost), so your iPhone can't use it. Set up Tailscale on the Mac, then scan a fresh code." | |
| ) | |
| case .loopbackRejected: | |
| return L10n.string( | |
| "mobile.pairing.loopbackRejected", | |
| defaultValue: "This code points at the Mac itself (localhost), so your device can't use it. Set up Tailscale on the Mac, then scan a fresh code." | |
| ) |
Rule Used: Flag production user-facing text that is not fully... (source)
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!
Comment on lines
+172
to
+195
| macDeviceID: "", | ||
| macDisplayName: nil, | ||
| routes: routes, | ||
| expiresAt: nil, | ||
| authToken: nil | ||
| ) | ||
| try ticket.validate() | ||
| return ticket | ||
| } | ||
| } | ||
|
|
||
| /// The route id the Mac's route resolver mints for the route at `index` | ||
| /// (`tailscale` for the first, `tailscale_N` after). | ||
| private func synthesizedRouteID(index: Int) -> String { | ||
| index == 0 | ||
| ? CmxAttachTransportKind.tailscale.rawValue | ||
| : "\(CmxAttachTransportKind.tailscale.rawValue)_\(index + 1)" | ||
| } | ||
|
|
||
| /// The priority the Mac's route resolver assigns the route at `index`. | ||
| private func synthesizedRoutePriority(index: Int) -> Int { | ||
| 10 + index * 10 | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
parseHostPort accepts bare : as a valid host from an unbracketed colon-prefixed input
For an input like ::8080 (no brackets), trimmed.lastIndex(of: ":") lands on index 1, so host = ":" and portText = "8080". isPlainHost(":") returns true (: is in the allowed byte set for IPv6). The route passes validation, CmxLoopbackHost().matches(":") returns false, and CmxAttachRoute is constructed with host: ":" — a host that will dial-fail immediately but is meaningless. The input should be rejected: an IPv6 literal without brackets must still contain at least two colons to be a minimally plausible address, and a single bare : is neither IPv4 nor IPv6.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/Mobile/Pairing/MobilePairingModel.swift (2)
92-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the previous status observer before every refresh.
A refresh that exits through
.signedOut,.needsTailscale, or.failednever reachesobserveConnections(), so the oldconnectionObservationTaskstays subscribed tohost.statusUpdates()until some later status event or window close. Repeated refreshes can therefore accumulate stale observers.Suggested fix
func refresh() async { + stopObserving() refreshGeneration &+= 1 let generation = refreshGeneration state = .loadingAlso applies to: 203-205
🤖 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 `@Sources/Mobile/Pairing/MobilePairingModel.swift` around lines 92 - 95, In refresh(), cancel any existing connectionObservationTask before incrementing refreshGeneration and setting state to .loading so previous observers don't accumulate; specifically, ensure connectionObservationTask (the Task that subscribes via host.statusUpdates() in observeConnections()) is cancelled at the start of refresh() (and similarly in the other refresh locations referenced) before creating/awaiting a new observation or calling observeConnections(), then clear or replace connectionObservationTask with the new Task to avoid stale subscriptions.
267-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter loopback routes out of the manual Tailscale list.
readyContentrenders every string fromtailscaleLines, but this helper currently includes all.tailscaleroutes. On a Mac that has both a real Tailscale address and the DEBUG loopback route, the manual fallback will still show the unusable127.0.0.1/::1entry even though the QR path intentionally rejects it.Suggested fix
private static func tailscaleLines(_ routes: [CmxAttachRoute]) -> [String] { routes.compactMap { route in - guard route.kind == .tailscale, + guard isPhoneReachableRoute(route), case let .hostPort(host, port) = route.endpoint else { return nil } return "\(host):\(port)" } }🤖 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 `@Sources/Mobile/Pairing/MobilePairingModel.swift` around lines 267 - 274, tailscaleLines currently returns all .tailscale host:port entries including loopback addresses, causing unusable 127.0.0.1/::1 entries to appear in readyContent; update tailscaleLines(_:) to skip routes whose host is a loopback address (e.g. "127.0.0.1" or "::1") before building the "\(host):\(port)" string. Locate the tailscaleLines function and add a guard that inspects the extracted host (from the .hostPort pattern) and returns nil for loopback hosts so only real Tailscale addresses are included.Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2372-2385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check connection currency after the paired-Mac write await.
Line 2372 introduces a new suspension after the last
generation == connectionGeneration/remoteClient === clientguard. If the user starts another pairing while this write is in flight, the old attempt can resume, persist the wrong Mac as active, and then overwrite the newer attempt's workspace list andconnectionStatewith stale data. Pass anifStillCurrentclosure intopersistPairedMacFromTicket(...)here and guardisCurrentRemoteConnection(client:generation:)again immediately after the await.Suggested fix
- await persistPairedMacFromTicket(ticket) + await persistPairedMacFromTicket( + ticket, + ifStillCurrent: { [weak self] in + self?.isCurrentRemoteConnection(client: client, generation: generation) == true + } + ) + guard isCurrentRemoteConnection(client: client, generation: generation) else { + return nil + } applyRemoteWorkspaceList(response, preferActiveTicketTarget: workspaceListRequest.preferActiveTicketTarget) syncSelectedTerminalForWorkspace() connectionState = .connected🤖 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 2372 - 2385, The await in persistPairedMacFromTicket can resume after a new pairing starts and cause stale state to be applied; update persistPairedMacFromTicket to accept an ifStillCurrent closure and call it to abort the write when the connection is no longer current, then immediately after awaiting persistPairedMacFromTicket(...) re-check isCurrentRemoteConnection(client:generation:) (the same guard used earlier) before calling applyRemoteWorkspaceList(...), syncSelectedTerminalForWorkspace(), setting connectionState = .connected, markMacConnectionHealthy(), or recording DiagnosticEvent(.pairOk) so stale data cannot overwrite a newer pairing; reference persistPairedMacFromTicket, isCurrentRemoteConnection(client:generation:), applyRemoteWorkspaceList, syncSelectedTerminalForWorkspace, and connectionState when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/MobileHostStatusVerificationLimiterTests.swift`:
- Around line 15-24: Add a new concurrent test that verifies
MobileHostStatusVerificationLimiter is thread-safe under contention: create a
test (e.g., handlesConcurrentAcquires) that instantiates
MobileHostStatusVerificationLimiter(limit: 2), uses withTaskGroup to launch
multiple parallel acquire() tasks (e.g., 4 tasks), collects their Bool results,
and assert that the number of successful acquires equals the limiter's limit
(2); also ensure you call release() as needed in tasks or after to avoid leaking
permits.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1616-1626: The fallback display-name lookup (currently using
ticket.macDisplayName and pairedMacStore.loadAll(stackUserID: nil) before
queuing) must be moved inside the performSerializedPairedMacWrite closure so it
runs under the serialized/atomic write and sees the latest row; inside that
closure, if ticket.macDisplayName is nil, perform a scoped lookup for the
current user (e.g., load by stackUserID and macDeviceID or a single-row load API
on pairedMacStore using ticket.macDeviceID and the active stackUserID) and use
that result for resolvedDisplayName, avoiding the pre-queue loadAll(stackUserID:
nil) which can leak another user’s name and cause stale overwrites. Ensure the
closure still references ifStillCurrent and ticket.macDisplayName/macDeviceID
and that the upsert uses the resolved name determined inside
performSerializedPairedMacWrite.
In
`@Packages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift`:
- Around line 299-307: The comment above the fail-fast logic in
CmxNetworkByteTransport.swift is inaccurate: it claims both connection-refused
and host-unreachable are treated as definitive, but the implemented policy
(waitingKindFailsConnect) only fast-fails .connectionRefused. Update the comment
to reflect the actual behavior — state that only `.connectionRefused` is treated
as a definitive immediate failure for the initial connect (not
`.hostUnreachable`) and clarify that other `.waiting` reasons remain treated as
transient and allowed to be retried until the overall timeout.
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2372-2385: The await in persistPairedMacFromTicket can resume
after a new pairing starts and cause stale state to be applied; update
persistPairedMacFromTicket to accept an ifStillCurrent closure and call it to
abort the write when the connection is no longer current, then immediately after
awaiting persistPairedMacFromTicket(...) re-check
isCurrentRemoteConnection(client:generation:) (the same guard used earlier)
before calling applyRemoteWorkspaceList(...),
syncSelectedTerminalForWorkspace(), setting connectionState = .connected,
markMacConnectionHealthy(), or recording DiagnosticEvent(.pairOk) so stale data
cannot overwrite a newer pairing; reference persistPairedMacFromTicket,
isCurrentRemoteConnection(client:generation:), applyRemoteWorkspaceList,
syncSelectedTerminalForWorkspace, and connectionState when making the change.
In `@Sources/Mobile/Pairing/MobilePairingModel.swift`:
- Around line 92-95: In refresh(), cancel any existing connectionObservationTask
before incrementing refreshGeneration and setting state to .loading so previous
observers don't accumulate; specifically, ensure connectionObservationTask (the
Task that subscribes via host.statusUpdates() in observeConnections()) is
cancelled at the start of refresh() (and similarly in the other refresh
locations referenced) before creating/awaiting a new observation or calling
observeConnections(), then clear or replace connectionObservationTask with the
new Task to avoid stale subscriptions.
- Around line 267-274: tailscaleLines currently returns all .tailscale host:port
entries including loopback addresses, causing unusable 127.0.0.1/::1 entries to
appear in readyContent; update tailscaleLines(_:) to skip routes whose host is a
loopback address (e.g. "127.0.0.1" or "::1") before building the
"\(host):\(port)" string. Locate the tailscaleLines function and add a guard
that inspects the extracted host (from the .hostPort pattern) and returns nil
for loopback hosts so only real Tailscale addresses are included.
🪄 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: c9bcc6cf-ac50-4324-9cbf-30eef025df9f
📒 Files selected for processing (39)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxLoopbackHost.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualPairingEntry.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachRoute.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxLoopbackHostTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileHostStatusVerificationLimiter.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingQRImageView.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/Mobile/Pairing/MobilePairingWindowController.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostStatusVerificationLimiterTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Comment on lines
+15
to
+24
| @Test func capsInFlightLookups() async { | ||
| let limiter = MobileHostStatusVerificationLimiter(limit: 2) | ||
|
|
||
| #expect(await limiter.acquire()) | ||
| #expect(await limiter.acquire()) | ||
| #expect(!(await limiter.acquire())) | ||
|
|
||
| await limiter.release() | ||
| #expect(await limiter.acquire()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding concurrent-acquire test coverage.
The test validates the basic acquire/release flow, but a concurrency limiter should also be tested under concurrent load. Consider adding a test that launches multiple parallel acquire() calls to verify thread-safety and the limit under contention.
Example concurrent test structure
`@Test` func handlesConcurrentAcquires() async {
let limiter = MobileHostStatusVerificationLimiter(limit: 2)
await withTaskGroup(of: Bool.self) { group in
// Launch 4 concurrent acquires against limit of 2
for _ in 0..<4 {
group.addTask { await limiter.acquire() }
}
var successes = 0
for await result in group {
if result { successes += 1 }
}
`#expect`(successes == 2)
}
}🤖 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 `@cmuxTests/MobileHostStatusVerificationLimiterTests.swift` around lines 15 -
24, Add a new concurrent test that verifies MobileHostStatusVerificationLimiter
is thread-safe under contention: create a test (e.g., handlesConcurrentAcquires)
that instantiates MobileHostStatusVerificationLimiter(limit: 2), uses
withTaskGroup to launch multiple parallel acquire() tasks (e.g., 4 tasks),
collects their Bool results, and assert that the number of successful acquires
equals the limiter's limit (2); also ensure you call release() as needed in
tasks or after to avoid leaking permits.
lawrencecchen
added a commit
that referenced
this pull request
Jun 11, 2026
…ility round, #5872) Conflict resolution: keep workspace-groups capability lines alongside the QR branch's host-identity adoption calls; capabilities stay in the MobileHostService+Capabilities extension (dropped the fold's duplicate inline copy); pbxproj unions both sides' file additions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-qr-compact
branch
from
June 11, 2026 21:26
4fc0000 to
716f59b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 192-367: This adds a large composer/draft subsystem into
MobileShellComposite; extract it into a dedicated helper/class (e.g.
MobileComposerDraftManager or MobileShellComposer) and inject it into
MobileShellComposite to reduce file size and separation of concerns: move the
state and behavior related to composition and per-terminal drafts (properties
terminalInputText, isComposerPresented, composerDismissedTerminalIDs,
composerFocusRequest, composerFocusRequestPending,
composerFocusRequestTerminalID, composerFieldIsFocused,
isSubmittingComposerInput, draftStore, isLoadingDraft, draftOperationTail,
pendingDraftSaveTextByTerminalID, draftedOutgoingTerminalID,
draftedOutgoingText, draftLoadPendingTerminalID) plus the draft-related routines
(persistCurrentDraft, enqueueDraftOperation, swapDraft, applyLoadedDraft,
requestComposerFieldFocus, presentAndFocusComposer,
consumePendingComposerFocusRequest, submitComposerInput and any helpers they
call) into the new type; keep MobileShellComposite’s selectedTerminalID logic
but delegate draft switching/ persistence to the new manager (call its
swap/persist/load APIs), inject draftStore into the new manager, update call
sites and tests, and ensure behavior (guarding with isLoadingDraft and
draftLoadPendingTerminalID, and the focus handshake) is preserved.
🪄 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: d76c7d2f-5d1d-40d8-a6b5-decfce041d75
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (46)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxLoopbackHost.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualPairingEntry.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRBitmap.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachRoute.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxLoopbackHostTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeCaptureController.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameCandidate.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameSelection.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeMetadataReceiver.swiftPackages/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileHostStatusVerificationLimiter.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingQRImageView.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/Mobile/Pairing/MobilePairingWindowController.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostStatusVerificationLimiterTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 192-367: This adds a large composer/draft subsystem into
MobileShellComposite; extract it into a dedicated helper/class (e.g.
MobileComposerDraftManager or MobileShellComposer) and inject it into
MobileShellComposite to reduce file size and separation of concerns: move the
state and behavior related to composition and per-terminal drafts (properties
terminalInputText, isComposerPresented, composerDismissedTerminalIDs,
composerFocusRequest, composerFocusRequestPending,
composerFocusRequestTerminalID, composerFieldIsFocused,
isSubmittingComposerInput, draftStore, isLoadingDraft, draftOperationTail,
pendingDraftSaveTextByTerminalID, draftedOutgoingTerminalID,
draftedOutgoingText, draftLoadPendingTerminalID) plus the draft-related routines
(persistCurrentDraft, enqueueDraftOperation, swapDraft, applyLoadedDraft,
requestComposerFieldFocus, presentAndFocusComposer,
consumePendingComposerFocusRequest, submitComposerInput and any helpers they
call) into the new type; keep MobileShellComposite’s selectedTerminalID logic
but delegate draft switching/ persistence to the new manager (call its
swap/persist/load APIs), inject draftStore into the new manager, update call
sites and tests, and ensure behavior (guarding with isLoadingDraft and
draftLoadPendingTerminalID, and the focus handshake) is preserved.
🪄 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: d76c7d2f-5d1d-40d8-a6b5-decfce041d75
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (46)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxLoopbackHost.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualPairingEntry.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRBitmap.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachRoute.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxLoopbackHostTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeCaptureController.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameCandidate.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameSelection.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeMetadataReceiver.swiftPackages/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileHostStatusVerificationLimiter.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingQRImageView.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/Mobile/Pairing/MobilePairingWindowController.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostStatusVerificationLimiterTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
🛑 Comments failed to post (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
192-367: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract the composer/draft subsystem out of
MobileShellComposite.Line 192 onward adds a large, separate composer+draft domain (focus handshake, per-terminal persistence queue, send reconciliation) into a class that already owns connection lifecycle, transport/RPC orchestration, recovery, and workspace state. In this file size, that materially increases change risk and review/test surface.
As per coding guidelines, "
{Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift: Flag Swift production files that exceed 400 lines ... flag when more than 250 lines are added to an existing production Swift file already over 800 lines ..." and "Flag files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place."Also applies to: 2396-2590, 3230-3360
🤖 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 192 - 367, This adds a large composer/draft subsystem into MobileShellComposite; extract it into a dedicated helper/class (e.g. MobileComposerDraftManager or MobileShellComposer) and inject it into MobileShellComposite to reduce file size and separation of concerns: move the state and behavior related to composition and per-terminal drafts (properties terminalInputText, isComposerPresented, composerDismissedTerminalIDs, composerFocusRequest, composerFocusRequestPending, composerFocusRequestTerminalID, composerFieldIsFocused, isSubmittingComposerInput, draftStore, isLoadingDraft, draftOperationTail, pendingDraftSaveTextByTerminalID, draftedOutgoingTerminalID, draftedOutgoingText, draftLoadPendingTerminalID) plus the draft-related routines (persistCurrentDraft, enqueueDraftOperation, swapDraft, applyLoadedDraft, requestComposerFieldFocus, presentAndFocusComposer, consumePendingComposerFocusRequest, submitComposerInput and any helpers they call) into the new type; keep MobileShellComposite’s selectedTerminalID logic but delegate draft switching/ persistence to the new manager (call its swap/persist/load APIs), inject draftStore into the new manager, update call sites and tests, and ensure behavior (guarding with isLoadingDraft and draftLoadPendingTerminalID, and the focus handshake) is preserved.Source: Coding guidelines
lawrencecchen
force-pushed
the
feat-ios-qr-compact
branch
from
June 11, 2026 22:22
70e0290 to
9ba98ee
Compare
The pairing QR's compact payload no longer encodes an expiry (e) or the Mac display name (n). A pairing QR never expires: the owner's Stack access token is the host's sole authorization gate, so ticket age authorizes nothing, and the phone's "This pairing link expired" failure path for scanned QRs is removed. The Mac's name now arrives post-handshake via mobile.host.status (new mac_display_name field, served from one shared publicStatusPayload), so a freshly paired phone replaces the device-id placeholder once connected and never clobbers a known name with nil. CmxAttachTicket.expiresAt becomes optional data for the attach-token consumers (MobileCoreRPCClient at token use, the Mac's ticket store), not a structural validity condition; validate() no longer takes a clock. Route ids the decoder can resynthesize (kind, kind_N) and endpoint types implied by the keys present are also omitted. The decoder stays tolerant of first-revision compact payloads (extra e/n keys ignored, explicit ids and endpoint types honored) and of legacy full-key payloads with or without expiresAt. Representative ticket from the real encoder (ECC L): 1 route 217B/QR v11 -> 141B/QR v9; 2 routes 313B/v14 -> 203B/v11. Tests: round-trip of the new grammar (including repeated-kind and custom route ids), first-revision compact and legacy full-key decode of stale payloads, and an end-to-end pairs-10-minutes-after-mint store test.
The pairing window's QR was fixed at 220pt. It now fills the window width (1:1 aspect), the window is resizable (min 380x480) so the code can be made larger for scanning at a distance, and the image is generated once at native module resolution and upscaled with interpolation disabled so every module stays a sharp nearest-neighbor square at any size and backing scale. The 20pt white padding around the code is the QR quiet zone, comfortably above the 4-module spec minimum at this size.
The compact-ticket restructure dropped version: v when rebuilding the CmxAttachTicket, so every compact payload silently decoded as currentVersion and the unsupportedVersion gate never fired for future grammar revisions. Pass the decoded v through and pin it with a v:2-must-throw test.
The pairing QR now encodes only what pairing needs: where to dial. cmux-ios://attach?v=2&r=<host>:<port>[&r=...] replaces the base64 JSON payload for the Mac pairing window's code. Representative output from the real encoder (ECC L): 1 route 154B/QR v7 -> 40B/QR v3; MagicDNS+IP 257B/QR v10 -> 78B/QR v4. Dropped from the QR, with their replacement channels: - mac device id and display name: reported post-handshake by mobile.host.status (new mac_device_id field next to the existing mac_display_name; both are the random pairing UUID / Bonjour-visible name, on routes only reachable over the user's own tailnet). The phone adopts the reported identity into the active ticket and only then persists the paired-Mac record, so QR pairing stays reconnectable and switchable. - expiry: already removed from the grammar; v2 has no field to even misread. - auth token: never authorized anything (Stack auth is the host's sole gate). Loopback is banned end to end, in both directions: - The Mac never mints a weak code: the v2 encoder drops a DEBUG build's dev loopback route, the pairing window requires a non-loopback Tailscale route before showing a code (a DEBUG Mac without Tailscale now shows the set-up-Tailscale guidance instead of a loopback QR), and it refuses to display any attach URL that is not v2. - The phone rejects scanned/pasted v2 codes naming loopback hosts (127/8, ::1, ::ffff:127/8, *.localhost) with a localized error that names the actual fix (set up Tailscale) instead of dialing itself. The legacy v1 payload path is deliberately not gated: the simulator/dev auto-pair flow injects loopback attach URLs there. CmxLoopbackHost is the single loopback classifier shared by the encoder, the decoder, and the pairing-window gate. Decoder compatibility is unchanged for every legacy grammar: v1 compact short-key payloads, first-revision compact payloads with extra e/n keys, legacy full-key payloads with or without expiry, and the ancient cmux-ios://pair links all still decode (covered by tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aiting Scan-to-pair latency finding: the QR receiver fires on the first camera frame; the dominant delay is downstream, in the route dial. When a dead route sorts first (the old QR's loopback route on every DEBUG dogfood Mac, or a stale Tailscale address), Network.framework surfaces the refused/unreachable answer as .waiting and retries; the transport ignored waiting events, so connect() sat silent until the timeout (8-15s) before the next route was tried. Test dials a just-closed loopback port with a 5s connect timeout and requires the classified connectionFailed(.connectionRefused) error. Red on this commit: connect() parks in .waiting until the deadline and surfaces connectionTimedOut after 5.1s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NWConnection parks dials it intends to retry in .waiting instead of .failed, including connection-refused, host-unreachable, and DNS failure, which for our single-address connect are definitive answers. Treat those kinds as connect failures while the transport is still in the connecting state, so the caller's next route starts immediately; the pairing route loop and the reconnect path both retry by design. timedOut/generic waits stay parked (a tailnet link finishing its handshake can genuinely recover) and the existing connect timeout still bounds them. Once ready, waiting events remain ignored: the RPC layer's liveness watchdog owns mid-stream recovery. Green: the refused dial now fails in ~0.1s instead of eating the whole connect deadline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found the trust-boundary classifier could be bypassed by canonical-equivalent spellings the string checks missed: uncompressed IPv6 (0:0:0:0:0:0:0:1), the fully-qualified localhost. root-dot form, and the legacy IPv4 numeric forms the resolver happily dials (127.1, 2130706433, 0x7f.0.0.1). Any of these in a scanned v2 code would have re-introduced the self-dial path. CmxLoopbackHost now parses hosts with the same libc semantics the dialer's resolver applies: inet_aton for IPv4-ish names (covering dotted-quad, short, octal, hex, and 32-bit decimal forms) and inet_pton for IPv6 (covering every spelling of one address, plus IPv4-mapped and IPv4-compatible embeddings, with zone indexes stripped). The unspecified range (0.0.0.0/8, ::) counts as self-dialing too, since a TCP connect to it lands on the local machine. Tests pin the bypass spellings on both the classifier and the v2 QR decoder, and pin that non-loopback legacy numeric forms (128.1, 1681915909) stay accepted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manual-entry fallback in the Mac pairing window only offered selectable host:port text; copying the address or the port alone for the phone's separate host and port fields meant fiddly text selection. The window now shows two distinct bordered buttons, Copy IP and Copy Port, each flashing a brief Copied check (the MarkdownPanelView copy confirmation pattern, generation-guarded). What they copy is policy, so it lives in CMUXMobileCore next to the other route trust rules: CmxManualPairingEntry.best(in:) picks the best phone-dialable route, never loopback (shared CmxLoopbackHost classifier, same rule as the QR encoder), preferring Tailscale and, among Tailscale routes, a numeric IP literal over the MagicDNS name (a typed IP works even when the phone's DNS is not pointed at the tailnet), tie-broken by the Mac's route priority order. Covered by SPM tests; labels localized en + ja. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt callers Review found that moving mac_device_id and mac_display_name into the status reply (the post-handshake identity channel that replaced the QR's identity fields) put a stable tracking identifier on an unauthenticated network surface: status is the one verb any peer that can reach the listener port may call, so any LAN/tailnet process could poll the Mac's pairing UUID and display name without proving account ownership. The status reply is now two-tier. A tokenless probe gets the cached identity-free payload (routes, fidelity, capabilities) exactly as before, without touching the main actor or the Stack verifier, so the probe's reachability use and DoS posture are unchanged, and the existing testMobileHostNetworkStatusDoesNotExposePrivateMetadata assertion holds again. A request that presents the owner's Stack access token is verified through the same MobileHostStackAuthVerifier gate as every authorized verb and answered with the identity payload; a failing token degrades to the identity-free reply rather than an error. The iOS client attaches its Stack token to the status probe opportunistically: only when the route is trusted to carry the bearer token (MobileShellRouteAuthPolicy, so a plain-LAN manual route never sees it) and never failing the probe when no token is available. A QR-pairing connect is always signed in, so the identity adoption flow keeps working end to end. SPM tests pin all three client behaviors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found the second gap in the post-handshake identity flow: the mobile.host.status request runs inside the long-lived listener task, and nothing proved the response still belonged to the current connection before it mutated state. If the user re-paired while the request was in flight, the old Mac's reply could adopt its device id and name onto the new connection's empty-id ticket and persist a mixed paired-Mac record. resolveTerminalOutputTransport and applyHostReportedIdentity now re-check remoteClient === client after every suspension point (the same currency guard the subscribe failure path and the event-stream loop already use) before touching terminalOutputTransport, supportsWorkspaceActions, activeTicket, connectedHostName, or the paired-Mac store; a stale reply returns inert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tale writes Review follow-ups on the post-handshake identity flow: The mobile.host.status read that recovers a freshly QR-paired Mac's identity rode the terminal-output capability probe, which deliberately fails fast (750ms) so the terminal can fall back to raw bytes. On a slow tailnet link the connection could succeed while the probe timed out, leaving the ticket's macDeviceID empty and the paired Mac never persisted (no record, no reconnect-on-launch). The probe still applies identity when it succeeds (no extra request in the common case), but when it cannot deliver one for an identity-less ticket it now schedules a dedicated status request with the full RPC timeout, owned by a task that is cancelled on disconnect and funnels into the same guarded adoption path. The currency guards also now hold through the store writes themselves: persistPairedMacFromTicket and applyHostReportedDisplayName take an ifStillCurrent check evaluated immediately before their upsert, so a status reply whose task suspended across a re-pair can no longer commit markActive for the old Mac after the user moved to a new one. The connect path's own persist stays unconditional, since its write is for the connection it just established. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found the legacy-grammar bypass: the v2 pairing-QR decoder rejects loopback routes, but a scanned/pasted code could still use the legacy v1 payload grammar to smuggle a 127.0.0.1 route past the rejection. Loopback is in the Stack-auth-trusted route set, so on a physical phone that connection would dial the phone's own localhost and hand the account bearer token to whatever local process answers. The legacy grammars cannot simply ban loopback in the decoder: the simulator dev flow depends on them (in the simulator 127.0.0.1 IS the host Mac, and dev auto-pair, the XCUITest fixtures, and the scripted feature tests all pair over loopback v1 payloads). The axis that separates the legitimate uses from the attack is the device, not the input source, so the rule is: on a physical iPhone/iPad no grammar may pair to a loopback route, ever; in the simulator and in macOS-hosted package tests loopback pairing stays intact. MobileShellRouteAuthPolicy.ticketRejectsLoopbackRoutes carries the rule as a pure function (unit tested for both device values, including a loopback host hiding under the tailscale kind and a mixed-route ticket); connectPairingURLResult applies it after decode for every grammar, surfacing the same localized set-up-Tailscale guidance as the v2 rejection. Only the one-line compile-time device wiring is untested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Host-unreachable, DNS, and permission-denied waits happen transiently while a Tailscale link converges or the Local Network privacy prompt is up, so they must keep the bounded connect timeout instead of killing the dial. Only an RST is definitive: the host answered and nothing listens on the port. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atus identity Review follow-ups: The initial-connect fast-fail treated hostUnreachable, dnsFailed, permissionDenied, and secureChannelFailed waiting reasons as terminal, but NWConnection parks dials in .waiting precisely because those can recover while the path converges: unreachable/DNS happen transiently as a Tailscale link comes up, and permission-denied covers the Local Network privacy prompt the first dial triggers, so fast-failing them could abort a pairing that would have succeeded within the bounded timeout. Only connection-refused stays fast-fail: an RST proves the host is reachable with nothing listening, which was the actual scan-to-pair latency bug (the dead loopback route). Everything else parks under the existing connect timeout. networkStatusResult verified the status request's Stack token directly, skipping the DEBUG dev-token policy that authorizationError honors, so a DEBUG dev-token client could list workspaces yet never receive the Mac's identity (breaking v2 QR identity adoption in the dogfood path). The dev-token check is now one shared helper (devStackTokenAuthorized) and status identity goes through verifiedStackCaller, the same gate as the authorized verbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Expiry no longer gates pairing (it is enforced only where the RPC attach token is used), so an expired legacy QR is a valid pairing input. The offline preflight still skipped such tickets, sending an offline scan into the route loop's stacked connect timeouts instead of the fast offline error. Drop the expiry predicate from the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pairing window re-minted the code on a ticketTTL-30 timer, a vestige of the expiring-ticket design. The v2 pairing URL carries no token and no expiry, so there is nothing to keep fresh on a schedule; regenerating behind the user's back only made the QR flash mid-scan. Route changes while the window sits open are handled by the manual Refresh Code button. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 20pt fixed padding around the QR was about 2.7 modules of quiet zone at the default window size, under the 4-module ISO/IEC 18004 minimum that third-party scanners in particular depend on. Rendering moves into CmxPairingQRBitmap (CMUXMobileCore, next to the payload codec): one pixel per module, pure black on pure white, with the full 4-module quiet zone baked into the bitmap so it scales with the code and cannot be cropped by view layout or recolored by theme. ECC switches L -> M: the routes-only payload is small enough that M keeps the code at version <= 6 (tests assert it), and the extra redundancy absorbs the glare and off-angle blur of scanning a glossy Mac screen. The pairing window default grows to 540x720 so the full-width code renders larger out of the box. Pixel-level tests cover the white quiet-zone ring, the pure black/white output, and the module-count-vs-version arithmetic (which also pins the generator's 1-module-margin assumption). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The in-app scanner was losing to the system Camera app on the same code. Three causes, all in the capture setup: - It opened the bare default camera (the wide angle). On recent Pro phones that lens cannot focus nearer than ~20 cm, exactly where people hold the phone to a code on a Mac screen, so it hunts forever; the Camera app silently switches to the close-focusing ultra-wide. The scanner now opens the best virtual multi-lens device (triple > dual-wide > dual > wide) so constituent switching does the same thing. - No focus tuning. Now: continuous autofocus restricted to the near range, smooth AF off (video nicety that slows refocus snaps), continuous exposure, and low-light boost, all capability-guarded. Still no torch, deliberately: the Mac screen is backlit and a torch on glossy glass washes out the code. - The metadata callback read only metadataObjects.first, so any other detection ordered before the pairing QR in a frame masked it. The selection rule now considers every detection; it lives in QRCodeFrameSelection, an AVFoundation-free type with tests for the multi-detection frames the delegate path cannot simulate in CI. Session preset is pinned to 1080p where supported, and a comment documents why rectOfInterest stays at its full-frame default (it is in normalized landscape capture coordinates; scoping it to a viewfinder requires metadataOutputRectConverted(fromLayerRect:)). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the two pure camera helpers from private statics to file-scope private funcs, split QRCodeFrameCandidate into its own file, document its public init, and drop em dashes from the new doc comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both call sites already compiled through the CoreGraphics Int initializers; the explicit CGFloat conversion just makes that visible to reviewers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move 18 file-scope private helpers into private extensions of the types they serve (CmxPairingQRCode, CompactAttachTicket, CmxManualPairingEntry, CmxLoopbackHost, QRCodeCaptureController, MobileCoreRPCClient isHostStatusRequest, MobileShellComposite placeholderHostName, CmxNetworkByteTransport waitingKindFailsConnect). Init-time callers use static members via Self; behavior unchanged. Budget refresh accepts the branch growth as known debt: MobileShellComposite +280 (pairing persistence serialization + host identity adoption, coupled to the composite's private connection state; precedent: the composer merge refreshed this same file), MobileHostService +117 (status identity payloads + verified-caller gate, new app-target file would need pbxproj wiring), CmxNetworkByteTransport +38 (connect fast-fail classification). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
expiredQRTicketWhileOfflineReportsExpiredNotOffline asserted the old order (expiry beats offline). This branch deliberately removed expiry from pairing classification (73ba349 dropped it from the QR grammar, 3da66fa runs the offline preflight for expired tickets too, and the connect-time expiry gate is gone), so an expired legacy ticket is now a valid pairing input and the offline preflight correctly reports offline. Rename to expiredLegacyTicketWhileOfflineReportsOfflineNotExpired and assert the offline message; still fails fast with no dial. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… first step minimalPairingCodeConnectsAndAdoptsHostReportedIdentity and minimalPairingCodePersistsPairedMacWithoutServerPushEvents polled an intermediate adoption step (in-memory connectedHostName / adopted device id) and then asserted the display-name upsert, which lands later on the serialized paired-Mac write chain. Under CI load the poll won that race and read displayName nil. Poll for the persisted display name (the sequence's final write) so the assertions only run once the state they check is durable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The known-name lookup for a nameless compact-QR ticket ran outside performSerializedPairedMacWrite, so a queued fresher-name write could land between the read and our upsert and get clobbered with the stale name. Move the lookup inside the serialized closure and prefer the current Stack user's record for the Mac before falling back to any account's record on this device. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The serialized display-name lookup fix adds 6 net lines. Accepting the small spillover instead of extracting mid-train; the liveness/paired-mac subsystem extraction is tracked as a follow-up refactor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-ios-qr-compact
branch
from
June 11, 2026 22:28
9ba98ee to
32ab233
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swift (1)
254-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap payload loopback rejection into
.loopbackRejectedin the central classifier.
MobilePairingFailureCategorynow defines.loopbackRejected, butclassify(error:route:)never returns it. AMobileSyncPairingPayloadError.loopbackRouteRejectedcurrently falls through to.unknown, so the new localized message and analytics reason are skipped on that path.Suggested fix
public static func classify(error: any Error, route: CmxAttachRoute?) -> MobilePairingFailureCategory { let hostPort = route.flatMap(hostPort(for:)) let host = hostPort?.host let port = hostPort?.port @@ if error is CancellationError { return .cancelled } + + if let payloadError = error as? MobileSyncPairingPayloadError { + switch payloadError { + case .loopbackRouteRejected: + return .loopbackRejected + case .expired: + return .ticketExpired + case .unsupportedVersion, .emptyHost, .invalidPort, + .forbiddenSecretField, .invalidURL, .invalidPayloadEncoding: + return .invalidCode + } + } + return .unknown(host: host, port: port) }🤖 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/MobilePairingFailure.swift` around lines 254 - 314, The classifier in classify(error: any Error, route: CmxAttachRoute?) never maps MobileSyncPairingPayloadError.loopbackRouteRejected to MobilePairingFailureCategory.loopbackRejected; add a check (e.g., after the MobileShellConnectionError block or before the final unknown return) that casts error as MobileSyncPairingPayloadError and returns .loopbackRejected when the case is .loopbackRouteRejected (preserving host/port via hostPort(for:) like other returns).
🤖 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/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Around line 121-123: Update isPairingCodeURL(_:) to require the cmux pairing
scheme and host in addition to the version query so only real pairing-code URLs
pass the public check: in isPairingCodeURL(_:) (and anywhere the same gate is
used before decode(_:), e.g., the decode(_:) caller path) ensure
components.scheme == "cmux-ios" and components.host == "attach" (or the exact
expected host string used by the pairing flow) AND that components.queryItems
contains v == "\(Self.version)". This tightens the check so arbitrary URLs with
v=2 cannot be treated as pairing-code shaped before decode(_:).
In `@Packages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swift`:
- Around line 74-87: resolvedType() currently picks the first matching inferred
shape (u → i → h+p) which silently accepts ambiguous payloads; change it so that
if t is set return it, otherwise compute flags for the three inferred shapes
(url: u != nil, peer: i != nil, host_port: h != nil && p != nil), count how many
are true and if exactly one is true return the corresponding string ("url",
"peer", or "host_port"), otherwise throw Self.corruptedEndpoint(...) to reject
missing or ambiguous combinations; update error message to indicate ambiguity
when multiple shape flags are present.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 1725-1742: cachedVerdict(auth:) only returns positive cached
bindings and returns nil for misses, causing repeated fresh Stack lookups for
the same bad token; add a negative-cache or token-keyed cooldown so failed
verifications are cached briefly and/or coalesce in-flight lookups to avoid
repeated getUser probes. Specifically, when Self.cacheKey(for: accessToken) is
missing or expired, record a short-lived failure entry (e.g., store a cached
object with userID = nil or a failed flag and expiresAt = Date() +
shortCooldown) and make cachedVerdict return false for that entry; alternatively
implement a per-token in-flight promise map keyed by Self.cacheKey(for:) to
await an ongoing verifyStackAuthOffMainActor(...) call instead of spawning
multiple lookups. Ensure the same cache key and the
currentAuthenticatedLocalUserID/authorizeStackUser checks remain consistent with
success-path caching.
In `@Sources/Mobile/Pairing/MobilePairingModel.swift`:
- Around line 188-202: The refresh() path must cancel any existing status
observer before starting a new one to avoid accumulating dormant subscriptions:
call stopObserving() (or at least cancel connectionObservationTask and nil it)
immediately before you subscribe to host.statusUpdates() when entering the
.ready branch (and also before early exits that return
.signedOut/.needsTailscale/.failed), ensuring the existing
connectionObservationTask is always cancelled prior to creating a new Task;
update refresh() to reference connectionObservationTask and stopObserving() so
the previous observer is torn down on every refresh/generation change.
- Around line 259-265: The rendered Tailscale route list still shows
loopback/dev endpoints because tailscaleLines(_:) isn't using the new
non-loopback contract; update tailscaleLines(_:) to filter its CmxAttachRoute
entries with MobilePairingModel.isPhoneReachableRoute(_:) (the same predicate
used for QR/manualEntry) so the view only renders routes that pass
isPhoneReachableRoute(_:), ensuring MobilePairingView.swift displays the exact
same non-loopback set that the QR/manual flow accepts.
In `@Sources/TerminalController.swift`:
- Line 11960: Replace the optional collection pattern for basePayload with an
explicit Result to avoid optional collections: introduce a variable like result:
Result<[String: Any], V2CallResult> initialized to a failure sentinel, set
result = .success(...) inside v2MainSync instead of assigning basePayload, and
then switch or use guard-case let to extract the payload (or return the .err
value) where you currently check basePayload; reference the existing basePayload
variable and v2MainSync callback to locate where to change assignments and
checks.
- Around line 11218-11229: The nonisolated(unsafe) observation introduces a
race: ensure the KVO is invalidated before v2AwaitCallback can return by moving
the observation invalidation into the v2AwaitCallback completion/timeout path
(i.e., call observation?.invalidate() and set observation = nil inside the
callback/timeout handler that invokes finish), or alternatively make observation
actor-isolated (remove nonisolated(unsafe) and store it on the main actor) so
all accesses to observation, webView.observe(\.url), and
observation?.invalidate() occur on the main actor (use v2MainSync for both
registering and invalidating); reference symbols: observation, v2AwaitCallback,
v2MainSync, webView.observe(_:options:), finish, and observation?.invalidate().
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swift`:
- Around line 254-314: The classifier in classify(error: any Error, route:
CmxAttachRoute?) never maps MobileSyncPairingPayloadError.loopbackRouteRejected
to MobilePairingFailureCategory.loopbackRejected; add a check (e.g., after the
MobileShellConnectionError block or before the final unknown return) that casts
error as MobileSyncPairingPayloadError and returns .loopbackRejected when the
case is .loopbackRouteRejected (preserving host/port via hostPort(for:) like
other returns).
🪄 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: dbc1ac09-6ff0-4f41-804c-95748d60360e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (46)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxLoopbackHost.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualPairingEntry.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRBitmap.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachRoute.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxLoopbackHostTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeCaptureController.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameCandidate.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameSelection.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeMetadataReceiver.swiftPackages/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileHostStatusVerificationLimiter.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingQRImageView.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/Mobile/Pairing/MobilePairingWindowController.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostStatusVerificationLimiterTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swift (1)
254-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap payload loopback rejection into
.loopbackRejectedin the central classifier.
MobilePairingFailureCategorynow defines.loopbackRejected, butclassify(error:route:)never returns it. AMobileSyncPairingPayloadError.loopbackRouteRejectedcurrently falls through to.unknown, so the new localized message and analytics reason are skipped on that path.Suggested fix
public static func classify(error: any Error, route: CmxAttachRoute?) -> MobilePairingFailureCategory { let hostPort = route.flatMap(hostPort(for:)) let host = hostPort?.host let port = hostPort?.port @@ if error is CancellationError { return .cancelled } + + if let payloadError = error as? MobileSyncPairingPayloadError { + switch payloadError { + case .loopbackRouteRejected: + return .loopbackRejected + case .expired: + return .ticketExpired + case .unsupportedVersion, .emptyHost, .invalidPort, + .forbiddenSecretField, .invalidURL, .invalidPayloadEncoding: + return .invalidCode + } + } + return .unknown(host: host, port: port) }🤖 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/MobilePairingFailure.swift` around lines 254 - 314, The classifier in classify(error: any Error, route: CmxAttachRoute?) never maps MobileSyncPairingPayloadError.loopbackRouteRejected to MobilePairingFailureCategory.loopbackRejected; add a check (e.g., after the MobileShellConnectionError block or before the final unknown return) that casts error as MobileSyncPairingPayloadError and returns .loopbackRejected when the case is .loopbackRouteRejected (preserving host/port via hostPort(for:) like other returns).
🤖 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/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Around line 121-123: Update isPairingCodeURL(_:) to require the cmux pairing
scheme and host in addition to the version query so only real pairing-code URLs
pass the public check: in isPairingCodeURL(_:) (and anywhere the same gate is
used before decode(_:), e.g., the decode(_:) caller path) ensure
components.scheme == "cmux-ios" and components.host == "attach" (or the exact
expected host string used by the pairing flow) AND that components.queryItems
contains v == "\(Self.version)". This tightens the check so arbitrary URLs with
v=2 cannot be treated as pairing-code shaped before decode(_:).
In `@Packages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swift`:
- Around line 74-87: resolvedType() currently picks the first matching inferred
shape (u → i → h+p) which silently accepts ambiguous payloads; change it so that
if t is set return it, otherwise compute flags for the three inferred shapes
(url: u != nil, peer: i != nil, host_port: h != nil && p != nil), count how many
are true and if exactly one is true return the corresponding string ("url",
"peer", or "host_port"), otherwise throw Self.corruptedEndpoint(...) to reject
missing or ambiguous combinations; update error message to indicate ambiguity
when multiple shape flags are present.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 1725-1742: cachedVerdict(auth:) only returns positive cached
bindings and returns nil for misses, causing repeated fresh Stack lookups for
the same bad token; add a negative-cache or token-keyed cooldown so failed
verifications are cached briefly and/or coalesce in-flight lookups to avoid
repeated getUser probes. Specifically, when Self.cacheKey(for: accessToken) is
missing or expired, record a short-lived failure entry (e.g., store a cached
object with userID = nil or a failed flag and expiresAt = Date() +
shortCooldown) and make cachedVerdict return false for that entry; alternatively
implement a per-token in-flight promise map keyed by Self.cacheKey(for:) to
await an ongoing verifyStackAuthOffMainActor(...) call instead of spawning
multiple lookups. Ensure the same cache key and the
currentAuthenticatedLocalUserID/authorizeStackUser checks remain consistent with
success-path caching.
In `@Sources/Mobile/Pairing/MobilePairingModel.swift`:
- Around line 188-202: The refresh() path must cancel any existing status
observer before starting a new one to avoid accumulating dormant subscriptions:
call stopObserving() (or at least cancel connectionObservationTask and nil it)
immediately before you subscribe to host.statusUpdates() when entering the
.ready branch (and also before early exits that return
.signedOut/.needsTailscale/.failed), ensuring the existing
connectionObservationTask is always cancelled prior to creating a new Task;
update refresh() to reference connectionObservationTask and stopObserving() so
the previous observer is torn down on every refresh/generation change.
- Around line 259-265: The rendered Tailscale route list still shows
loopback/dev endpoints because tailscaleLines(_:) isn't using the new
non-loopback contract; update tailscaleLines(_:) to filter its CmxAttachRoute
entries with MobilePairingModel.isPhoneReachableRoute(_:) (the same predicate
used for QR/manualEntry) so the view only renders routes that pass
isPhoneReachableRoute(_:), ensuring MobilePairingView.swift displays the exact
same non-loopback set that the QR/manual flow accepts.
In `@Sources/TerminalController.swift`:
- Line 11960: Replace the optional collection pattern for basePayload with an
explicit Result to avoid optional collections: introduce a variable like result:
Result<[String: Any], V2CallResult> initialized to a failure sentinel, set
result = .success(...) inside v2MainSync instead of assigning basePayload, and
then switch or use guard-case let to extract the payload (or return the .err
value) where you currently check basePayload; reference the existing basePayload
variable and v2MainSync callback to locate where to change assignments and
checks.
- Around line 11218-11229: The nonisolated(unsafe) observation introduces a
race: ensure the KVO is invalidated before v2AwaitCallback can return by moving
the observation invalidation into the v2AwaitCallback completion/timeout path
(i.e., call observation?.invalidate() and set observation = nil inside the
callback/timeout handler that invokes finish), or alternatively make observation
actor-isolated (remove nonisolated(unsafe) and store it on the main actor) so
all accesses to observation, webView.observe(\.url), and
observation?.invalidate() occur on the main actor (use v2MainSync for both
registering and invalidating); reference symbols: observation, v2AwaitCallback,
v2MainSync, webView.observe(_:options:), finish, and observation?.invalidate().
---
Outside diff comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swift`:
- Around line 254-314: The classifier in classify(error: any Error, route:
CmxAttachRoute?) never maps MobileSyncPairingPayloadError.loopbackRouteRejected
to MobilePairingFailureCategory.loopbackRejected; add a check (e.g., after the
MobileShellConnectionError block or before the final unknown return) that casts
error as MobileSyncPairingPayloadError and returns .loopbackRejected when the
case is .loopbackRouteRejected (preserving host/port via hostPort(for:) like
other returns).
🪄 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: dbc1ac09-6ff0-4f41-804c-95748d60360e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (46)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxLoopbackHost.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualPairingEntry.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRBitmap.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachRoute.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxLoopbackHostTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeCaptureController.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameCandidate.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeFrameSelection.swiftPackages/CmuxMobileCamera/Sources/CmuxMobileCamera/QRCodeMetadataReceiver.swiftPackages/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileHostStatusVerificationLimiter.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingQRImageView.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/Mobile/Pairing/MobilePairingWindowController.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostStatusVerificationLimiterTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
🛑 Comments failed to post (7)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift (1)
121-123:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden v2 URL identity checks in the public decode path.
Line 121 currently treats any URL with
v=2as pairing-code shaped. Because Line 148 gatesdecode(_:)on that check, direct callers can decode non-cmux-ios://attachcomponents. MakeisPairingCodeURL(_:)enforce scheme+host too.Suggested fix
public func isPairingCodeURL(_ components: URLComponents) -> Bool { - components.queryItems?.first(where: { $0.name == "v" })?.value == "\(Self.version)" + components.scheme == "cmux-ios" + && components.host == "attach" + && components.queryItems?.first(where: { $0.name == "v" })?.value == "\(Self.version)" }Also applies to: 147-150
🤖 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/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift` around lines 121 - 123, Update isPairingCodeURL(_:) to require the cmux pairing scheme and host in addition to the version query so only real pairing-code URLs pass the public check: in isPairingCodeURL(_:) (and anywhere the same gate is used before decode(_:), e.g., the decode(_:) caller path) ensure components.scheme == "cmux-ios" and components.host == "attach" (or the exact expected host string used by the pairing flow) AND that components.queryItems contains v == "\(Self.version)". This tightens the check so arbitrary URLs with v=2 cannot be treated as pairing-code shaped before decode(_:).Packages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swift (1)
74-87:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject ambiguous inferred endpoint shapes when
tis absent.Line 74 infers by first match (
u→i→h+p). A payload containing multiple endpoint key sets is silently coerced to one type instead of being rejected, which can decode corrupted data into the wrong endpoint shape. Require exactly one inferred shape and throw on ambiguity.Suggested fix
private func resolvedType() throws -> String { if let t { return t } - if u != nil { - return "url" - } - if i != nil { - return "peer" - } - if h != nil, p != nil { - return "host_port" - } - throw Self.corruptedEndpoint("Attach endpoint carries no recognizable fields") + let inferred = [ + u != nil ? "url" : nil, + i != nil ? "peer" : nil, + (h != nil && p != nil) ? "host_port" : nil, + ].compactMap { $0 } + guard inferred.count == 1, let only = inferred.first else { + throw Self.corruptedEndpoint("Attach endpoint must carry exactly one endpoint shape") + } + return 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/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachEndpoint.swift` around lines 74 - 87, resolvedType() currently picks the first matching inferred shape (u → i → h+p) which silently accepts ambiguous payloads; change it so that if t is set return it, otherwise compute flags for the three inferred shapes (url: u != nil, peer: i != nil, host_port: h != nil && p != nil), count how many are true and if exactly one is true return the corresponding string ("url", "peer", or "host_port"), otherwise throw Self.corruptedEndpoint(...) to reject missing or ambiguous combinations; update error message to indicate ambiguity when multiple shape flags are present.Sources/Mobile/MobileHostService.swift (1)
1725-1742:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a short negative cache or token-keyed cooldown for failed status verifications.
cachedVerdict(auth:)only short-circuits fresh positive bindings. Onmobile.host.status, the same bad token therefore falls through to a fresh Stack lookup every time a limiter slot frees. Because the limiter only caps concurrency, not repeated misses, an unauthenticated peer can keep driving continuous outboundgetUsertraffic and keep the slot pool occupied with a small loop of rejected probes. Cache failed verdicts briefly, or coalesce in-flight lookups per token, before re-enteringverifyStackAuthOffMainActor(...).🤖 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 `@Sources/Mobile/MobileHostService.swift` around lines 1725 - 1742, cachedVerdict(auth:) only returns positive cached bindings and returns nil for misses, causing repeated fresh Stack lookups for the same bad token; add a negative-cache or token-keyed cooldown so failed verifications are cached briefly and/or coalesce in-flight lookups to avoid repeated getUser probes. Specifically, when Self.cacheKey(for: accessToken) is missing or expired, record a short-lived failure entry (e.g., store a cached object with userID = nil or a failed flag and expiresAt = Date() + shortCooldown) and make cachedVerdict return false for that entry; alternatively implement a per-token in-flight promise map keyed by Self.cacheKey(for:) to await an ongoing verifyStackAuthOffMainActor(...) call instead of spawning multiple lookups. Ensure the same cache key and the currentAuthenticatedLocalUserID/authorizeStackUser checks remain consistent with success-path caching.Sources/Mobile/Pairing/MobilePairingModel.swift (2)
188-202:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the stale
statusUpdates()observer on every refresh.
refreshGenerationstops stale state writes, but it does not tear down the observer created by the previous.readystate. When a laterrefresh()exits through.signedOut,.needsTailscale, or.failed, the oldhost.statusUpdates()loop stays subscribed until the host emits another update, so repeated refreshes can accumulate dormant observers.Suggested fix
func refresh() async { + connectionObservationTask?.cancel() + connectionObservationTask = nil refreshGeneration &+= 1 let generation = refreshGeneration state = .loading🤖 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 `@Sources/Mobile/Pairing/MobilePairingModel.swift` around lines 188 - 202, The refresh() path must cancel any existing status observer before starting a new one to avoid accumulating dormant subscriptions: call stopObserving() (or at least cancel connectionObservationTask and nil it) immediately before you subscribe to host.statusUpdates() when entering the .ready branch (and also before early exits that return .signedOut/.needsTailscale/.failed), ensuring the existing connectionObservationTask is always cancelled prior to creating a new Task; update refresh() to reference connectionObservationTask and stopObserving() so the previous observer is torn down on every refresh/generation change.
259-265:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the rendered route list on the same non-loopback contract.
isPhoneReachableRoute(_:)now defines which routes are safe enough to show a QR for, buttailscaleLines(_:)still includes every.tailscaleendpoint.Sources/Mobile/Pairing/MobilePairingView.swiftrenders each of those lines, so a loopback/dev route can still appear in the pairing window even though the QR andmanualEntrypath reject it.Suggested fix
private static func tailscaleLines(_ routes: [CmxAttachRoute]) -> [String] { routes.compactMap { route in - guard route.kind == .tailscale, + guard isPhoneReachableRoute(route), case let .hostPort(host, port) = route.endpoint else { return nil } return "\(host):\(port)" } }🤖 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 `@Sources/Mobile/Pairing/MobilePairingModel.swift` around lines 259 - 265, The rendered Tailscale route list still shows loopback/dev endpoints because tailscaleLines(_:) isn't using the new non-loopback contract; update tailscaleLines(_:) to filter its CmxAttachRoute entries with MobilePairingModel.isPhoneReachableRoute(_:) (the same predicate used for QR/manualEntry) so the view only renders routes that pass isPhoneReachableRoute(_:), ensuring MobilePairingView.swift displays the exact same non-loopback set that the QR/manual flow accepts.Sources/TerminalController.swift (2)
11218-11229: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Review the
nonisolated(unsafe)KVO observation pattern for race conditions.The
observationvariable is declarednonisolated(unsafe)and used in a callback-based KVO pattern (lines 11218-11229). The sequence is:
- Register the observation on the main actor (inside
v2MainSync)- The KVO callback can fire on any thread when
webView.urlchanges- After
v2AwaitCallbackreturns (via timeout or callback),invalidate()is called on the main actorPotential race: If the KVO fires after the timeout expires but before
invalidate()is called, thefinish(true)callback would be invoked after the function has already returned. Whilev2AwaitCallbacklikely guards against double-invocation, thenonisolated(unsafe)annotation bypasses Swift's concurrency safety checks, so any implementation flaw would not be caught by the compiler.Safer alternatives:
- Invalidate the observation before returning from
v2AwaitCallback(e.g., inside the timeout block)- Use an actor-isolated observation state instead of
nonisolated(unsafe)- Consider structured concurrency (async/await with
withTaskCancellationHandler) instead of callback-based KVOIf
v2AwaitCallbackhas robust once-only callback semantics and the KVO callback is idempotent, the current pattern may be safe. However,nonisolated(unsafe)is a strong escape hatch that should be carefully justified and documented.🤖 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 `@Sources/TerminalController.swift` around lines 11218 - 11229, The nonisolated(unsafe) observation introduces a race: ensure the KVO is invalidated before v2AwaitCallback can return by moving the observation invalidation into the v2AwaitCallback completion/timeout path (i.e., call observation?.invalidate() and set observation = nil inside the callback/timeout handler that invokes finish), or alternatively make observation actor-isolated (remove nonisolated(unsafe) and store it on the main actor) so all accesses to observation, webView.observe(\.url), and observation?.invalidate() occur on the main actor (use v2MainSync for both registering and invalidating); reference symbols: observation, v2AwaitCallback, v2MainSync, webView.observe(_:options:), finish, and observation?.invalidate().
11960-11960: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using a non-optional result type instead of optional collections.
SwiftLint flags
var basePayload: [String: Any]?as a discouraged optional collection (lines 11960, 13422). While the pattern is semantically justified (usingnilas a sentinel for "resolution failed"), SwiftLint's guidance points to more idiomatic alternatives:Current pattern:
var basePayload: [String: Any]? v2MainSync { /* conditionally set basePayload */ } guard let payload = basePayload else { return .err(...) }Alternatives:
Use a
Resulttype to distinguish success/failure:var result: Result<[String: Any], V2CallResult> = .failure(.err(...)) v2MainSync { result = .success(...) } switch result { case .success(let payload): ... }Use a tuple with an explicit flag:
var outcome: (found: Bool, payload: [String: Any]) = (false, [:]) v2MainSync { outcome = (true, [...]) } guard outcome.found else { return .err(...) }These alternatives make the "found vs. not-found" distinction more explicit and avoid the "two empty states" issue that SwiftLint warns about.
Also applies to: 13422-13422
🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 11960-11960: Prefer empty collection over optional collection
(discouraged_optional_collection)
🤖 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 `@Sources/TerminalController.swift` at line 11960, Replace the optional collection pattern for basePayload with an explicit Result to avoid optional collections: introduce a variable like result: Result<[String: Any], V2CallResult> initialized to a failure sentinel, set result = .success(...) inside v2MainSync instead of assigning basePayload, and then switch or use guard-case let to extract the payload (or return the .err value) where you currently check basePayload; reference the existing basePayload variable and v2MainSync callback to locate where to change assignments and checks.
… wait The final expectation ran immediately after the second replay REQUEST was observed, but the response still flows back asynchronously, so the test lost the race deterministically on the slower ipad simulator (surfaced now that the ios-simulator success override catches Swift Testing failures). Poll for line delivery before asserting, the same bounded wait the sibling tests already use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 12, 2026
Re-applies origin/feat-ios-dog-unified (771f532, dog 11b) onto current origin/main (f2627b9). Main's reviewed forms win for landed features (composer #5876, QR #5872, presence gate #5912, CI fix #5906): seven pairing/RPC/transport files take main's private-extension structure, persistPairedMacFromTicket keeps main's serialized-write-chain lookup, QR pairing tests keep main's durable-write polls. Dog-carried unlanded work is preserved: MobileHostService+Capabilities.swift superset (notification.dismiss.v1, terminal.paste.v1, workspace.groups.v1, DEBUG dogfood verbs) replaces main's inline subset var, dismiss-sync observer and capability flag resets kept, dogfood pane model kept.
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jun 12, 2026
…browser CLI, pairing QR, iOS PRs included: - manaflow-ai#5816 ControlCommandCoordinator extraction (package coordinator skeleton; fork keeps legacy v2* dispatchers) - manaflow-ai#5859 sidebar perf - manaflow-ai#5857 RendererRealization (added as SurfaceHibernation adapter) - manaflow-ai#5867 in-process custom sidebars - manaflow-ai#5778 browser CLI / system-proxy bypass - manaflow-ai#5872 minimal pairing QR - iOS pairing/manual-entry stack - 30+ hot fixes Fork-side adjustments: - Skip 21 TerminalController+Control* extension files (PR manaflow-ai#5816 architecture refactor not adopted) - Add Sources/App/RendererRealizationSettingsAdapter.swift to bridge new RendererRealizationSettings to fork's existing SurfaceHibernationSettings - Restore v2SurfaceDragToSplit shim removed by upstream - Add SettingsNavigationTarget.customSidebars case - Stub ghostty_surface_set_renderer_realized callsites pending GhosttyKit rebuild (zig 0.15.2 required, host has 0.16.0) - Update ghostty submodule to 44b2baa81 (cherry-pick the 3 renderer commits onto fork's manaflow-ai#5128 link-fix pointer) - Keep fork's CMUXSessionDaemon module pbxproj refs and SurfaceHibernation settings
azooz2003-bit
added a commit
that referenced
this pull request
Jun 12, 2026
Main landed #5872 (full-width native-module QR render, routes-only payload) and scanner tuning that is a superset of this branch's near-focus restriction, so this merge takes main's side everywhere. The branch now carries no changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 13, 2026
lawrencecchen
added a commit
that referenced
this pull request
Jun 13, 2026
…#5596/#5625/#5628) over current main Beta queue (#5876/#5872/#5869/#5875/#5927/#5912/#5726/#5776/#5916) is now on main; conflicts resolved by taking main as authoritative for the merged workspace-list/notifications/read-state/close surface, while preserving the carry-set: terminal.paste capability (#5572), hidden-input strings (#5596), smooth-scroll/scroll-to-bottom (#5628), and the live notifications feed (notificationsStore + mobile.notifications.list/mark_read dispatch). Dropped the superseded mute design. Capability flags unified onto main's computed supportedHostCapabilities set (added computed supportsTerminalPaste + DEBUG supportsDogfoodChecklist). xcstrings merged (HEAD-precedence union, mute keys dropped). pbxproj took HEAD consistently; budget regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 27, 2026
azooz2003-bit
added a commit
that referenced
this pull request
Aug 21, 2026
…0499) * test(mobile): pairing-window Tailscale code must be minimal v2, not full-key JSON The omitted-target legacy disclosure path still emits the base64 full-key v1 ticket for any Tailscale route: ~790 characters that render a version-23 (109x109-module) QR and disclose the Mac's device id, display name, and build metadata to anything that photographs the pairing window. Fielded clients have decoded the plain v2 grammar since #5872, and the phone recovers all of that metadata post-handshake from mobile.host.status. Red half of the regression pair: asserts the pairing window's Tailscale compatibility code speaks the v2 grammar, carries only routes plus the ub account binding and pc compatibility level, and stays at or below QR version 8 at ECC M. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(mobile): emit the minimal v2 grammar for the pairing window's Tailscale code The pairing window's Tailscale compatibility QR was still the base64 full-key v1 JSON ticket: ~790 characters rendering a version-23 (109x109-module) QR whose payload disclosed the Mac's device id, display name, Stack user id, and app version/build to anything that photographs the pairing window. Fielded iOS clients have decoded the plain v2 grammar since #5872, and every field beyond the routes is either consulted post-handshake via mobile.host.status or never needed at all. The compatibility disclosure now reindexes mixed route snapshots down to the canonical Tailscale subsequence (sharing the physical-device target's canonicalization) and emits the v2 grammar with only the two fields the phone consults before dialing: ub, the opaque account binding the pairing preflight matches for the wrong-account fast-fail (#6028), and pc, the compatibility level fielded decoders default to 0 when absent (omitting it would spuriously fire the cross-version warning). av/ab are no longer written anywhere; the decoder still reads them from older Macs' codes. Tickets the v2 grammar cannot express (workspace-scoped, escaped hosts) keep the compact v1 fallback, and CmxLegacyPrivateNetworkPairingCode is deleted with its last caller. A realistic account-bound two-route code now renders QR version 8 or lower at ECC M (49x49 modules, asserted through the real encoder in CmxPairingQRBitmapTests), so each module is ~2.6x larger on screen than before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplifies iPhone pairing from the Mac pairing window (Settings → Mobile → Pair a Device, the window from #5493).
Minimal payload. The pairing QR now speaks a plain v2 grammar:
cmux-ios://attach?v=2&r=<host>:<port>[&r=...], Tailscale routes only. The vestigial auth token, expiry, display name, and device id are gone from the code; authorization is the Stack same-account check at the host, and the Mac's identity arrives post-handshake frommobile.host.status. Workspace-scoped tickets and RPC consumers keep the compact v1 JSON payload, and the iOS decoder still accepts all three grammars. A test prints and asserts the size drop: a representative 2-route code stays under 100 bytes / QR version 6 at ECC M (the old base64-JSON payload was several versions higher).Localhost refusal. A scannable code can never point the phone at itself. The encoder drops a DEBUG Mac's dev loopback route and refuses to encode a ticket with no Tailscale route; the pairing window shows localized set-up-Tailscale guidance instead of a useless QR (
mobile.pairing.needsTailscale.body); the iOS decoder rejects loopback hosts in every grammar with a localized explanation (mobile.pairing.loopbackRejected). Loopback is classified by parsed address bytes (127.0.0.0/8, ::1, IPv4-mapped forms,localhost), not string matching. This is also a pairing-latency fix: a scanned loopback route used to sort first and park the phone in anNWConnection.waitingblack hole before the real route was tried.Much bigger render. The QR renders at the QR's native module resolution and upscales with interpolation disabled, filling the window width (resizable for more) instead of a fixed 220 pt square, with ECC level M and the smaller payload keeping the version low so each module is larger on both axes.
Copy IP / Copy Port. Two separate buttons under the manual-entry fallback copy the best phone-dialable route's host and port to
NSPasteboard, with a brief "Copied" flash. Localized en + ja.No expiry. The displayed code never expires and is never regenerated on a timer; the
ticketTTL - 30re-mint task was removed. A decoded code that sat on screen for an hour still pairs (decodedTicketStillPairsLongAfterMint). Refresh Code remains as the manual path if a Tailscale address changes.Tests.
swift testgreen on all five touched packages: CMUXMobileCore (95), CmuxMobileRPC (29), CmuxMobileShellModel (30), CmuxMobileTransport (24), CmuxMobileShell (55). Coverage includes round-trip encode/decode of the v2 grammar, encode refusal for loopback-only tickets, decode rejection of loopback hosts (parameterized across 127.x/::1/localhost/IPv4-mapped), hostile route-count caps, malformed-route rejection, and the payload-size/QR-version assertion. Compile-verified: macOS Debug (/tmp/cmux-qrc) and iOS arm64 simulator viaios/cmux.xcworkspace(/tmp/cmux-ios-qrc), both BUILD SUCCEEDED.Localization audit. All 36 pairing-window keys used by the changed macOS surfaces verified translated en + ja in
Resources/Localizable.xcstrings(including newmobile.pairing.manual.copyIP,mobile.pairing.manual.copyPort,mobile.pairing.manual.copied); all 29 keys used by the changed iOS shell surfaces verified en + ja inios/cmux/Resources/Localizable.xcstrings(including newmobile.pairing.loopbackRejected).Scannability round (dogfood: "make qr code bigger and easier to scan. camera app somehow works better").
Mac render. The 20pt fixed padding gave only ~2.7 modules of quiet zone at the default window size, under the 4-module ISO/IEC 18004 minimum that third-party scanners depend on. Rendering moved into
CmxPairingQRBitmap(CMUXMobileCore, beside the payload codec): one pixel per module, pure black on pure white regardless of theme, with the full 4-module quiet zone baked into the bitmap so it scales with the code and cannot be cropped by layout. The view still upscales with interpolation disabled, so modules stay sharp nearest-neighbor squares. The default window grows from 460x640 to 540x720 so the full-width code renders larger out of the box.ECC decision: L to M. The routes-only payload is small enough that M keeps the code at version 6 or lower (the version test now asserts against the ECC-M capacity table), and M's redundancy absorbs the glare and off-angle blur of scanning a glossy screen. L's one advantage, larger modules, is not the binding constraint at these payload sizes.
iOS scanner: why the Camera app won. Three capture-setup causes. First, the scanner opened the bare default wide-angle camera; on recent Pro iPhones that lens cannot focus nearer than ~20 cm, exactly where people hold the phone to a Mac screen, so it hunts while the Camera app silently switches to the close-focusing ultra-wide. The scanner now opens the best virtual multi-lens device (triple, then dual-wide, dual, wide) so automatic constituent switching does the same. Second, no focus tuning; now continuous AF with near range restriction, smooth AF off, continuous exposure, and low-light boost, all capability-guarded. Third, the metadata callback read only
metadataObjects.first, so any other detection ordered before the pairing QR in a frame masked it; selection now considers every detection via the testableQRCodeFrameSelection. Session preset pinned to 1080p where supported.rectOfInterestwas audited and is correct: it was never set, so it scans the full frame (no mis-mapped viewfinder bug); a comment documents themetadataOutputRectConverted(fromLayerRect:)requirement for anyone scoping it later. Torch stays absent on purpose: the code is on a backlit screen and a torch on glossy glass washes it out.Round evidence. CMUXMobileCore 98 tests green (3 new pixel-level bitmap tests: white quiet-zone ring, pure black/white output, module-count vs version arithmetic that also pins the generator's 1-module-margin assumption). CmuxMobileCamera 7 green (5 new frame-selection tests). Lens switching and AF are not CI-testable; manual check is scanning with the in-app scanner on a Pro iPhone at close range. Compile-verified macOS Debug and iOS arm64 simulator, both BUILD SUCCEEDED. No new user-facing strings.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes mobile pairing trust boundaries (loopback rejection, optional ticket expiry, Stack-gated identity on status) and connect routing behavior; broad test coverage mitigates regressions but auth and persistence paths deserve careful review.
Overview
iPhone pairing is reworked around a smaller, safer scan path: the Mac pairing window prefers a v2 QR (
cmux-ios://attach?v=2&r=host:port…) with Tailscale routes only—no token, expiry, name, or device id in the code—while compact v1 JSON remains for scoped tickets and lossy cases.Trust and identity: Shared
CmxLoopbackHostblocks loopback in v2 decode and on physical devices for legacy payloads (simulator dev pairing unchanged).CmxAttachTicketno longer fails validation on expiry; pairing is not gated on ticket age—isExpiredapplies only where attach tokens are used. After connect,mobile.host.statusmay returnmac_device_id/mac_display_nameonly for verified Stack callers (cached + rate-limited verifications); the iOS shell adopts and persists identity with stale-reply guards and serialized paired-Mac writes.UX and latency: Mac UI drops timer-based QR refresh, requires a phone-dialable Tailscale route before showing a code, adds Copy IP/Port, and renders a full-width ECC M QR with baked quiet zone. Transport fast-fails initial connects on connection refused instead of waiting the full timeout; the camera scanner picks multi-lens devices, near focus, and scans all QR candidates in a frame.
Reviewed by Cursor Bugbot for commit 11d2795. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make iPhone pairing faster and safer with a minimal v2 QR (routes‑only), sharp full‑width rendering, loopback rejection across grammars, Copy IP/Port, verified post‑handshake identity, and codes that never expire. This shortens scan‑to‑pair time, preserves trusted identity, and avoids self‑dial and stale codes.
New Features
cmux-ios://attach?v=2&r=<host>:<port>[&r=...]with Tailscale routes only; no token, no expiry, no name, no device id. Decoder accepts compact and legacy payloads; falls back to compact v1 when non‑Tailscale fallback routes must be kept.mobile.host.statusreturnsmac_display_name/mac_device_idonly to verified same‑account callers with a capped verifier; client adopts/persists with serialized writes, retries on anonymous/timeout, and supports no‑push runtimes.expiresAtis optional and enforced only when an attach token is used.Bug Fixes
Written for commit 11d2795. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests