Require matching email for iOS pairing#6028
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:
📝 WalkthroughWalkthroughExtend attachment tickets with Mac email, pairing-compat version, app version, and build fields; expand v2 pairing QR to encode/decode those fields; add email-mismatch classification and an approval-gated pairing-version warning flow with UI, localization, and tests. ChangesMac Pairing with Email, App Metadata, and Version Skew Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: ddaaa864e5
ℹ️ 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".
| guard let phoneVersion = Self.normalizedNonEmpty(phoneStamp.appVersion), | ||
| phoneVersion != macVersion else { |
There was a problem hiding this comment.
This check compares the iPhone bundle marketing version directly with the Mac bundle marketing version, but those are currently different version streams in this repo (iOS ios/Config/Shared.xcconfig has MARKETING_VERSION = 1.0.0, while the Mac project has MARKETING_VERSION = 0.64.15). With matching current builds, every v2 QR therefore returns .needsUserApproval before dialing and shows a destructive “Version mismatch” warning even when there is no real compatibility skew; consider comparing a shared protocol/compatibility version or otherwise accounting for the expected Mac/iOS version scheme.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds account and build-identity metadata to pairing tickets and QR codes, and enforces a pre-dial email/user-ID match on iOS before the phone dials the Mac. App-version skew now surfaces a user-approvable warning instead of a hard block.
Confidence Score: 4/5Safe to merge after addressing the nil-identity silent pass in emailFailure; the version-warning and attach-URL deferral logic are sound. The pre-dial user-ID check in emailFailure returns nil (no failure) when ticket.macUserID is set but the phone's identity provider returns nil, silently skipping the check that is meant to block wrong-account pairing. A phone mid-sign-in or with a temporarily broken identity provider can scan a user-ID-scoped QR and proceed to dial. The Mac-side token auth is still a backstop, but the phone-side guard should not fail open. Everything else — wire-format additions, version-warning flow, session-restore deferral, localization — looks correct. MobileShellComposite.swift (emailFailure nil-identity path) and CompactAttachTicket.swift (@ heuristic for u field disambiguation) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS UI (QR / attach URL)
participant MSC as MobileShellComposite
participant ID as IdentityProvider
participant Mac as Mac (dial)
UI->>MSC: connectPairingURLResult(rawURL)
MSC->>MSC: beginPairingValidationAttempt()
MSC->>MSC: CmxAttachTicketInput.decode(rawURL)
alt decode error
MSC-->>UI: .failed
end
MSC->>ID: currentUserID / currentUserEmail
MSC->>MSC: emailFailure(ticket, actualUserID, actualEmail)
alt userID or email mismatch
MSC-->>UI: .failed
end
MSC->>MSC: versionWarning(ticket)
alt compatibility version differs
MSC->>MSC: store pendingPairingVersionWarningURL
MSC-->>UI: .needsUserApproval
UI->>UI: show version-warning alert
alt user accepts
UI->>MSC: acceptPairingVersionWarning()
MSC->>MSC: "connectPairingURLResult(stored URL, accepted=true)"
else user cancels
UI->>MSC: cancelPairing()
MSC-->>UI: clears warning
end
end
MSC->>MSC: beginPairingAttempt()
MSC->>Mac: dial routes
Mac-->>MSC: connected / failed
MSC-->>UI: .connected / .failed
Reviews (12): Last reviewed commit: "Restore pairing validation analytics" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func authorizeStackEmail(localEmail: String?, remoteEmail: String?) throws { | ||
| guard let localEmail = normalizedEmail(localEmail) else { | ||
| throw MobileHostAuthorizationError.missingLocalUser | ||
| } | ||
| guard localUserID == remoteUserID else { | ||
| guard normalizedEmail(remoteEmail) == localEmail else { | ||
| throw MobileHostAuthorizationError.accountMismatch | ||
| } | ||
| } |
There was a problem hiding this comment.
authorizeStackEmail accepts remoteEmail: String? and treats a nil value identically to a mismatched email: normalizedEmail(nil) returns nil, nil != localEmail (which is non-nil after the guard), so the function throws accountMismatch. The old authorizeStackUser took remoteUserID: String (non-optional) because user.id was always present. The new path fetches user.primaryEmail — which the Swift type marks as optional — caches it (including nil), and passes it here without any nil guard. Any Stack account whose primaryEmail returns nil would be permanently locked out of the Mac host with a misleading "different cmux account" error, despite potentially being the legitimate owner.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift`:
- Around line 1437-1488: Add a sibling test to cover the cancel/dismiss flow:
duplicate the setup from
minimalPairingCodeVersionMismatchWarnsAndContinuesAfterAcceptance(), call
store.cancelPairing() after verifying the initial result is .needsUserApproval
(from store.connectPairingURLResult), then assert that
store.pairingVersionWarning is nil, store.connectionState remains .disconnected,
and that no workspace RPCs were sent (use try await responses.sentRequests()).
Name the test something like
minimalPairingCodeVersionMismatchClearsWarningOnCancel and use the same
ScriptedTransportResponses, testRuntime, store.signIn(), and
connectPairingURLResult invocation so it mirrors the acceptance test.
- Around line 1407-1435: In the test
minimalPairingCodeRequiresMatchingEmailBeforeDialing add an explicit assertion
that store.pairingVersionWarning is nil to ensure the failure is due to email
mismatch rather than a version warning; locate the test method and append a
check like XCTAssertNil(store.pairingVersionWarning) (or your test-framework
equivalent) alongside the existing expectations so the distinction between
email-mismatch errors and version-skew warnings is enforced.
In `@Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Around line 270-274: The percentEncodeQueryValue(_:) currently falls back to
returning the raw unencoded value on failure which can inject reserved
characters into query strings; change it to return a safe fallback (e.g., empty
string or a fixed placeholder like "__ENCODING_FAILED__") instead of the raw
value, and adjust callers that build the query (where percentEncodeQueryValue is
used) to filter out or skip parameters whose encoded value is empty or the
placeholder so malformed values are not inserted into the URL; update references
to percentEncodeQueryValue and the query-building logic to implement this
defensive behavior.
In `@Packages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swift`:
- Around line 20-29: The CompactAttachTicket.normalizedNonEmpty function
currently checks !value.isEmpty without trimming, causing whitespace-only
strings to be kept; update normalizedNonEmpty to trim whitespace (e.g., call
.trimmingCharacters(in: .whitespacesAndNewlines)) and return nil if the trimmed
result is empty so its behavior matches CmxPairingQRCode.normalizedNonEmpty and
MobileHostBuildIdentity.normalized; ensure this change applies to all callers in
CompactAttachTicket.init (workspaceID, terminalID, macUserEmail, macAppVersion,
macAppBuild) and verify trimming is acceptable for those fields.
🪄 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: 619e33bd-d3bc-44c7-90e9-4c5779d4321d
📒 Files selected for processing (23)
Packages/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingFailureTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobilePairingURLConnectionResult.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swiftPackages/CmuxMobileWorkspace/Sources/CmuxMobileWorkspace/MobileRootAuthGate.swiftPackages/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostBuildIdentity.swiftSources/Mobile/MobileHostService.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostAuthorizationTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
| @MainActor | ||
| @Test func minimalPairingCodeRequiresMatchingEmailBeforeDialing() async throws { | ||
| let responses = ScriptedTransportResponses([ | ||
| try rpcWorkspaceListFrame(workspaceID: "qr-workspace", title: "QR Workspace"), | ||
| ]) | ||
| let runtime = testRuntime( | ||
| supportedRouteKinds: [.tailscale], | ||
| transportFactory: ScriptedTransportFactory(responses: responses) | ||
| ) | ||
| let store = CMUXMobileShellStore( | ||
| runtime: runtime, | ||
| workspaces: PreviewMobileHost.workspaces, | ||
| identityProvider: TestIdentityProvider( | ||
| currentUserIDValue: "phone-user", | ||
| currentUserEmailValue: "phone@example.com" | ||
| ) | ||
| ) | ||
|
|
||
| store.signIn() | ||
| let result = await store.connectPairingURLResult( | ||
| "cmux-ios://attach?v=2&e=mac@example.com&r=100.71.210.41:\(CmxMobileDefaults.defaultHostPort)" | ||
| ) | ||
|
|
||
| #expect(result == .failed) | ||
| #expect(store.connectionState == .disconnected) | ||
| #expect(store.connectionError?.contains("mac@example.com") == true) | ||
| #expect(store.connectionError?.contains("phone@example.com") == true) | ||
| #expect(try await responses.sentRequests().isEmpty) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider verifying pairingVersionWarning is nil.
The test validates email mismatch rejection, but doesn't explicitly assert that store.pairingVersionWarning == nil. Adding this would strengthen the distinction between email-mismatch failures (which should not show a version warning) and version-skew scenarios.
Optional assertion to add
`#expect`(store.connectionState == .disconnected)
`#expect`(store.connectionError?.contains("mac@example.com") == true)
`#expect`(store.connectionError?.contains("phone@example.com") == true)
+#expect(store.pairingVersionWarning == nil)
`#expect`(try await responses.sentRequests().isEmpty)🤖 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 `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift` around lines
1407 - 1435, In the test minimalPairingCodeRequiresMatchingEmailBeforeDialing
add an explicit assertion that store.pairingVersionWarning is nil to ensure the
failure is due to email mismatch rather than a version warning; locate the test
method and append a check like XCTAssertNil(store.pairingVersionWarning) (or
your test-framework equivalent) alongside the existing expectations so the
distinction between email-mismatch errors and version-skew warnings is enforced.
| @MainActor | ||
| @Test func minimalPairingCodeVersionMismatchWarnsAndContinuesAfterAcceptance() async throws { | ||
| let responses = ScriptedTransportResponses([ | ||
| try rpcWorkspaceListFrame(workspaceID: "qr-workspace", title: "QR Workspace"), | ||
| try rpcHostStatusFrame( | ||
| renderGrid: false, | ||
| macDeviceID: "status-reported-mac", | ||
| macDisplayName: "Status Mac" | ||
| ), | ||
| ]) | ||
| let runtime = testRuntime( | ||
| supportedRouteKinds: [.tailscale], | ||
| transportFactory: ScriptedTransportFactory(responses: responses), | ||
| supportsServerPushEvents: false | ||
| ) | ||
| let store = CMUXMobileShellStore( | ||
| runtime: runtime, | ||
| workspaces: PreviewMobileHost.workspaces, | ||
| identityProvider: TestIdentityProvider( | ||
| currentUserIDValue: "phone-user", | ||
| currentUserEmailValue: "user@example.com" | ||
| ), | ||
| feedbackStampProvider: { | ||
| MobileFeedbackStamp( | ||
| buildType: .dev, | ||
| appVersion: "0.65.0", | ||
| appBuild: "10", | ||
| bundleIdentifier: "dev.cmux.ios.test", | ||
| osVersion: "iOS test", | ||
| deviceModel: "test" | ||
| ) | ||
| } | ||
| ) | ||
|
|
||
| store.signIn() | ||
| let result = await store.connectPairingURLResult( | ||
| "cmux-ios://attach?v=2&e=user@example.com&av=0.64.0&ab=9&r=100.71.210.41:\(CmxMobileDefaults.defaultHostPort)" | ||
| ) | ||
|
|
||
| #expect(result == .needsUserApproval) | ||
| #expect(store.connectionState == .disconnected) | ||
| #expect(store.pairingVersionWarning?.contains("0.65.0 (10)") == true) | ||
| #expect(store.pairingVersionWarning?.contains("0.64.0 (9)") == true) | ||
| #expect(try await responses.sentRequests().isEmpty) | ||
|
|
||
| await store.acceptPairingVersionWarning() | ||
|
|
||
| #expect(store.pairingVersionWarning == nil) | ||
| #expect(store.connectionState == .connected) | ||
| #expect(store.selectedWorkspace?.id.rawValue == "qr-workspace") | ||
| #expect(try await responses.sentRequests().contains { $0.method == "workspace.list" }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for version warning rejection/cancellation.
This test validates the acceptance path (acceptPairingVersionWarning()), but per the review stack layer 7, the feature also "clears warning on pairing-cancel." Consider adding a sibling test that validates the cancel/dismiss flow: when the version warning is shown (.needsUserApproval) and the user cancels pairing (e.g., store.cancelPairing()), verify that pairingVersionWarning is cleared and the connection remains disconnected.
🤖 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 `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift` around lines
1437 - 1488, Add a sibling test to cover the cancel/dismiss flow: duplicate the
setup from minimalPairingCodeVersionMismatchWarnsAndContinuesAfterAcceptance(),
call store.cancelPairing() after verifying the initial result is
.needsUserApproval (from store.connectPairingURLResult), then assert that
store.pairingVersionWarning is nil, store.connectionState remains .disconnected,
and that no workspace RPCs were sent (use try await responses.sentRequests()).
Name the test something like
minimalPairingCodeVersionMismatchClearsWarningOnCancel and use the same
ScriptedTransportResponses, testRuntime, store.signIn(), and
connectPairingURLResult invocation so it mirrors the acceptance test.
| func percentEncodeQueryValue(_ value: String) -> String { | ||
| var allowed = CharacterSet.urlQueryAllowed | ||
| allowed.remove(charactersIn: "&=+") | ||
| return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider explicit handling when percent-encoding fails.
The fallback ?? value returns the original unencoded value if addingPercentEncoding fails. For email addresses with + (e.g., user+tag@example.com) or versions with special characters, this could inject reserved characters into the URL query, potentially breaking parsing.
While encoding failure is rare for typical email/version strings, explicit handling would be more defensive.
🛡️ Safer fallback options
Option 1: Return empty string and skip the parameter:
func percentEncodeQueryValue(_ value: String) -> String {
var allowed = CharacterSet.urlQueryAllowed
allowed.remove(charactersIn: "&=+")
- return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value
+ return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? ""
}Then filter empty results:
if let email = normalizedNonEmpty(ticket.macUserEmail) {
- items.append("e=\(percentEncodeQueryValue(email))")
+ let encoded = percentEncodeQueryValue(email)
+ if !encoded.isEmpty {
+ items.append("e=\(encoded)")
+ }
}Option 2: Use a safe placeholder:
- return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value
+ return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? "ENCODING_FAILED"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func percentEncodeQueryValue(_ value: String) -> String { | |
| var allowed = CharacterSet.urlQueryAllowed | |
| allowed.remove(charactersIn: "&=+") | |
| return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? value | |
| } | |
| func percentEncodeQueryValue(_ value: String) -> String { | |
| var allowed = CharacterSet.urlQueryAllowed | |
| allowed.remove(charactersIn: "&=+") | |
| return value.addingPercentEncoding(withAllowedCharacters: allowed) ?? "ENCODING_FAILED" | |
| } |
🤖 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 270 - 274, The percentEncodeQueryValue(_:) currently falls back to
returning the raw unencoded value on failure which can inject reserved
characters into query strings; change it to return a safe fallback (e.g., empty
string or a fixed placeholder like "__ENCODING_FAILED__") instead of the raw
value, and adjust callers that build the query (where percentEncodeQueryValue is
used) to filter out or skip parameters whose encoded value is empty or the
placeholder so malformed values are not inserted into the URL; update references
to percentEncodeQueryValue and the query-building logic to implement this
defensive behavior.
| init(_ ticket: CmxAttachTicket) { | ||
| v = ticket.version | ||
| w = Self.normalizedNonEmpty(ticket.workspaceID) | ||
| t = Self.normalizedNonEmpty(ticket.terminalID) | ||
| d = ticket.macDeviceID | ||
| u = Self.normalizedNonEmpty(ticket.macUserEmail) | ||
| av = Self.normalizedNonEmpty(ticket.macAppVersion) | ||
| ab = Self.normalizedNonEmpty(ticket.macAppBuild) | ||
| r = Self.compactedRoutes(ticket.routes) | ||
| } |
There was a problem hiding this comment.
Normalization inconsistency across metadata fields.
CompactAttachTicket.normalizedNonEmpty (line 46) checks !value.isEmpty without trimming whitespace, while CmxPairingQRCode.normalizedNonEmpty (CmxPairingQRCode.swift:265) and MobileHostBuildIdentity.normalized (MobileHostBuildIdentity.swift:14) trim whitespace before checking. This means:
- A whitespace-only
macUserEmaillike" "would be omitted from QR URLs (normalized to nil after trim) but included in compact payloads (kept as-is). - The same ticket could encode differently depending on which grammar is used.
Since these values come from controlled sources (authenticated email, Bundle info), the practical risk is low, but consistency would prevent subtle edge-case bugs.
🔧 Proposed fix to align normalization
Update normalizedNonEmpty to trim whitespace like the other normalizers:
private static func normalizedNonEmpty(_ value: String?) -> String? {
- guard let value, !value.isEmpty else {
- return nil
- }
- return value
+ let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines)
+ return trimmed?.isEmpty == false ? trimmed : nil
}Note: This also affects workspaceID and terminalID (lines 22-23), so verify that trimming is appropriate for those fields as well.
🤖 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/CompactAttachTicket.swift`
around lines 20 - 29, The CompactAttachTicket.normalizedNonEmpty function
currently checks !value.isEmpty without trimming, causing whitespace-only
strings to be kept; update normalizedNonEmpty to trim whitespace (e.g., call
.trimmingCharacters(in: .whitespacesAndNewlines)) and return nil if the trimmed
result is empty so its behavior matches CmxPairingQRCode.normalizedNonEmpty and
MobileHostBuildIdentity.normalized; ensure this change applies to all callers in
CompactAttachTicket.init (workspaceID, terminalID, macUserEmail, macAppVersion,
macAppBuild) and verify trimming is acceptable for those fields.
…l-version-warning
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c31354032e
ℹ️ 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".
| .sheet(isPresented: addDeviceSheetBinding) { | ||
| pairingSheet | ||
| } |
There was a problem hiding this comment.
With the sheet now attached to rootContent, it is no longer scoped to the authenticated disconnected branch. If Stack revalidation or sign-out flips isAuthenticated to false while Add device is presented, the false branch in onChange(of: isAuthenticated) only returns, so isShowingAddDeviceSheet stays true and the pairing UI remains over SignInView instead of being torn down as before; clear/dismiss the sheet on auth loss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cb16477fe
ℹ️ 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".
| guard let macCompatibilityVersion = ticket.macPairingCompatibilityVersion, | ||
| macCompatibilityVersion != CmxMobileDefaults.pairingCompatibilityVersion else { | ||
| return nil |
There was a problem hiding this comment.
When a legacy/full or compact payload attach URL lacks macPairingCompatibilityVersion, the decoded ticket carries nil and this guard returns no warning, so the phone dials immediately. That bypasses the new approval gate for exactly the pre-compatibility payloads still accepted by CmxAttachTicketInput (for example workspace-scoped/custom-route fallback URLs), while the v2 decoder maps a missing pc to unknown compatibility and warns; treat nil as unknown here instead of as compatible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 173742f517
ℹ️ 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".
| macDisplayName: macDisplayName ?? fallbackDisplayName, | ||
| macUserEmail: macUserEmail, | ||
| macUserID: macUserID, | ||
| macPairingCompatibilityVersion: macPairingCompatibilityVersion, |
There was a problem hiding this comment.
When Add device is used with a typed host, connectManualHost requests mobile.attach_ticket.create, decodes MobileManualAttachTicketCreateResponse, then calls response.ticket.constrainingRoutes(...) and immediately connects that ticket; this newly preserved macPairingCompatibilityVersion is never checked with versionWarning(for:). A Mac returning a compatibility level different from the iOS CmxMobileDefaults.pairingCompatibilityVersion therefore bypasses the explicit approval warning enforced for scanned/pasted URLs and can pair incompatible builds without user consent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 005cedc. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 454c29f11f
ℹ️ 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".
| guard isAuthenticated else { return false } | ||
| pendingAttachURL = nil | ||
| Task { | ||
| await store.connectPairingURL(rawURL) |
There was a problem hiding this comment.
When a queued non-attach pairing URL is consumed after authentication (for example an older cmux-ios://pair link saved while the user was signed out/restoring), this still calls the Boolean connectPairingURL wrapper. This commit now normalizes those legacy payloads to compatibility 0, so connectPairingURLResult can return .needsUserApproval; the wrapper collapses that to false and no code opens the add-device sheet, leaving store.pairingVersionWarning hidden and the pairing flow stalled. Use the result-returning API here and surface the sheet/approval UI as connectAttachURL does.
Useful? React with 👍 / 👎.
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve conflicts from the iOS pairing version-warning + email-match work (#6028) landing on main alongside this branch's pairing checklist: - MobileShellComposite: keep both the checklist state/helpers and main's pairingVersionWarning state + emailFailure/versionWarning helpers; thread the pairing phase into the checklist resolver so a pre-network failure never shows a false cleared gate. - MobilePairingFailureCategory: map the new .emailMismatch case to the trust gate; resolving(_:reachedMac:) gates "prior gates cleared" on actually reaching the Mac. - PairingView / CMUXMobileRootView: keep both the version-warning section and the network/auth/trust checklist; match the merged memberwise-init argument order. - Tests: cover emailMismatch and the pre-network authFailed path; drive the composite checklist tests (and the shared makeConnectedStore helper) through the new "Continue anyway" version-warning approval so the scripted connect proceeds. - .github/swift-file-length-budget.tsv: regenerated for the merged tree via scripts/swift_file_length_budget.py --write-budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…enable presence-online-preferred via live presence (#5792) Two post-rebase changes: 1. Manual pairing must supersede an in-flight auto-attach at the TOP of connectPairingURLResult, before validation, so even an invalid code parks the background attempt (the documented invariant the manualPairingSupersedes* / invalidManualHost* tests assert). Main's #6028 refactor split the post-validation beginPairingAttempt(method: "qr") out of the early path, so the supersede no longer rode on it. Call supersedeInFlightAutoAttach() explicitly; it is guarded on autoAttachInFlight, so it is a no-op when nothing is parked. 2. Presence #5792 has landed, so wire its live signal into auto-attach target selection (deferred when the PR was opened). The shell already owns a live PresenceMap from the device-tree subscription; derive online device ids from it (new PresenceMap.onlineDeviceIDs, same rollup rule as deviceSummary) when no explicit MobileAutoAttachPresence provider is injected and presence data exists. The external provider seam still wins if set, and an empty map (no presence yet) keeps the recency-only path unchanged, so existing behavior and tests are preserved. This stays behind the auto-attach flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Validation
swift testinPackages/CMUXMobileCoreswift testinPackages/CmuxMobileRPCswift testinPackages/CmuxMobileShellswift testinPackages/CmuxMobileWorkspacexcodebuild build -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination id=5E1A1E60-52E0-4ACE-B75B-67CFB27EECAB -derivedDataPath /tmp/cmux-emver-ios-buildxcodebuild test -workspace cmux.xcworkspace -scheme cmux-unit -only-testing:cmuxTests/MobileHostAuthorizationTests/testStackEmailAuthorizationRequiresMatchingEmail -only-testing:cmuxTests/MobileHostAuthorizationTests/testStackEmailAuthorizationRequiresSignedInMacUser -destination platform=macOS -derivedDataPath /tmp/cmux-emver-unit-testNotes
swift testinios/cmuxPackageis blocked by the package manifest defaulting library products to macOS 10.13 while dependencies require macOS 14.0; the normal iOS app Xcode build passed.MobileHostAuthorizationTestsclass run had one unrelated failure intestMobileHostConnectionCloseLeavesViewportReportsForPollingClient; the two changed email-authorization tests passed on targeted rerun.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes sit on the pairing and Stack authorization path (account matching and pre-connect gates), but behavior is covered by targeted mobile-core/RPC/shell tests and legacy QR decoding remains tolerant.
Overview
Pairing payloads on Mac and iOS now embed non-secret context: opaque Stack user id on v2 QR (
ub), pairing compatibility level, and app version/build, wired throughCmxAttachTicket, compact v1 JSON, and Mac ticket minting. Public QRs prefer user id over email; legacy compact payloads that still carry an email inukeep decoding.Before dialing, the iOS shell compares the ticket to the signed-in identity (user id when present, otherwise normalized email) and surfaces a new
emailMismatchfailure with localized copy. Compatibility skew no longer hard-fails: mismatched or missingpcyieldsneedsUserApproval, a stored warning, and Continue anyway in the pairing sheet and host picker; attach-ticket auth is not cleared while approval is pending.Mac host responses add
mac_app_version/mac_app_build, and Stack authorization is aligned on user id matching. Smaller follow-ons include extractingPhonePushPayload, pairing-flow connection-attempt guards, and test harness updates (includingMobileHostAuthorizationTests→ Swift Testing).Reviewed by Cursor Bugbot for commit 454c29f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Require the same account for iOS pairing (QR uses an opaque user id; legacy uses normalized email). Add a user‑approved compatibility warning shown in the host picker and pairing view, restore Mac↔iPhone notification dismiss/badge sync, keep existing connections intact, and record pairing validation analytics.
New Features
ub(user id),pc(compat),av(version),ab(build); compact tickets addu/pc/av/ab. iOS preflights with user id when present, else compares normalized emails; mismatches show a localized error. Validation analytics are recorded for mismatches and warnings.pcreturns.needsUserApproval. The warning is presented in the host picker and pairing sheet; users can accept to continue. Existing connections aren’t cleared, attach URLs wait for auth restore, the pairing sheet stays reachable, and the host picker dismisses after a retry even if it fails.Refactors
mobile.host.status; exposemac_app_version/mac_app_buildand a sharedpairingCompatibilityVersion. Legacy QR inputs without compatibility decode as0for warning-only checks..needsUserApprovalresult and guarded acceptance taps. ExtractPhonePushPayload/PhonePushPayloadKind. Move the no‑op delivered‑notification clearer to its own file and update tests/UI to surface the new warning.Written for commit 454c29f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation / Localization