Repository navigation
Remove Stack auth from mobile attach hot path - #6921
Closed
austinywang wants to merge 95 commits into
Closed
austinywang wants to merge 95 commits into
austinywang wants to merge 95 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:
📝 WalkthroughWalkthroughAttach-token state now persists through paired-Mac storage, reconnect, and RPC auth selection. SQLite migrations add the new columns, wrappers forward them, and tests assert the updated attach-token and Stack-auth behavior. A separate UI test and a zsh PATH test were also adjusted. ChangesDurable attach-token flow
RPC attach-token auth
UI focus regression guard
zsh PATH diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (20 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 |
added 3 commits
June 26, 2026 13:22
# Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Greptile SummaryThis PR removes Stack Auth from the mobile attach hot path by using non-expired attach-ticket credentials directly for ticket-covered RPCs, reserving Stack auth as a fallback for tokenless pairing, out-of-scope requests, and
Confidence Score: 5/5Safe to merge. The auth logic is carefully layered: structured error codes gate every retry decision, client and host scope checks are aligned, and the durable token path has an explicit fallback to Stack auth on any rejection. The client-host auth contract is internally consistent: every RPC code the host can produce (unauthorized, forbidden, invalid_attach_token, account_mismatch) is handled by a distinct, deliberate branch on the client with no free-form string matching. Keychain/SQLite split atomicity is handled by read-before-write with rollback of the keychain value on SQLite failure. Test coverage spans auth selection, scope checks, durable fallback, workspace mutation auth, and store attach-token persistence. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant iOS as iOS Client
participant RPC as MobileCoreRPCClient
participant Host as MobileHostService
participant Ticket as MobileAttachTicketStore
Note over iOS,Ticket: Happy path — durable attach token covers request
iOS->>RPC: sendRequest(workspace.list)
RPC->>RPC: requestNeedsStackAuthFallback? false
RPC->>Host: "attach_token=tok, workspace.list"
Host->>Ticket: validAuthorization(tok)
Ticket-->>Host: authorization
Host->>Host: ticketMatchesCurrentMacAccount → true
Host-->>iOS: OK
Note over iOS,Ticket: Fallback — token unknown/expired on host
iOS->>RPC: sendRequest(terminal.input)
RPC->>Host: "attach_token=stale, terminal.input"
Host->>Ticket: validAuthorization(stale)
Ticket-->>Host: nil
Host-->>RPC: rpcError(unauthorized)
RPC->>RPC: shouldRetryWithStackAuth → true
RPC->>Host: "stack_access_token=…, terminal.input"
Host-->>iOS: OK
Note over iOS,Ticket: Reconnect — stale durable ticket rejected
iOS->>RPC: connectManualHost(durableTicket)
Host-->>RPC: rpcError(unauthorized)
RPC->>iOS: shouldRetryDurableAttachTicket → true
iOS->>Host: manualHostTicket via Stack auth
iOS->>RPC: connectManualHost(freshTicket)
Host-->>iOS: OK
%%{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 iOS as iOS Client
participant RPC as MobileCoreRPCClient
participant Host as MobileHostService
participant Ticket as MobileAttachTicketStore
Note over iOS,Ticket: Happy path — durable attach token covers request
iOS->>RPC: sendRequest(workspace.list)
RPC->>RPC: requestNeedsStackAuthFallback? false
RPC->>Host: "attach_token=tok, workspace.list"
Host->>Ticket: validAuthorization(tok)
Ticket-->>Host: authorization
Host->>Host: ticketMatchesCurrentMacAccount → true
Host-->>iOS: OK
Note over iOS,Ticket: Fallback — token unknown/expired on host
iOS->>RPC: sendRequest(terminal.input)
RPC->>Host: "attach_token=stale, terminal.input"
Host->>Ticket: validAuthorization(stale)
Ticket-->>Host: nil
Host-->>RPC: rpcError(unauthorized)
RPC->>RPC: shouldRetryWithStackAuth → true
RPC->>Host: "stack_access_token=…, terminal.input"
Host-->>iOS: OK
Note over iOS,Ticket: Reconnect — stale durable ticket rejected
iOS->>RPC: connectManualHost(durableTicket)
Host-->>RPC: rpcError(unauthorized)
RPC->>iOS: shouldRetryDurableAttachTicket → true
iOS->>Host: manualHostTicket via Stack auth
iOS->>RPC: connectManualHost(freshTicket)
Host-->>iOS: OK
Reviews (37): Last reviewed commit: "Guard compact mobile glass helpers" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
# Conflicts: # .github/swift-file-length-budget.tsv
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoring.swift (1)
15-28: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument
nilas “preserve existing ticket” in the protocol contract.The overload at Lines 95-116 relies on
attachToken: nil/attachTokenExpiresAt: nilnot clearing existing durable ticket state. Put that invariant on the required parameters so future conformers do not erase reconnect credentials during route refreshes.Suggested contract clarification
- /// - attachToken: Local-only attach ticket secret for fast reconnect. - /// - attachTokenExpiresAt: Expiration time for `attachToken`. + /// - attachToken: Local-only attach ticket secret for fast reconnect. + /// `nil` preserves the existing local ticket when updating an existing row. + /// - attachTokenExpiresAt: Expiration time for `attachToken`. When + /// `attachToken` is `nil`, implementations must preserve the existing + /// expiration on updates.🤖 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/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoring.swift` around lines 15 - 28, Document the nil-handling contract on MobilePairedMacStoring.upsert so future conformers know that attachToken: nil and attachTokenExpiresAt: nil must preserve the existing durable reconnect ticket instead of clearing it. Update the protocol comment near upsert and keep the behavior consistent in the overload that refreshes routes, making the invariant explicit alongside the attachToken and attachTokenExpiresAt parameters.
🤖 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/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swift`:
- Around line 15-17: The MobilePairedMacStore actor has grown beyond the
800-line production Swift limit, so split out persistence responsibilities now
before adding more logic. Extract schema migration code and row mapping/SQL
helper methods from MobilePairedMacStore into separate types or files, keeping
the actor focused on orchestration while preserving the current
currentSchemaVersion behavior and public API.
In `@tests/test_claude_wrapper_user_binary_resolution.py`:
- Around line 222-235: The zsh branch in the PATH resolution test is no longer
enforcing the empty-component contract, so regressions that drop or move
`::...:` segments can slip through. Update the
`test_claude_wrapper_user_binary_resolution` assertions in the zsh-specific
branch to explicitly verify the empty PATH entries are still present in the
correct positions/count, using the existing `shell_name`, `entries`, and
`expected_path` checks. If zsh is intentionally allowed to weaken this behavior,
split or rename the test to match the new contract instead of keeping the
stronger test name.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoring.swift`:
- Around line 15-28: Document the nil-handling contract on
MobilePairedMacStoring.upsert so future conformers know that attachToken: nil
and attachTokenExpiresAt: nil must preserve the existing durable reconnect
ticket instead of clearing it. Update the protocol comment near upsert and keep
the behavior consistent in the overload that refreshes routes, making the
invariant explicit alongside the attachToken and attachTokenExpiresAt
parameters.
🪄 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: 29342897-3084-4e32-89c0-c837cbbac36a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
Packages/Shared/CmuxSyncStore/Tests/CmuxSyncStoreTests/CmuxSyncStoreTests.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMac.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoring.swiftPackages/iOS/CmuxMobilePairedMac/Tests/CmuxMobilePairedMacTests/MobilePairedMacStoreTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCTokenTimeoutTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/BackingUpPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/DeleteComputersVerifierPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/IOSBuildScopedPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/TeamScopedPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DelayedTeamPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/GatedUpsertStore.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swifttests/test_claude_wrapper_user_binary_resolution.py
added 6 commits
June 26, 2026 18:40
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swift`:
- Around line 16-20: The SQLite handle in MobilePairedMacStore should remain
hidden behind the actor-isolated API; restore the access restriction on the db
property so only the store’s own methods can touch the unsafe OpaquePointer.
Update the declaration in MobilePairedMacStore to make db private again, while
leaving the deinit cleanup and actor-isolated access pattern unchanged.
In
`@Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore`+Rows.swift:
- Around line 189-220: The SQLite row-loading loops in MobilePairedMacStore+Rows
are treating any non-`SQLITE_ROW` result from sqlite3_step as a clean ավարտ,
which can return partial data after an error. Update the row iteration logic in
the Mac and route fetch helpers to explicitly handle sqlite3_step failures (e.g.
SQLITE_BUSY, SQLITE_ERROR, and other non-ROW/non-DONE cases) by throwing instead
of falling through. Keep the existing row mapping code intact, but ensure the
functions that build the arrays surface the SQLite error rather than returning
incomplete results.
In
`@Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoreLog.swift`:
- Line 3: The file-scoped PairedMacStore logger is currently exposed and subject
to default actor isolation; update the `pairedMacStoreLog` declaration in
`MobilePairedMacStoreLog.swift` to be a file-private, nonisolated constant so it
stays out of the module API and matches the logging guideline. Keep the same
`Logger(subsystem:category:)` setup, but change the declaration to use
`nonisolated private let` for `pairedMacStoreLog`.
In
`@Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoreMacRow.swift`:
- Around line 3-17: Mark the MobilePairedMacStoreMacRow persistence model as
nonisolated so it does not inherit default actor isolation in Swift 6. Update
the struct declaration for MobilePairedMacStoreMacRow to explicitly opt out of
actor isolation since it is a pure value model with only stored properties and
no UI binding. Keep the model otherwise unchanged, and apply the annotation
directly on the struct definition.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Line 1004: The cancelled registry refresh path in MobileShellComposite still
allows an in-flight task to reach pairedMacStore.upsert after sign-out or team
change. Update the registry refresh flow around cancelRegistryRouteRefresh and
the related refresh/task methods at the other referenced call sites to either
keep task handles until they are drained or add a final current-generation/scope
check immediately before the store mutation so stale writes are blocked.
- Around line 1774-1782: The background registry refresh in MobileShellComposite
is incorrectly taking ownership of the active Mac state by calling
pairedMacStore.upsert with markActive set to true. Update this flow to
route-only behavior by using the store’s route-update path or by
preserving/re-reading the existing active flag inside the pairedMacStore
transaction, so the refresh updates routes without changing which Mac is active.
Keep the fix centered on the pairedMacStore.upsert call site and any related
store method that owns active-state persistence.
🪄 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: 013832bf-e681-4345-84d4-01257ad99817
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (13)
Packages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMac.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+Connection.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+Migrations.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+Rows.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+SQLite.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoreLog.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoreMacRow.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStoring.swiftPackages/iOS/CmuxMobilePairedMac/Tests/CmuxMobilePairedMacTests/MobilePairedMacCodingTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftcmuxTests/WorkspaceUnitTests.swifttests/test_claude_wrapper_user_binary_resolution.py
# Conflicts: # .github/swift-file-length-budget.tsv # Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatScrollEdgeCoordinator.swift # Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/ChatTranscriptTableView.swift # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
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 #6151
Summary:
Tests:
Local note: swift test --package-path ios/cmuxPackage could not run in this checkout because GhosttyKit.xcframework is missing its binary artifact.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Speeds up mobile launches and recovery by using scoped attach‑ticket auth on same‑account hot paths and keeping Stack Auth off the critical path; fixes #6151. Adds durable token persistence and strict scope/fallback rules with clear host-side errors.
New Features
mobile.events.subscribe, terminal input/mouse/group ops);workspace.listaccepts Mac‑wide or workspace tickets;workspace.createrequires Mac‑wide; require workspace scope forterminal.list; terminal‑scoped tickets cannot mutate workspaces. Only use the attach‑token path on same‑account pairs; skip Stack waits when covered; retry with Stack only for legacy unauthorized variants or expired/stale tokens (no fallback oninvalid_attach_token, accountless, or scope errors); surfaceattachTicketExpired. QR pairing carries no bearer token. Host rejects unknown tickets early withinvalid_attach_token. Secondary clients reuse durable tickets and redact auth errors.Refactors
MobileShellComposite+SecondaryClients; move and guard compact glass helpers viaCmuxMobileSupport/MobileGlassEffectContainerwith#if compiler(>=6.2); add@MainActorwhere needed; safer gesture teardown and notification‑status isolation; terminal output‑queue surface handle; instance hardware‑key resolver.Written for commit 19a14c6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests