Repository navigation
Compact mobile pairing QR ticket references - #7007
Closed
austinywang wants to merge 37 commits into
Closed
austinywang wants to merge 37 commits into
austinywang wants to merge 37 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesCompact QR ticket reference flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MobileCoreRPCClient
participant MobileCoreRPCTicketRedemptionGate
participant TerminalController
participant MobileHostService
MobileCoreRPCClient->>MobileCoreRPCTicketRedemptionGate: ticket(timeoutNanoseconds:provider:)
MobileCoreRPCTicketRedemptionGate->>TerminalController: mobile.attach_ticket.redeem
TerminalController->>MobileHostService: redeemAttachTicket(ticketRef:)
MobileHostService-->>TerminalController: attach-ticket payload
TerminalController-->>MobileCoreRPCClient: redeemed ticket response
MobileCoreRPCClient->>MobileCoreRPCClient: ticketState.replace(with:)
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…bile-pairing-qr-carry-a-ticket-refe
Contributor
Greptile SummaryThis PR switches mobile pairing QR codes and deep links from v2 (bare routes, implicit bearer token) to v3 (non-secret ticket reference + bare routes), and adds the
Confidence Score: 5/5Safe to merge. The concurrent redemption race from prior review is fully addressed by the new gate actor, and no regressions were found in auth, scope merge, or backwards compat. The concurrent redemption gate correctly deduplicates in-flight RPC calls and handles timeout/cancellation/abandon without leaking tasks. Scope merge in redeemedTicket keeps scanned QR scope authoritative. Backwards compatibility for v2 QR codes is preserved in isPairingCodeURL. Internationalization is complete across all 20 locales. No test seams were added to production source — ticketRedemptionGate is widened to internal so the test target can observe it via @testable import, exactly the pattern the codebase requires. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Mac as Mac (MobileHostService)
participant QR as v3 QR Code
participant iOS as iOS (MobileCoreRPCClient)
participant Gate as RedemptionGate (actor)
Mac->>Mac: "createTicket() -> ticketRef + authToken"
Mac->>QR: "encode(v=3&tr=ticketRef&r=host:port)"
iOS->>iOS: "CmxPairingQRCode.decode() -> ticket{ticketRef, routes, no authToken}"
Note over iOS,Gate: First authorized request triggers redemption
iOS->>Gate: ticket(timeoutNs, provider)
Gate->>Gate: "launch Task { provider() }"
Note over iOS,Gate: Concurrent authorized requests join same task
iOS->>Gate: ticket(timeoutNs, provider) waiter2
Gate->>Gate: "waiters += 1, await same Task"
Gate->>Mac: "mobile.attach_ticket.redeem {ticket_ref, stack_token}"
Mac->>Mac: "validAuthorization(ticketRef) -> ticket{authToken}"
Mac->>Gate: "{ticket: {ticketRef, authToken, macDeviceID}}"
Gate->>Gate: ticketState.replace(with: redeemed)
Gate->>iOS: redeemed ticket (waiter1 and waiter2)
iOS->>Mac: "workspace.list {attach_token: authToken, stack_token}"
Mac->>iOS: workspaces
iOS->>iOS: persistPairedMacFromTicket(redeemed)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Mac as Mac (MobileHostService)
participant QR as v3 QR Code
participant iOS as iOS (MobileCoreRPCClient)
participant Gate as RedemptionGate (actor)
Mac->>Mac: "createTicket() -> ticketRef + authToken"
Mac->>QR: "encode(v=3&tr=ticketRef&r=host:port)"
iOS->>iOS: "CmxPairingQRCode.decode() -> ticket{ticketRef, routes, no authToken}"
Note over iOS,Gate: First authorized request triggers redemption
iOS->>Gate: ticket(timeoutNs, provider)
Gate->>Gate: "launch Task { provider() }"
Note over iOS,Gate: Concurrent authorized requests join same task
iOS->>Gate: ticket(timeoutNs, provider) waiter2
Gate->>Gate: "waiters += 1, await same Task"
Gate->>Mac: "mobile.attach_ticket.redeem {ticket_ref, stack_token}"
Mac->>Mac: "validAuthorization(ticketRef) -> ticket{authToken}"
Mac->>Gate: "{ticket: {ticketRef, authToken, macDeviceID}}"
Gate->>Gate: ticketState.replace(with: redeemed)
Gate->>iOS: redeemed ticket (waiter1 and waiter2)
iOS->>Mac: "workspace.list {attach_token: authToken, stack_token}"
Mac->>iOS: workspaces
iOS->>iOS: persistPairedMacFromTicket(redeemed)
Reviews (26): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
added 4 commits
June 28, 2026 20:21
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/lib/attach-url.mjs (1)
89-112: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTighten the canonical-URL check to the non-secret v2 shape.
Sources/Mobile/MobileAttachTicketStore.swift:165-193still emits...://attach?v=1&payload=...when the minimal grammar cannot represent the ticket.isCanonicalAttachURL()will accept that too, so this branch can preserve a bearer-carrying deep link through the "canonical/non-secret" path. Parse the URL and require the v2 bare-route form (v=2,tr, nopayload) before reusing it.🤖 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 `@scripts/lib/attach-url.mjs` around lines 89 - 112, The canonical attach-URL check in isCanonicalAttachURL is too loose and currently accepts secret-bearing v1 payload links as well as the intended non-secret form. Update the attach-url handling in attach-url.mjs so the reuse branch only accepts the v2 bare-route shape by parsing the URL and requiring v=2, a tr parameter, and no payload before preserving payload.attach_url; keep the existing route-count guard and leave the fallback encoding path for non-canonical links.ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift (1)
1439-1461: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test still exercises the legacy
payload=attach URL, not the new QR grammar.The updated comment says this covers the current QR path, but Lines 1458-1461 still build
cmux-ios://attach?...&payload=.... That won't catch regressions inCmxPairingQRCode'str=encoding/decoding or the ticket-ref redemption flow. Please either build this fixture through the real QR encoder or rename it as a legacy attach-URL test.🤖 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 1439 - 1461, The fixture in this test is still using the legacy attach URL with payload= instead of the current QR grammar, so update it to exercise the real QR path or rename it to reflect legacy behavior. Use the relevant symbols CmxPairingQRCode and CmxAttachTicketCompactCoder to build the QR payload with tr= encoding/decoding and ensure the ticket-ref redemption flow is covered. If you keep the legacy URL shape, make the test name and comments explicit that it only validates the old attach-link format.cmuxTests/TerminalControllerSocketSecurityTests.swift (1)
621-626: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd redeem authz coverage
The capability-set assertion is fine, but there’s still no test showingmobile.attach_ticket.redeemrejects unauthenticated or cross-account callers. Add a focused authorization case alongside this coverage.🤖 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/TerminalControllerSocketSecurityTests.swift` around lines 621 - 626, Add a focused authorization test for mobile.attach_ticket.redeem in TerminalControllerSocketSecurityTests to cover unauthenticated and cross-account callers. Extend the existing capability/auth coverage by adding a case that invokes redeem through the same socket test harness and asserts it is rejected when no valid auth context is present or when the caller belongs to a different account. Use the existing TerminalControllerSocketSecurityTests helpers and the mobile.attach_ticket.redeem method name to keep the new check aligned with the current security coverage.
🤖 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/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift`:
- Around line 339-340: The redeem flow in MobileCoreRPCClient should fail closed
when the returned ticket does not match the requested ticketRef. In the
redemption path around MobileAttachTicketRedeemResponse.decode and
Self.redeemedTicket, validate the redeemed ticket against the original request
before updating ticketState, and do not fall back to the scanned reference
unless it is the only authoritative source. Make sure the logic in the fallback
location also preserves a single source of truth so a mismatched redeem response
cannot overwrite state for a different reference.
In
`@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift`:
- Around line 38-40: The detached completion path in
MobileCoreRPCTicketRedemptionGate’s redemption tracking is clearing the gate
state too early, which drops the timeout cooldown set by timeoutWaiter() before
timedOutResetNanoseconds elapses. Update the Task.detached completion observer
and the related cleanup in clear(id:) / timeoutWaiter() so task completion only
removes abandoned work, while the timedOutUntil sentinel remains in place until
the reset window naturally expires. Ensure the next authorization check still
sees the timed-out state after a cancelled redemption finishes.
In
`@Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swift`:
- Around line 162-168: The v2 pairing URL tests only cover the valid
ticket-reference path, so add a fail-closed assertion for decoding a v2 attach
URL without tr. Update the relevant decoder coverage in
CmxAttachTicketInputTests and the v2 pairing QR code tests around
CmxPairingQRCodeTests to verify that CmxAttachTicketInput/CmxPairingQRCode
rejects or returns nil for cmux-ios://attach?v=2&r=... when ticketRef is
missing, instead of treating it as valid input.
In
`@Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swift`:
- Around line 416-423: The test in MobileCoreRPCClientTests around secondTask is
relying on Task.sleep polling, which can pass without proving the second caller
actually joined the in-flight redemption. Replace the fixed-wait loop with an
explicit synchronization signal from the redemption path, similar to the earlier
queuedRequestIDs check, and only release redeemRelease once that signal confirms
the second authorized call has entered the shared redemption flow.
In
`@Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/ScriptedRPCTransport.swift`:
- Around line 66-70: The scripted transport is swallowing serialization and
frame-encoding failures in enqueueResponse(_:), which hides bad fixtures and can
leave tests hanging. Update the ScriptedRPCTransport send(_:) flow to surface
these failures immediately by making the response-enqueue path throw instead of
returning silently, and propagate the JSONSerialization/MobileSyncFrameCodec
errors back to the caller so the failing scripted response is reported at the
source.
In `@Sources/TerminalController.swift`:
- Around line 13541-13548: Remove the raw error string from the redeem RPC
failure response in TerminalController’s catch block: keep the localized
internal_error message in the .err result, but omit the data entry that
serializes the caught error. Update the redeem path so the API body does not
expose String(describing: error) or any other internal failure details.
---
Outside diff comments:
In `@cmuxTests/TerminalControllerSocketSecurityTests.swift`:
- Around line 621-626: Add a focused authorization test for
mobile.attach_ticket.redeem in TerminalControllerSocketSecurityTests to cover
unauthenticated and cross-account callers. Extend the existing capability/auth
coverage by adding a case that invokes redeem through the same socket test
harness and asserts it is rejected when no valid auth context is present or when
the caller belongs to a different account. Use the existing
TerminalControllerSocketSecurityTests helpers and the
mobile.attach_ticket.redeem method name to keep the new check aligned with the
current security coverage.
In `@ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift`:
- Around line 1439-1461: The fixture in this test is still using the legacy
attach URL with payload= instead of the current QR grammar, so update it to
exercise the real QR path or rename it to reflect legacy behavior. Use the
relevant symbols CmxPairingQRCode and CmxAttachTicketCompactCoder to build the
QR payload with tr= encoding/decoding and ensure the ticket-ref redemption flow
is covered. If you keep the legacy URL shape, make the test name and comments
explicit that it only validates the old attach-link format.
In `@scripts/lib/attach-url.mjs`:
- Around line 89-112: The canonical attach-URL check in isCanonicalAttachURL is
too loose and currently accepts secret-bearing v1 payload links as well as the
intended non-secret form. Update the attach-url handling in attach-url.mjs so
the reuse branch only accepts the v2 bare-route shape by parsing the URL and
requiring v=2, a tr parameter, and no payload before preserving
payload.attach_url; keep the existing route-count guard and leave the fallback
encoding path for non-canonical links.
🪄 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: 08e69469-6813-46c0-83af-04b58d7e06de
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (34)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileAttachTicketRedeemResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketState.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/ScriptedRPCTransport.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/ScriptedRPCTransportFactory.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/CmxAttachTicket+ConstrainingRoutes.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/MobileHost/ControlCommandCoordinator+MobileHost.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorMobileHostTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftResources/Localizable.xcstringsSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/TerminalController.swiftcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swiftscripts/lib/attach-url.mjsscripts/lib/attach-url.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift (1)
32-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOne stuck abandoned redeem can permanently wedge later retries.
After the first timed-out task is moved into
abandoned, a second timed-out redeem leavescurrenton a new id while the old cancelled task is still hanging. When that second cooldown expires, Line 37 rejects every later call until the first abandoned task finally finishes. If the transport ignores cancellation, pairing is bricked forever after two timeouts. The rollover path needs to cap or replace stale abandoned work instead of treating any old abandoned task as a permanent blocker.Also applies to: 80-87
🤖 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/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift` around lines 32 - 40, The rollover logic in MobileCoreRPCTicketRedemptionGate’s timeout handling currently treats any non-empty abandoned collection as a permanent blocker, which can wedge later retries after multiple timeouts. Update the timeout path that moves work into abandoned so stale abandoned tasks are capped, replaced, or pruned before adding the next timed-out request, and adjust the guard in the current/timedOutUntil flow so later calls can proceed once prior abandoned work is no longer relevant. Use the existing symbols current, abandoned, timedOutUntil, and isCompleted to locate and fix the retry gating behavior consistently in both affected sections.
🤖 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/MobileHostAuthorizationTests.swift`:
- Around line 401-419: The test uses the process-global MobileHostService.shared
and mutates its accepted Stack auth token before awaiting async work, so
concurrent authorization tests can interfere with each other. Update
testDebugConfiguredStackAuthTokenAuthorizesAttachTicketRedeem to avoid shared
mutable state by using a per-test isolated MobileHostService instance or by
serializing access around the override and restore. Make sure the
debugConfigureAcceptedStackAuthTokenForTesting reset is guaranteed within the
same isolated scope so other tests cannot observe the temporary token.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Line 145: The pairing QR parser in CmxPairingQRCode is reusing grammar version
2 for a new format that now requires tr, which breaks previously valid v=2 URLs
that only contain r. Update the versioned decoding logic so the tr-required
ticket-reference format uses a new grammar version (or make the v=2 path
backwards-compatible), and keep the existing v=2 decoder accepting the older
cmux-ios://attach?v=2&r=... shape. Make the change in the validation/parse flow
around the query checks and the version dispatch so the version-specific
compatibility path remains intact.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift`:
- Around line 32-40: The rollover logic in MobileCoreRPCTicketRedemptionGate’s
timeout handling currently treats any non-empty abandoned collection as a
permanent blocker, which can wedge later retries after multiple timeouts. Update
the timeout path that moves work into abandoned so stale abandoned tasks are
capped, replaced, or pruned before adding the next timed-out request, and adjust
the guard in the current/timedOutUntil flow so later calls can proceed once
prior abandoned work is no longer relevant. Use the existing symbols current,
abandoned, timedOutUntil, and isCompleted to locate and fix the retry gating
behavior consistently in both affected sections.
🪄 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: b3161482-d068-470f-b0a2-8ab075dc41dd
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (20)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingURLSchemeTests.swiftPackages/iOS/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/ScriptedRPCTransport.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobilePairingScannerPolicyTests.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swiftSources/TerminalController.swiftcmuxTests/MobileHostAuthorizationTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swiftscripts/lib/attach-url.mjsscripts/lib/attach-url.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/lib/attach-url.mjs (1)
90-92: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not reject canonical v3 URLs when Swift strips non-QR routes.
MobileAttachTicketStore.attachURL(for:)intentionally omits loopback/dev-only routes from the canonical v3 URL, but this guard still comparespayload.attach_urlagainst the rawpayload.ticket.routes.length. For a ticket with one Tailscale route plus one loopback route,isCanonicalAttachURL(..., 2)returns false andbuildAttachURLfalls back to the v1payload=URL, reintroducing the bearer token the new flow is meant to hide. It also leavesattach_urlandresult.routesfree to disagree about the effective route set. Validate against the QR-eligible subset (or parse the canonical URL back into the returned route list) instead of the unfiltered ticket routes.Also applies to: 126-129
🤖 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 `@scripts/lib/attach-url.mjs` around lines 90 - 92, The canonical v3 attach URL check in buildAttachURL is using the full payload.ticket.routes length, which causes valid Swift-generated URLs to fail when loopback/dev-only routes are stripped. Update the validation around isCanonicalAttachURL and the routes length comparison to use only the QR-eligible/effective route subset (or derive the expected routes from the canonical URL) so attach_url and result.routes can agree without falling back to the v1 payload URL.Source: Path instructions
Sources/Mobile/MobileAttachTicketStore.swift (1)
25-25: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not grow this lock-owned store further.
This change adds another mutable index plus new lookup/write paths under
NSLock. The repo’s Swift rules require shared runtime state like this to live behind actor isolation unless there is a documented low-level reason a lock is unavoidable. Please moveMobileAttachTicketStoreto an actor (or document that constraint here) before expanding the lock-based design. As per coding guidelines, "Do not addNSLock,pthread_mutex, or similar manual locking around shared mutable state when an actor orMainActor-isolated model would be safer; only allow very small lock usage around non-async low-level platform bridges when the code documents why an actor cannot be used."Also applies to: 27-68, 112-137
🤖 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/MobileAttachTicketStore.swift` at line 25, `MobileAttachTicketStore` is growing more shared mutable state under manual locking, so move its runtime state behind actor isolation instead of expanding the `NSLock`-based design. Refactor the store so the mutable dictionaries and lookup/write paths are owned by an actor (or, if that is truly impossible, add a clear documented justification for why `NSLock` is unavoidable). Update the affected methods and access patterns in `MobileAttachTicketStore` to use the actor’s isolated APIs, and remove the lock-based shared-state handling around `authTokensByTicketRef` and the other mutable indices.Source: Coding guidelines
Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swift (1)
47-151: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid wall-clock timing in these redemption tests.
These assertions depend on real 1ms/10ms timeout windows plus fixed
Task.sleepdelays, so they can fail under CI load even when the gate is correct. Inject a controllable clock intoMobileCoreRPCTicketRedemptionGateand advance it from the test instead of sleeping real time. As per coding guidelines, "Tests must not depend on real wall-clock time" and timeout behavior must use a virtual/fake clock.🤖 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/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swift` around lines 47 - 151, The redemption tests currently rely on real timeout windows and Task.sleep, which makes them flaky under load. Update MobileCoreRPCTicketRedemptionGate to accept a controllable clock and drive timeout/cooldown behavior through that clock instead of wall-clock time. Then rewrite timedOutTicketReferenceRedemptionKeepsCooldownAfterTaskCompletes and repeatedTimedOutTicketReferenceRedemptionsDoNotWedgeRetry to advance the fake clock and assert against the gate’s timeout behavior without sleeping.Source: Coding guidelines
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift (1)
102-145: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep canceled and superseded provider tasks tracked until they actually finish.
cancelWaiter()dropscurrentafter canceling its observer, andreplaceAbandoned(with:)does the same to older abandoned entries. If the provider ignores cancellation, those redeem tasks keep running untracked, so a later caller can start another redeem concurrently while the old one is still in flight. Keep them inabandonedand letcomplete(id:)retire them instead of canceling the observer early.🤖 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/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift` around lines 102 - 145, The waiter cancellation and abandoned-task cleanup logic in MobileCoreRPCTicketRedemptionGate is dropping in-flight provider work too early. Update cancelWaiter(id:), clear(id:), replaceAbandoned(with:), and complete(id:) so canceled or superseded redemption tasks remain tracked in abandoned until complete(id:) retires them, instead of canceling completionObserver and nil-ing current immediately. Use the existing current and abandoned bookkeeping to preserve task tracking even when the provider ignores cancellation.
🤖 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/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Around line 145-159: The v3 pairing URL check in
CmxPairingQRCode.isPairingCodeURL(_:) is too permissive because it accepts any
URL with a non-empty tr even when there are no r route items. Tighten the
validator to require at least one route for Self.version, matching the legacy
branch, so isPairingCodeURLString(_:) in MobilePairingModel only treats
attach_url values as QR-safe when they are actually decodable.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swift`:
- Around line 102-145: The waiter cancellation and abandoned-task cleanup logic
in MobileCoreRPCTicketRedemptionGate is dropping in-flight provider work too
early. Update cancelWaiter(id:), clear(id:), replaceAbandoned(with:), and
complete(id:) so canceled or superseded redemption tasks remain tracked in
abandoned until complete(id:) retires them, instead of canceling
completionObserver and nil-ing current immediately. Use the existing current and
abandoned bookkeeping to preserve task tracking even when the provider ignores
cancellation.
In
`@Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swift`:
- Around line 47-151: The redemption tests currently rely on real timeout
windows and Task.sleep, which makes them flaky under load. Update
MobileCoreRPCTicketRedemptionGate to accept a controllable clock and drive
timeout/cooldown behavior through that clock instead of wall-clock time. Then
rewrite timedOutTicketReferenceRedemptionKeepsCooldownAfterTaskCompletes and
repeatedTimedOutTicketReferenceRedemptionsDoNotWedgeRetry to advance the fake
clock and assert against the gate’s timeout behavior without sleeping.
In `@scripts/lib/attach-url.mjs`:
- Around line 90-92: The canonical v3 attach URL check in buildAttachURL is
using the full payload.ticket.routes length, which causes valid Swift-generated
URLs to fail when loopback/dev-only routes are stripped. Update the validation
around isCanonicalAttachURL and the routes length comparison to use only the
QR-eligible/effective route subset (or derive the expected routes from the
canonical URL) so attach_url and result.routes can agree without falling back to
the v1 payload URL.
In `@Sources/Mobile/MobileAttachTicketStore.swift`:
- Line 25: `MobileAttachTicketStore` is growing more shared mutable state under
manual locking, so move its runtime state behind actor isolation instead of
expanding the `NSLock`-based design. Refactor the store so the mutable
dictionaries and lookup/write paths are owned by an actor (or, if that is truly
impossible, add a clear documented justification for why `NSLock` is
unavoidable). Update the affected methods and access patterns in
`MobileAttachTicketStore` to use the actor’s isolated APIs, and remove the
lock-based shared-state handling around `authTokensByTicketRef` and the other
mutable indices.
🪄 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: fc51e0b9-71fe-4ea1-9d45-ee47ad9ed19e
📒 Files selected for processing (24)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRBitmapTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingURLSchemeTests.swiftPackages/iOS/CmuxMobileCamera/Tests/CmuxMobileCameraTests/QRCodeFrameSelectionTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCTicketRedemptionGate.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTicketRedemptionGateTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingFailureTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobilePairingScannerPolicyTests.swiftPackages/iOS/CmuxMobileWorkspace/Tests/CmuxMobileWorkspaceTests/MobileRootAuthGateTests.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/Pairing/MobilePairingModel.swiftcmuxTests/MobileHostAuthorizationTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swiftscripts/lib/attach-url.mjsscripts/lib/attach-url.test.mjs
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
Repeated timed-out redemptions could accumulate one retained task plus its completion observer per attempt: `abandon(_:)` cancelled prior abandoned work but never dropped it, so a non-cooperative provider that ignores cancellation and never resolves `task.result` would grow memory without bound on this long-lived RPC owner. Supersede prior abandoned work on each rollover so the retained set stays bounded to the most recent attempt, and cover it with a repeated-timeout assertion on the new `abandonedCount`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pairing-ticket-reference change adds the public `emptyTicketRef` case to this shared-package error enum; document the type and each case so the public API surface carries Swift-DocC comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
Older Macs stamp the compact pairing grammar as v == 1 while writing the opaque Stack user id (no @) into `u`. The current decoder maps every v == 1 payload's `u` to macUserEmail, so a phone on the new grammar reads that opaque id as an email and the account preflight rejects a still-valid pairing before dialing. This test pins the expected behavior (u decodes to macUserID) and fails without the accompanying fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
origin/main's compact encoder stamped v == 1 and wrote the opaque Stack user id into `u`, and its decoder disambiguated email vs id with an `@` probe. The new grammar-version routing dropped that probe and mapped every v == 1 `u` to macUserEmail, so a phone on the new build scanning an older Mac's still-valid QR set macUserEmail to an opaque id (macUserID nil) and MobileShellComposite.emailFailure rejected the pairing during account preflight before dialing. Restore the `@` heuristic for the legacy v == 1 grammar while keeping the explicit user-id interpretation for the current v == 2 grammar. Fixes the regression pinned by the preceding test commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 45 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2369-2832: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile keeps absorbing unrelated responsibility; consider splitting before it grows further.
This diff adds a cancellation-aware mac-switch-attempt state machine (
beginMacSwitchAttempt/switchToMac/restorePreviousMacIfNeeded/serialized paired-mac writes) and a large terminal-replay-barrier subsystem (beginTerminalReplayBarrierthroughcancelAllTerminalReplayTasks, plus hybrid-transport delivery decisions) into a file that is already far past 800 lines. Per the repo's own file/package-boundary rule, an already-oversized production file growing by hundreds of lines across independently-testable concerns (pairing/switch coordination, terminal replay protocol, persistence write serialization) should have that logic extracted behind a smaller, independently testable type or SwiftPM package boundary rather than accreting further into this one file.As per path instructions: "Report a failure when a new production Swift file exceeds 400 lines without a clear single responsibility, or exceeds 800 lines even when the responsibility is mostly coherent... Report a failure when an existing production Swift file already over 800 lines grows by more than 250 lines, unless the PR removes or moves a mixed responsibility behind a new package boundary."
Also applies to: 6691-7371
🤖 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/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 2369 - 2832, The file is absorbing multiple unrelated responsibilities and should be refactored to honor the repo’s file-boundary rule. Extract the mac-switch attempt/state-machine logic centered on switchToMac, promoteSecondaryToForeground, restorePreviousMacIfNeeded, and the serialized paired-mac write helpers into a separate type or package boundary, and do the same for the terminal replay/barrier flow if it remains mixed here. Keep MobileShellComposite focused on orchestration only, with these subsystems moved behind smaller, independently testable abstractions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2369-2832: The file is absorbing multiple unrelated
responsibilities and should be refactored to honor the repo’s file-boundary
rule. Extract the mac-switch attempt/state-machine logic centered on
switchToMac, promoteSecondaryToForeground, restorePreviousMacIfNeeded, and the
serialized paired-mac write helpers into a separate type or package boundary,
and do the same for the terminal replay/barrier flow if it remains mixed here.
Keep MobileShellComposite focused on orchestration only, with these subsystems
moved behind smaller, independently testable abstractions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06d94ff6-014e-4e50-8320-eca47954fbbf
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxAttachTicketCompactCoder.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CompactAttachTicket.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxAttachTicketCompactCoderTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftResources/Localizable.xcstringsSources/TerminalController.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
💤 Files with no reviewable changes (3)
- cmuxTests/TerminalControllerSocketSecurityTests.swift
- Sources/TerminalController.swift
- ios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Address the one-major-type-per-file and pure-helper conventions on the ticket-redemption path: - Move MobileCoreRPCTicketRedemptionGate's nested `Current`/`Abandoned` structs into MobileCoreRPCTicketRedemptionGate+Current.swift and +Abandoned.swift (unqualified references inside the actor still resolve). - Extract the `ManualNanosecondClock` test clock and the `AsyncReleaseGate` test gate into their own files so each test file declares one major type. - Convert MobileCoreRPCClient's two pure ticket-redemption helpers (`ticketReferenceRequiringRedemption`, `redeemedTicket`) from private static funcs into instance methods on the existing private extension. - Replace MobileAttachTicketRedeemResponse's static `decode(_:)` namespace method with an `init(decoding:)` initializer. No behavior change: the package builds and all 74 tests in 9 suites pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous commit moved `ticketReferenceRequiringRedemption` and `redeemedTicket` onto MobileCoreRPCClient's private extension, which pushed MobileCoreRPCClient.swift past its Swift file-length budget (519 > 513). Move both helpers into MobileCoreRPCClient+TicketRedemption.swift (matching the package's existing `+Feature.swift` convention). MobileCoreRPCClient.swift drops to 480 lines, back under budget with no budget refresh or added debt. No behavior change; the package builds and all 74 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
A redeem reply that omits its own workspace/terminal scope (empty or whitespace-only workspaceID/terminalID) previously passed straight through in redeemedTicket(_:ticketRef:constrainedTo:) and, because CmxAttachTicket.validate() does not reject an empty workspace/terminal, was stored via ticketState.replace without a scope check. A malformed or partial redeem response could thereby widen the effective ticket past the workspace/terminal the QR was scoped to. Treat empty/whitespace-only workspaceID/terminalID in the reply as gaps and fall back to the scanned scope, matching the existing mac* gap-fill pattern in the same merge. A reply that carries its own scope still takes precedence over the scan. Regression test MobileCoreRPCRedeemScopeTests covers both directions. Verified locally red/green on the CmuxMobileRPC package: with the fix reverted, emptyReplyScopeFallsBackToScannedScope fails (merged.workspaceID -> " ", terminalID -> " "); with the fix present both tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The prior scope merge only fell back to the scanned scope when the redeem reply's workspace/terminal was empty. A reply carrying a *different* non-empty workspaceID/terminalID still overwrote the scanned scope, so a partial or hostile redeem response could retarget the ticket to a workspace/terminal other than the QR the user actually scanned. The redeem path only asserts ticketRef consistency, not scope, and CmxAttachTicket.validate() does not constrain scope, so nothing else caught this. Make the scanned scope authoritative per field via scopeFieldPreferringScanned: a non-empty scanned value always wins, and a redeemed value only fills a field the scan left empty. This honors the function's `constrainedTo scanned` contract and matches the compact v=3 grammar, which always scans empty scope (CmxPairingQRCode.canEncode requires empty workspace/terminal), so v=3 still adopts the redeemed scope while legacy non-empty scanned scope can no longer be retargeted. Addresses cubic P1 on 19e634a. Regression coverage in MobileCoreRPCRedeemScopeTests now asserts three directions: empty reply -> scanned, mismatched non-empty reply -> scanned, and empty scanned (v=3) -> redeemed. Verified red/green locally: with the guard reverted, mismatchedReplyScopeFallsBackToScannedScope fails (merged.workspaceID -> "ws-other", terminalID -> "term-other") while the v=3 and empty-reply tests still pass; full CmuxMobileRPC suite green (77 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
`scopeFieldPreferringScanned` is a pure, stateless helper. The cmux Aziz package-design policy (static-as-namespace / pure-helper) flags a `private static func` attached to a type that is called as `Type.method` as a caseless namespace in disguise. Move it to a file-scope `private func` and drop the `Self.` qualifier at its two call sites. Behavior is unchanged; the full CmuxMobileRPC suite (77 tests) stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`waiterCount`/`abandonedCount` on MobileCoreRPCTicketRedemptionGate had no production callers — they existed only for tests, a test-observation seam in production source. Following the cmux precedent (PR #6452), remove the accessors and widen the backing state to `private(set)` (internal read, private write) so `@testable` tests read `current?.waiters` and `abandoned.count` directly. The sibling RPCStackTokenGate keeps its state private with no such accessors, so this also restores symmetry. Behavior is unchanged; the full CmuxMobileRPC suite (77 tests) stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Commit 3f1b92d moved `scopeFieldPreferringScanned` to a file-scope `private func` to satisfy the Aziz static-as-namespace policy, but that tripped the `package-conventions-lint` CI check (`free-function` rule: no top-level free functions — scope functionality to a type). The two rules only conflict on `static func` vs file-scope: the Aziz grep matches `static func`/`class func`; the convention lint's regex is anchored at column 0, so it only matches file-scope declarations. An instance method on the extension (indented, non-static) satisfies both. Keep it grouped with its sole caller `redeemedTicket`, and add a comment so it is not "cleaned up" back into either flagged form. Behavior is unchanged; the full CmuxMobileRPC suite (77 tests) stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression coverage for a concurrency bug in the ticket-reference redemption path. `ticketForRequest` shares one redemption task across concurrent authorized requests (via MobileCoreRPCTicketRedemptionGate), but the provider captures the *triggering* request's `deadline` and passes it into `redeemAttachTicket` → `session.send`. A short-deadline request that times out while the redeem is in flight therefore times out the shared `session.send`, so a concurrent longer-deadline waiter fails spuriously even though it had ample budget. This mirrors the existing `shortTokenTimeoutDoesNotCancelLongerTokenWaiter` coverage for the Stack-token gate, whose provider correctly takes no caller deadline. The test is red at this commit (the longer waiter throws `requestTimedOut`); the fix follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`ticketForRequest` deduplicates ticket-reference redemption across concurrent authorized requests through MobileCoreRPCTicketRedemptionGate, but the shared provider passed the triggering request's `deadline` into `redeemAttachTicket` → `session.send`. When the first waiter had little budget left, its deadline timed out the shared redeem and every waiter failed — even one that joined with a much longer timeout. Give the shared provider a gate-governed (unbounded) deadline instead. Each waiter's own budget is already enforced by the gate (`timeoutNanoseconds:` + cancellation once the last waiter leaves), and `session.send` is cancellation-aware, so an abandoned redemption is still torn down. This matches RPCStackTokenGate, whose shared provider likewise carries no caller deadline. Turns the regression test from the previous commit green; full CmuxMobileRPC suite (78 tests) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `shorterWaiterTimeoutDoesNotPoisonSharedTicketRedemption` regression test grows MobileCoreRPCClientTests.swift by ~106 lines (621 -> 727), which the file-length budget guard flags. Refresh the budget to accept this test-coverage growth (the file is a test suite; the added coverage proves a real concurrency fix). Regenerated with `scripts/swift_file_length_budget.py --write-budget`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
…bile-pairing-qr-carry-a-ticket-refe # Conflicts: # .github/swift-file-length-budget.tsv
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5540
Summary
Local validation
Note: ios/cmuxPackage swift test was attempted but local GhosttyKit.xcframework is absent in this checkout, so that suite is left to CI.
Summary by cubic
Switch mobile pairing to v3 QR/deep links that carry a non‑secret ticket reference (
tr), redeemed over Stack‑authenticated RPC before any authorized request. Keeps QRs small, lets the Mac own TTL/revocation, and hardens redemption under retries/timeouts. Fixes #5540.New Features
tr=<ticket-ref>with plainhost:port; compact JSON v2 addsq(no inline expiry).trbefore any authorized request, shares concurrent attempts behind a timeout gate, merges redeemed scope safely, exposescurrentTicket(), and adds localized redeem‑failure messages.mobile.attach_ticket.redeem, returns a canonicalattach_urlwithtr(preserves release/dev scheme);scripts/lib/attach-url.mjsprefers the canonical URL.Bug Fixes
emptyTicketRef), mapticket_expired; v3 QR decoder still blocks loopback.uas an opaque user id (no@) to avoid false account‑mismatch.CmxAttachTicketErrorcases; helper placement and file splits align with package conventions (no behavior change).Written for commit 90dc248. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes