Repository navigation
Isolate and label tagged iOS dev computers - #7864
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:
📝 WalkthroughWalkthroughChangesBuild-scoped identity and paired-Mac routing now use versioned iOS scopes and exact instance tags. Attach-ticket creation supports simulator, physical-device, and ticket-only targets with target-specific route filtering and payloads. Development scripts share tag validation and target propagation, while presence backup restores remain isolated by scope. Mobile identity and attach flow
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant MobileApp
participant MobileHostService
participant AttachTicketStore
participant PresenceService
participant PairedMacStore
MobileApp->>MobileHostService: create target-specific attach ticket
MobileHostService->>AttachTicketStore: filter routes and encode payload
AttachTicketStore-->>MobileApp: target-specific ticket
PresenceService->>MobileApp: tagged presence update
MobileApp->>PairedMacStore: persist routes in matching scope
Possibly related PRs
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 |
Greptile SummaryThis PR isolates tagged iOS dev builds from other Mac app instances. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (18): Last reviewed commit: "Reject authority-less backup host tuples" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .foregroundStyle(.secondary) | ||
| VStack(alignment: .leading, spacing: 2) { | ||
| Text(mac.displayName ?? mac.macDeviceID) | ||
| Text(scopedDisplayName(mac.resolvedName)) |
There was a problem hiding this comment.
When a paired Mac has a saved display name or falls back to its device id, this row now uses mac.resolvedName instead of the previous mac.displayName ?? mac.macDeviceID. The picker can show the raw host name while other paired-Mac surfaces still use the user-facing name, so users may pick or forget the wrong-looking Mac entry.
| Text(scopedDisplayName(mac.resolvedName)) | |
| Text(scopedDisplayName(mac.displayName ?? mac.macDeviceID)) |
Rule Used: Flag correctness-critical detection/identity deriv... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Rejected after checking MobilePairedMac.resolvedName: it returns customName first, then displayName, then macDeviceID. The suggested expression is the pre-change behavior and would drop the user custom name. Using resolvedName makes this picker consistent with the Computers surfaces while the build scope only appends the tag.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
Sources/TerminalController.swift (1)
14295-14313: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
routeUnavailablenow has two causes but the error data only reports one.
routeUnavailableis thrown both when a requestedroute_id/route_kindcan't be matched and (new in this PR) whentarget.selectRoutes(from:)finds no route matching the destination (e.g.physical_devicerequested on a Mac with only loopback routes and no explicit route filter). In the latter caseroute_id/route_kindare absent, sodatacomes back empty and the message "Requested mobile host route is not available" reads as if a specific route was asked for, obscuring that the real cause is the target having no eligible route. The siblinginvalidAttachURLcatch (Lines 14308-14313) already includestargetin its data for exactly this reason.🩹 Proposed fix to include target in routeUnavailable diagnostics
} catch MobileAttachTicketStoreError.routeUnavailable { var data: [String: Any] = [:] if let routeID { data["route_id"] = routeID } if let routeKind { data["route_kind"] = routeKind } + if rawTarget != nil { + data["target"] = target.rawValue + } return .err( code: "unavailable", message: "Requested mobile host route is not available", data: data.isEmpty ? nil : data )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController.swift` around lines 14295 - 14313, Update the MobileAttachTicketStoreError.routeUnavailable catch in the surrounding route-selection handler so its diagnostic data always includes target.rawValue, while retaining route_id and route_kind when present; adjust the message if needed to cover both unmatched requested routes and targets with no eligible routes, matching the diagnostic approach used by the invalidAttachURL catch.
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileIOSBuildScope.swift`:
- Around line 54-57: Introduce a shared static convenience method such as
MobileIOSBuildScope.appliedDisplayName(_:) that applies computerDisplayName(_:)
when a current scope exists and otherwise returns the original name. Replace the
repeated MobileIOSBuildScope.current()?.computerDisplayName(x) ?? x expressions
in MacComputerDetailView, MobileHostPickerView, WorkspaceListView+MacSelection,
and MacComputerSnapshot+Store with this helper.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 14295-14313: Update the
MobileAttachTicketStoreError.routeUnavailable catch in the surrounding
route-selection handler so its diagnostic data always includes target.rawValue,
while retaining route_id and route_kind when present; adjust the message if
needed to cover both unmatched requested routes and targets with no eligible
routes, matching the diagnostic approach used by the invalidAttachURL catch.
🪄 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: 3379ef71-b5a5-467d-8c55-9cef01128708
📒 Files selected for processing (28)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileIOSBuildScope.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ManualAttachTicket.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacAliases.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IOSBuildScopedPairedMacStoreTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PresenceMapTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerSnapshot+Store.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+MacSelection.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceMacSelectionTests.swiftSources/Cloud/DeviceRegistryClient.swiftSources/Cloud/MacPairedMacBackupPublisher.swiftSources/Cloud/PresenceHeartbeatClient.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/TerminalController.swiftcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobileHostIdentityTests.swiftios/scripts/reload.shscripts/dev-setup.shscripts/lib/attach-url.test.mjsscripts/lib/mobile-attach.shscripts/lib/mobile-attach.test.mjsscripts/mobile-attach-qr.shscripts/mobile-dev-launch.shscripts/reload.sh
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
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 (2)
Sources/Mobile/MobileHostService.swift (1)
1066-1092: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the empty-routes distinction
target.selectRoutes(from: [])now throws.routeUnavailablebeforeticketStore.createTicketruns, so the.noRoutescatch inSources/TerminalController+MobileAttachTicket.swiftno longer covers the common “host not listening yet” case. Add an explicit empty-routes guard here, or let.noRoutesbubble through for.ticketOnly, so the user sees the more accurate message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Mobile/MobileHostService.swift` around lines 1066 - 1092, Add an explicit empty-routes check in createAttachTicket before calling target.selectRoutes, preserving the noRoutes outcome when the host is not listening; for ticketOnly, either throw the existing .noRoutes error or otherwise ensure it bubbles to TerminalController+MobileAttachTicket.swift, while retaining target-specific routeUnavailable behavior where appropriate.Sources/Mobile/MobileAttachTicketStore.swift (1)
353-377: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCase-sensitive "default" sentinel check diverges from
MobileIOSBuildScope's case-insensitive check.
instanceDisplayNameonly treats an exact-lowercase"default"as the reserved sentinel (line 362), whileMobileIOSBuildScope.init?(Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileIOSBuildScope.swift:19) usestrimmed.lowercased() != "default". A build tagged"Default"would be scoped as untagged/stable on the iOS side but would still get suffixed as a real tag (" (Default)") on the Mac display-name side — a cross-platform naming/scoping mismatch for a tag that differs from the reserved sentinel only in case. This contradicts the PR's stated goal of rejecting the sentinel "through a shared validator."🐛 Proposed fix
let trimmedTag = buildTag?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - guard !trimmedTag.isEmpty, trimmedTag != "default" else { + guard !trimmedTag.isEmpty, trimmedTag.lowercased() != "default" else { return trimmedName }Do you want me to generate a shared validator (e.g. reuse
MobileIOSBuildScope's reserved-tag check) that both the iOS scoping path and this Mac-side naming path call into, so the sentinel definition can't drift again?🤖 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` around lines 353 - 377, The reserved “default” build-tag check in instanceDisplayName is case-sensitive and inconsistent with MobileIOSBuildScope. Treat trimmed tags case-insensitively by rejecting any value whose lowercased form is “default”, preferably through a shared validator used by both paths to prevent future drift.
🤖 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/MobileIOSBuildScope.swift`:
- Around line 29-32: Update the `MobileIOSBuildScope.current` method’s
`infoDictionary` default from an optional dictionary to
`Bundle.main.infoDictionary ?? [:]`, preserving the existing guard behavior
while avoiding the optional-collection lint warning.
---
Outside diff comments:
In `@Sources/Mobile/MobileAttachTicketStore.swift`:
- Around line 353-377: The reserved “default” build-tag check in
instanceDisplayName is case-sensitive and inconsistent with MobileIOSBuildScope.
Treat trimmed tags case-insensitively by rejecting any value whose lowercased
form is “default”, preferably through a shared validator used by both paths to
prevent future drift.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 1066-1092: Add an explicit empty-routes check in
createAttachTicket before calling target.selectRoutes, preserving the noRoutes
outcome when the host is not listening; for ticketOnly, either throw the
existing .noRoutes error or otherwise ensure it bubbles to
TerminalController+MobileAttachTicket.swift, while retaining target-specific
routeUnavailable behavior where appropriate.
🪄 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: c75bb4bb-9a61-432e-9db9-61a08175ace8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (29)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileIOSBuildScope.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileIOSBuildScope.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PresenceRouteSync.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceMap.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IOSBuildScopedPairedMacStoreTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePairedMacCoalescingTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TaggedBuildPresenceRouteIsolationTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TaggedBuildRegistryRouteIsolationTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileHostPickerView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+MacSelection.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MacComputerSnapshotBuildScopeTests.swiftSources/Cloud/MacPairedMacBackupPublisher.swiftSources/Mobile/MobileAttachTarget.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+MobileAttachTicket.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MacPairedMacBackupPublisherScopeTests.swiftcmuxTests/MobileHostWorkspaceTicketAuthorizationTests.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftscripts/mobile-stability-soak/mobile-soak.pytests/test_mobile_stability_soak_attach_contract.pyworkers/presence/src/do.tsworkers/presence/src/syncPairedMacs.tsworkers/presence/test/syncPairedMacs.test.tsworkers/presence/test/taggedRouteIsolation.test.ts
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileIOSBuildScope.swift
| public static func current( | ||
| infoDictionary: [String: Any]? = Bundle.main.infoDictionary, | ||
| bundleIdentifier: String? = Bundle.main.bundleIdentifier | ||
| ) -> MobileIOSBuildScope? { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Optional-collection lint on infoDictionary default.
SwiftLint's discouraged_optional_collection flags infoDictionary: [String: Any]? = Bundle.main.infoDictionary. Since the guard logic (infoDictionary?["CMUXDevTag"] as? String) treats nil and empty identically, defaulting to Bundle.main.infoDictionary ?? [:] would satisfy the lint without changing behavior.
🧹 Proposed fix
- infoDictionary: [String: Any]? = Bundle.main.infoDictionary,
+ infoDictionary: [String: Any] = Bundle.main.infoDictionary ?? [:],📝 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.
| public static func current( | |
| infoDictionary: [String: Any]? = Bundle.main.infoDictionary, | |
| bundleIdentifier: String? = Bundle.main.bundleIdentifier | |
| ) -> MobileIOSBuildScope? { | |
| public static func current( | |
| infoDictionary: [String: Any] = Bundle.main.infoDictionary ?? [:], | |
| bundleIdentifier: String? = Bundle.main.bundleIdentifier | |
| ) -> MobileIOSBuildScope? { |
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 30-30: Prefer empty collection over optional collection
(discouraged_optional_collection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileIOSBuildScope.swift`
around lines 29 - 32, Update the `MobileIOSBuildScope.current` method’s
`infoDictionary` default from an optional dictionary to
`Bundle.main.infoDictionary ?? [:]`, preserving the existing guard behavior
while avoiding the optional-collection lint warning.
Source: Linters/SAST tools
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53d5a9b. Configure here.
Lands main's dev-instance pairing isolation so the phone can tell tagged dev builds apart from production and each other (instance tags carried through pairing, routes, authority checks, and computer labels) — the root cause behind the sim silently rendering another instance's workspaces, including production's. This branch's v1 agent-chat deletion stays authoritative: main's interim chat fixes and the artifact-viewing UI built on the deleted chat package (#7862-era artifact chips, gallery, sheets, and their tests) are removed rather than restored; artifact viewing is a recorded parity item to reintegrate as transcript activity-rail content in the new GUI. ProcessSnapshotCentralizationTests keeps its compatibility test with the chat-registry-dependent test and helper actors stripped. AgentProcessObservationSource adapts to main's actor-backed process snapshot store, preserving exact-basename detection semantics. Composer hosting keeps this branch's judged implementation (nothing to port from main's in-file variant). Full localization-catalog union; budget, package-group, resolved-policy, and test-wiring gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Root cause
Tagged macOS builds reused the stable physical Mac device ID for both device-level and tagged-instance presentation. Tagged iOS builds then consumed shared and restored Mac records without applying their own build scope, so switching apps could show a computer left by another dev build.
Fix
This is a principled identity split: stable hardware identity remains stable, while build-instance presentation and routing are scoped explicitly.
Verification
Local Xcode test actions were not run because project policy prohibits them on the user Mac.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Changes pairing persistence, reconnect routing, backup restore, and cross-instance authority—core mobile connectivity paths where mistakes misroute users or resurrect stale hosts.
Overview
Tagged iOS dev builds and multiple Mac app instances on one machine no longer share or overwrite each other’s saved routes, reconnect paths, or backup rows.
Paired-Mac storage gains an
instanceTag(SQLite schema v5) plus atomicupsertIfNewerandupsertRoutesIfAuthorizedso stale restores and cross-tag writes are rejected. Host status decodesmac_instance_tag; registry refresh, presence push, reconnect, and backup uploads all gate route mutations on matching or unclaimed instance authority.MobileIOSBuildScopemoves to CMUXMobileCore with versioned backup scope (ios:v2:…), rejection of thedefaultsentinel, andcomputerDisplayNamesuffixing for the Computers UI. The build-scoped paired-Mac decorator and backup layer threadinstanceTagthrough upserts and conditional restores.Shell logic is split into focused extensions: tag authority resolution, ticket persistence with conditional unclaimed writes, presence-driven route sync, registry refresh with instance checks, and secondary-client promotion that re-validates stored tags.
CI adds
swift testfor CmuxMobilePairedMac; new regression suites cover instance-tag migration, route authority, and restore races.Reviewed by Cursor Bugbot for commit 06348d7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Tagged iOS dev builds now isolate Mac app instances by tag across routes, reconnects, presence, and backups. Dev builds show computer titles with a “(tag)” suffix; stable builds stay unscoped.
New Features
ios:v2:<tag>viaX-Cmux-Client-Scope; the Mac publisher sendsinstanceTagwith compare‑and‑set. Authenticated host status includesmac_instance_tag; the iOS store adds schema v5 withinstanceTagplus atomicupsertIfNewerandupsertRoutesIfAuthorized.CMUXMobileCoreMobileIOSBuildScope; presence shows liveness per exact instance. Attach tickets addtarget(ticket_only,simulator_injection,physical_device); scripts/tests use a shared dev‑tag validator. Attach‑ticket creation moved toTerminalController+MobileAttachTicketand keeps legacyattach_urlcompatibility.Bug Fixes
instanceTagand useupsertIfNewerso older backups can’t overwrite newer, tagged authority; worker reads stop falling back fromios:v2to unscoped data.Packages/iOS/CmuxMobilePairedMactests to gate tag/restore logic.Written for commit 06348d7. Summary will update on new commits.
Summary by CodeRabbit