Repository navigation
Add privacy-safe mobile dial diagnostics - #8264
x90skysn3k wants to merge 6 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@x90skysn3k is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughChangesThe PR adds Iroh as the primary mobile transport with Tailscale/LAN fallbacks. It introduces a Rust/C FFI and xcframework pipeline, shared Swift transports, Mac host lifecycle integration, iOS route selection and endpoint trust pinning, pairing QR v3 support, transport diagnostics, persistence, UI updates, tests, and CI provisioning. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (11 errors, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
2149-2159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShort-circuit the already-connected Iroh route.
This fast path only runs when
activeRouteis.hostPort. With Iroh as the default, selecting the current device/instance unnecessarily enters the destructive reconnect loop. Compare peer routes againstactiveRoutetoo, ideally through one route-equivalence helper.🤖 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 2149 - 2159, Update the existing fast path around the connection-state check to also recognize an already-active Iroh/peer route, not only .hostPort endpoints. Add or reuse a route-equivalence helper to compare each candidate route with activeRoute, preserving the current host normalization behavior, and return before the destructive reconnect loop when the selected device and instance are already connected.Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift (1)
238-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared tailscale-route-parsing and ticket-construction logic.
decodeLegacyTailscaleOnlyanddecodeCurrentduplicate the sameparseHostPort→ loopback-check →CmxAttachRoutesynthesis loop, and an almost identicalCmxAttachTicketconstruction block that differs only by theroutesarray. Any future grammar/ticket-field change now needs to be kept in sync across both functions.♻️ Proposed extraction
private extension CmxPairingQRCode { + func synthesizedTailscaleRoutes(from rawRoutes: [String]) throws -> [CmxAttachRoute] { + try rawRoutes.enumerated().map { index, rawRoute -> CmxAttachRoute in + let (host, port) = try parseHostPort(rawRoute) + guard !CmxLoopbackHost().matches(host) else { + throw MobileSyncPairingPayloadError.loopbackRouteRejected + } + return try CmxAttachRoute( + id: synthesizedRouteID(index: index), + kind: .tailscale, + endpoint: .hostPort(host: host, port: port), + priority: synthesizedRoutePriority(index: index) + ) + } + } + + func makeDecodedTicket(routes: [CmxAttachRoute], components: URLComponents) throws -> CmxAttachTicket { + let ticket = try CmxAttachTicket( + workspaceID: "", + terminalID: nil, + macDeviceID: "", + macDisplayName: nil, + macUserEmail: queryValue(named: "e", in: components), + macUserID: queryValue(named: "ub", in: components), + macPairingCompatibilityVersion: queryInt(named: "pc", in: components) ?? 0, + macAppVersion: queryValue(named: "av", in: components), + macAppBuild: queryValue(named: "ab", in: components), + routes: routes, + expiresAt: nil, + authToken: nil + ) + try ticket.validate() + return ticket + } + func decodeLegacyTailscaleOnly(_ components: URLComponents) throws -> CmxAttachTicket { let rawRoutes = (components.queryItems ?? []) .filter { $0.name == "r" } .compactMap(\.value) guard !rawRoutes.isEmpty, rawRoutes.count <= Self.maximumRouteCount else { throw MobileSyncPairingPayloadError.invalidURL } - let routes = try rawRoutes.enumerated().map { ... } - let ticket = try CmxAttachTicket(...) - try ticket.validate() - return ticket + return try makeDecodedTicket(routes: try synthesizedTailscaleRoutes(from: rawRoutes), components: components) }Similarly,
decodeCurrentwould build the iroh route, callsynthesizedTailscaleRoutes(from:)for ther=items, concatenate, and callmakeDecodedTicket(routes:components:).🤖 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/CmxPairingQRCode.swift` around lines 238 - 336, Extract the duplicated tailscale route mapping from decodeLegacyTailscaleOnly and decodeCurrent into a shared synthesizedTailscaleRoutes(from:) helper that preserves host/port parsing, loopback rejection, route IDs, kinds, endpoints, and priorities. Extract the shared CmxAttachTicket construction and validation into makeDecodedTicket(routes:components:), then have both decoders call it with their respective route arrays while preserving the existing iroh route handling in decodeCurrent.
🤖 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 @.github/workflows/ios-testflight.yml:
- Around line 369-374: Split the “Provision cmux-iroh FFI” workflow step into
separate steps for running install-rust-ci.sh and ensure-cmux-iroh.sh. Remove
the manual PATH export, relying on install-rust-ci.sh to configure GITHUB_PATH
for subsequent steps, and retain the existing execution order.
In @.github/workflows/perf-activation.yml:
- Around line 174-177: Add a Rust build cache step before the “Provision
cmux-iroh FFI” step, caching ~/.cargo/registry, ~/.cargo/git, and
native/cmux-iroh/target with a key that varies by runner and relevant
dependency/build configuration. Apply the same cache setup to other workflows
that invoke ensure-cmux-iroh.sh.
In `@docs/ios-swift-mobile-plan.md`:
- Around line 43-44: Renumber the milestones in the documented rollout plan
sequentially: keep the auth/reconnect milestone as 6, then place the Iroh
default and transport diagnostics milestone after it as milestone 7.
In `@ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift`:
- Around line 349-389: Change the validation flow around pairedMacStore.loadAll
and the currentUserID() lookup so it requires non-empty authenticated user and
team scope, without falling back to ticket.macUserID. Require an existing paired
record with a non-empty authoritative irohEndpointID and explicit trust
validation, rejecting missing or mismatched IDs and unavailable pinned routes.
Remove the upsert paths that create or populate trust from ticket.routes; handle
enrollment/re-trust only through a separate explicit user-approved flow.
In `@ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift`:
- Line 27: Update the initializer and default handling for
irohEndpointTrustValidator so omitted injection uses a throwing fail-closed
validator instead of a no-op. Preserve successful validation for explicitly
supplied validators, and move any no-op behavior behind an explicitly named
insecure test/helper path.
In `@ios/scripts/cloud-testflight.sh`:
- Line 235: Update the ensure-cmux-iroh.sh invocation in the archive preparation
flow to stop suppressing failures: remove the unconditional `|| true` while
preserving the existing executable check and repository-root working directory.
A failure from ensure-cmux-iroh.sh must immediately fail the production archive
rather than allowing a stale XCFramework to be packaged.
In `@native/cmux-iroh/src/lib.rs`:
- Around line 63-71: Update runtime() and its exported extern "C" callers to
make Tokio runtime creation fallible instead of using expect. Propagate
initialization failures as CmuxIrohError with ErrorKind::Internal before
crossing the FFI boundary, while preserving the existing successful runtime
reuse behavior.
In `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swift`:
- Around line 209-247: Move the asynchronous .attempt observer call and
makeTransport() setup from the actor-isolated connection path into the detached
task created by the connection method. Assign connectionTask synchronously
immediately after acquiring the connect lease, ensuring concurrent callers join
the same task; preserve failure reporting, lease cleanup, cancellation handling,
and connectionID setup using the captured state.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ReconnectRoutes.swift:
- Around line 135-139: Update freshReconnectRoutesAfterLocalFailure to return
[CmxAttachRoute] instead of an optional, returning [] when refreshed routes are
empty. Adjust every caller to check isEmpty rather than optional binding,
preserving the existing behavior for non-empty fresh routes.
- Around line 282-291: Unify peer-route identity between reconnectRouteKey and
mergedReconnectRoutes’ local endpointKey by extracting one shared key-building
function and reusing it in both paths. Ensure the shared key applies the same
field set for .peer routes, including or excluding relayHint consistently, so
deduplication and freshReconnectRoutesAfterLocalFailure comparisons cannot
disagree.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swift`:
- Around line 41-45: Update the activeTransportKind argument in
WorkspaceDetailContainer’s WorkspaceDetailView call to use the transport kind
from workspace’s per-Mac connection snapshot, rather than store.activeRoute.
Keep the workspace-specific connection status and ensure the displayed pill and
toolbar subtitle reflect the Mac that owns workspace.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+DerivedState.swift:
- Around line 11-18: Update selectedToolbarSubtitle to derive the terminal name
from the existing selectedTerminal property instead of resolving
store.selectedTerminalID and searching workspace.terminals directly. Preserve
the current connection-status and activeTransportKind behavior while ensuring
nil or stale selections use the same visible first-terminal fallback.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swift`:
- Around line 148-181: Add the missing localization catalog entries in
Localizable.xcstrings for every key used by irohTransportRow and
publishTailscaleRoutesRow, including both labels and on/off subtitles. Provide
entries for all supported locales using the catalog’s existing format and
translation conventions.
In
`@Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransport.swift`:
- Around line 136-144: Move the boundEndpointBeforeDeadline() call into the
existing error-mapping do block so any CmxIrohFailure is converted through the
bindFailed helper into CmxIrohByteTransportError.endpointBindFailed. Preserve
the elapsed-time check and timeout behavior, while ensuring callers do not
receive the raw FFI failure.
- Around line 125-135: Update CmxIrohByteTransport.connect() to track an
in-flight connection using a .connecting state or stored task, preventing
concurrent connect() and close() operations from producing duplicate or stale
.ready state. Ensure cancellation and close transition through the same
lifecycle path, and update receive() cancellation handling to set state to
.closed when releasing the FFI handle. Guarantee the handle is released exactly
once.
In
`@Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohFFIClient.swift`:
- Around line 6-16: Update CmxIrohEndpointReference and the corresponding
connection wrapper to add an idempotent deinitialization fallback that releases
the native handle when Swift ownership ends, while preserving explicit close()
behavior and synchronization with active uses. Ensure destruction cannot release
either handle more than once.
In
`@Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift`:
- Around line 48-55: Replace String-based associated values in
CmxNetworkByteTransport’s connectionFailed, receiveFailed, and sendFailed cases
with typed, localizable failure values. In CmxNetworkByteTransport.swift lines
693-709, map Network.framework failures to those typed cases without hard-coded
English or raw descriptions. In CmxNetworkRoutePinger.swift lines 54-56 and
90-92, map unexpected TCP and Iroh errors to safe typed failures while retaining
upstream details only in sanitized diagnostics.
- Around line 565-572: Update cancelSend to leave the matching sendContinuation
occupied until the underlying NWConnection.send completion callback finishes;
mark the operation canceled and resume its waiting continuation, but do not
clear the slot there. Adjust the completion handling to recognize the canceled
operation, clear the slot only after the callback, and preserve any connection
error rather than discarding it when the operation ID matches the canceled send.
In
`@Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift`:
- Around line 143-149: Update the xcframeworkURL construction in
CmxIrohByteTransportTests to target the package-local
CmuxIrohFFIBinary.xcframework path used by Package.swift, rather than the
repository-root CmuxIrohFFI.xcframework location. Keep the existing
file-existence guard and native loopback test flow unchanged once it resolves
the packaged framework correctly.
In
`@Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift`:
- Around line 68-73: Replace the scheduler-dependent synchronization in the
concurrent transport tests with explicit event signals: in
Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift:68-73,
await a receive-start signal before calling transport.close; in
Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift:220-233,
await the send-completion event directly and remove the race against the 250 ms
sleep. Keep each test’s existing transport behavior and assertions unchanged.
In `@reports.md`:
- Around line 68-70: Add a blank line after the TailscaleStatusTests.swift test
heading staleEvaluationCannotOverwriteFresherRefresh in reports.md, separating
the heading from the following list.
In `@Resources/Localizable.xcstrings`:
- Around line 123525-123537: Update the localized value for the
mobile.pairing.req.route.missing key to mention LAN alongside Iroh and
Tailscale, using wording such as “No Iroh, Tailscale, or LAN route is
available.” Apply the equivalent meaning consistently to the affected
localization.
In `@scripts/ensure-cmux-iroh.sh`:
- Around line 115-123: Replace the unbounded mkdir/sleep polling loop around
LOCK_DIR with an OS-backed advisory lock or race-safe independent temporary
build strategy followed by atomic publication. Ensure stale locks cannot block
future builds indefinitely, while preserving the cached XCFramework fast path
through link_local_xcframework and the existing successful-build behavior.
- Around line 50-62: Update hash_sources in ensure-cmux-iroh.sh to include the
build environment in the BUILD_KEY: hash Rust and Xcode toolchain versions,
configured target list, SDK versions, and effective IPHONEOS_DEPLOYMENT_TARGET
and MACOSX_DEPLOYMENT_TARGET values before hashing the source files. Reuse the
script’s existing environment and toolchain variables where available, and
preserve the current source-content hashing behavior.
In `@Sources/Mobile/MobileHostIrohSecretKeyStore.swift`:
- Around line 21-29: Make secret-key creation atomic in secretKey() by using the
store-if-absent operation and returning the persisted winner rather than
allowing concurrent generated keys to replace each other. Update the
duplicate-item handling in the persistence helper at
Sources/Mobile/MobileHostIrohSecretKeyStore.swift lines 67-87 to load and return
the established key on errSecDuplicateItem; keep replacement available only
through an explicit rotation path. Both affected sites are in the same file:
lines 21-29 require the atomic creation flow, and lines 67-87 require duplicate
handling to preserve the single persisted identity.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 1854-1861: Remove the DEBUG-only
debugWaitForIrohStartupForTesting() accessor from MobileHostService in
production Sources. Update the tests to use an internal lifecycle signal
accessible via `@testable` import or observe the existing status transition
instead, while preserving the startup-wait behavior.
- Around line 1009-1024: Use one authoritative, normalized route source for Iroh
availability: in finishIrohStartup, require a valid route before setting the
lane active or starting the accept loop; if absent, close the endpoint and fail
closed. In Sources/Mobile/MobileHostService.swift lines 1103-1112, clear and
withdraw the cached route when the current FFI lookup fails instead of returning
stale route data.
- Around line 1060-1090: Update the iroh accept loop in the service owner around
irohAcceptTask and acceptByteConnectionOffMain to use a cancellable,
close-unblocked accept operation instead of the 500 ms timeout polling. Route
endpoint closure and persistent accept failures through a generation-checked
MobileHostService transition that withdraws the active route and marks or
restarts the lane, ensuring only the owner performs lifecycle state changes and
preventing immediate error-spin log flooding.
In `@web/messages/en.json`:
- Around line 4-15: Add the iOS translation keys metaDescription, subtitle, and
byoNetwork to every locale file under web/messages, preserving each file’s
existing locale structure and providing translated values where applicable.
Ensure all locales define these keys so they do not fall back to English.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2149-2159: Update the existing fast path around the
connection-state check to also recognize an already-active Iroh/peer route, not
only .hostPort endpoints. Add or reuse a route-equivalence helper to compare
each candidate route with activeRoute, preserving the current host normalization
behavior, and return before the destructive reconnect loop when the selected
device and instance are already connected.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swift`:
- Around line 238-336: Extract the duplicated tailscale route mapping from
decodeLegacyTailscaleOnly and decodeCurrent into a shared
synthesizedTailscaleRoutes(from:) helper that preserves host/port parsing,
loopback rejection, route IDs, kinds, endpoints, and priorities. Extract the
shared CmxAttachTicket construction and validation into
makeDecodedTicket(routes:components:), then have both decoders call it with
their respective route arrays while preserving the existing iroh route handling
in decodeCurrent.
🪄 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: 479d205c-ae76-4410-a9dc-2fcf09bfcad9
⛔ Files ignored due to path filters (4)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsvcmux.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolvednative/cmux-iroh/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (142)
.github/workflows/ci-macos-compat.yml.github/workflows/ci.yml.github/workflows/ios-app-store.yml.github/workflows/ios-testflight.yml.github/workflows/nightly.yml.github/workflows/perf-activation.yml.github/workflows/release.yml.github/workflows/reload-build.yml.github/workflows/test-depot.yml.github/workflows/test-e2e.yml.github/workflows/test-ios.yml.github/workflows/tmux-corpus.yml.gitignorePackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/Shared/CmuxMobileTransport/CmuxIrohFFIBinary.xcframeworkPackages/Shared/CmuxMobileTransport/Package.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxIrohC/CmuxIrohC.cPackages/Shared/CmuxMobileTransport/Sources/CmuxIrohC/include/CmuxIrohC.hPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransport.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransportError.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohEndpointManager.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohFFIClient.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohFailure.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohKeychainSecretStore.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohSecretKeyStore.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkRoutePinger.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/MobileIrohTransportFlag.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/NetworkInterfaceAddress.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/NetworkInterfaceAddressProviding.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/NetworkReachability.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/ReachabilityProviding.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/ReachabilityService.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/SystemNetworkInterfaceAddressProvider.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/TailscaleStatus.swiftPackages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/TailscaleStatusMonitor.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxRoutePingTests.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/PingTestListener.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/ReachabilityServiceTests.swiftPackages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/TailscaleStatusTests.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMac.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+IrohPinning.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore+Records.swiftPackages/iOS/CmuxMobilePairedMac/Sources/CmuxMobilePairedMac/MobilePairedMacStore.swiftPackages/iOS/CmuxMobilePairedMac/Tests/CmuxMobilePairedMacTests/MobilePairedMacStoreIrohPinTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileRPCTransportConnectEvent.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncRuntime.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientIrohTrustTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/TransportTestDoubles.swiftPackages/iOS/CmuxMobileShell/Package.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/BackingUpPairedMacStore.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure+Iroh.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobilePairingFailure.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+AuthorizationFailure.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ManualAttachTicket.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectRoutes.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+TerminalReplayLifecycle.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceCreateRequest.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellMacAvailabilityFailureClassifier.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PairedMacBackupRecord.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PairedMacBackupRecordWire.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DialDiagnosticFailingTransport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DialDiagnosticRecorder.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DialDiagnosticTransportFactory.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileDialDiagnosticsTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingFailureTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectRouteSelectionTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CmxAttachTransportKind+MobileDisplay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileMacConnectionStatus+Display.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileMacConnectionStatusPill.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileMacConnectionStatusRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/OnboardingPage.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SetupHelpGateContent.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SetupHelpView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TailscaleInactiveCallout.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+DerivedState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListConnectionChromeTests.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/MobileCatalogSection.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/SettingCatalogTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/MobilePairingRoute.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/MobilePairingStatusSnapshot.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swiftSources/Mobile/MobileHostByteConnection.swiftSources/Mobile/MobileHostIrohConnectionAdapter.swiftSources/Mobile/MobileHostIrohFFI.swiftSources/Mobile/MobileHostIrohFlag.swiftSources/Mobile/MobileHostIrohRoute.swiftSources/Mobile/MobileHostIrohSecretKeyStore.swiftSources/Mobile/MobileHostNWConnectionAdapter.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileRouteResolver.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobileHostIrohSecretKeyTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftdocs/ios-swift-mobile-plan.mddocs/pro-badge-options.htmlios/cmux-ios.xcodeproj/project.pbxprojios/cmux/AppCompositionRoot.swiftios/cmux/Resources/Localizable.xcstringsios/cmux/cmuxApp.swiftios/cmuxPackage/Package.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swiftios/scripts/cloud-testflight.shios/scripts/reload.shnative/cmux-iroh/.gitignorenative/cmux-iroh/Cargo.tomlnative/cmux-iroh/include/cmux_iroh_ffi.hnative/cmux-iroh/include/module.modulemapnative/cmux-iroh/src/lib.rsplans/feat-ios-iroh/DESIGN.mdreports.mdscripts/check-package-resolved-policy.pyscripts/ensure-cmux-iroh.shscripts/install-rust-ci.shscripts/reload.shscripts/setup.shweb/messages/en.jsonweb/messages/ja.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (29)
.github/workflows/ios-testflight.yml (1)
369-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Separate Rust installation and FFI provisioning into distinct steps.
For consistency with other workflows (such as
ci.ymlandnightly.yml), consider separating the Rust installation and the cmux-iroh provisioning into distinct steps. Assuminginstall-rust-ci.shconfigures$GITHUB_PATH, this also removes the need to manually export thePATHwithin the same step.♻️ Proposed refactor
- - name: Provision cmux-iroh FFI - run: | - ./scripts/install-rust-ci.sh - export PATH="$HOME/.cargo/bin:$PATH" - ./scripts/ensure-cmux-iroh.sh + - name: Install Rust + run: | + ./scripts/install-rust-ci.sh + + - name: Provision cmux-iroh FFI + run: | + ./scripts/ensure-cmux-iroh.sh📝 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.- name: Install Rust run: | ./scripts/install-rust-ci.sh - name: Provision cmux-iroh FFI run: | ./scripts/ensure-cmux-iroh.sh🤖 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 @.github/workflows/ios-testflight.yml around lines 369 - 374, Split the “Provision cmux-iroh FFI” workflow step into separate steps for running install-rust-ci.sh and ensure-cmux-iroh.sh. Remove the manual PATH export, relying on install-rust-ci.sh to configure GITHUB_PATH for subsequent steps, and retain the existing execution order..github/workflows/perf-activation.yml (1)
174-177: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Consider caching the Rust build artifacts.
ensure-cmux-iroh.shcompiles the Rust crate for four different targets (aarch64-apple-darwin,x86_64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-sim). Since this step runs on every CI execution, compiling from scratch adds significant overhead.Consider adding an
actions/cachestep (or usingSwatinem/rust-cache) for~/.cargo/registry,~/.cargo/git, andnative/cmux-iroh/targetprior to provisioning the FFI to speed up CI runs. This applies to the other workflows utilizing this script as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/perf-activation.yml around lines 174 - 177, Add a Rust build cache step before the “Provision cmux-iroh FFI” step, caching ~/.cargo/registry, ~/.cargo/git, and native/cmux-iroh/target with a key that varies by runner and relevant dependency/build configuration. Apply the same cache setup to other workflows that invoke ensure-cmux-iroh.sh.docs/ios-swift-mobile-plan.md (1)
43-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Renumber the milestones sequentially.
Milestone 7 appears before milestone 6. Move the rollout-defaults item after milestone 6 and number it
7.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 43-43: Ordered list item prefix
Expected: 6; Actual: 7; Style: 1/2/3(MD029, ol-prefix)
[warning] 44-44: Ordered list item prefix
Expected: 7; Actual: 6; Style: 1/2/3(MD029, ol-prefix)
🤖 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 `@docs/ios-swift-mobile-plan.md` around lines 43 - 44, Renumber the milestones in the documented rollout plan sequentially: keep the auth/reconnect milestone as 6, then place the Iroh default and transport diagnostics milestone after it as milestone 7.Source: Linters/SAST tools
ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift (1)
349-389: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not bootstrap endpoint trust from the route being validated.
Empty IDs currently succeed, the user scope can fall back to
ticket.macUserID, and missing pins are silently created fromticket.routes. This lets pairing/reconnect input become its own trust authority before credentials are sent.Separate explicit user-approved pairing/re-trust enrollment from validation. Background validation should require the authenticated scope and an authoritative existing pin, failing closed otherwise.
As per path instructions, “the pinned iroh EndpointId / iroh-route availability decision comes from the authoritative pinned record + explicit trust validation,” and missing signals must fail closed.
🤖 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/Sources/cmuxFeature/CMUXMobileRootScene.swift` around lines 349 - 389, Change the validation flow around pairedMacStore.loadAll and the currentUserID() lookup so it requires non-empty authenticated user and team scope, without falling back to ticket.macUserID. Require an existing paired record with a non-empty authoritative irohEndpointID and explicit trust validation, rejecting missing or mismatched IDs and unavailable pinned routes. Remove the upsert paths that create or populate trust from ticket.routes; handle enrollment/re-trust only through a separate explicit user-approved flow.Source: Path instructions
ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift (1)
27-27: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find all CMUXMobileRuntime construction sites to check whether iroh-capable # call sites always set irohEndpointTrustValidator explicitly. rg -nP -C3 'CMUXMobileRuntime\(' --type=swift rg -nP -C3 '\.irohEndpointTrustValidator\s*=' --type=swiftRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the runtime and RPC files first. git ls-files 'ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift' \ 'ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCClient.swift' \ 'ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCSession.swift' \ 'ios/cmuxPackage/Sources/cmuxFeature' \ | sed 's#^`#FILE` #' echo '--- OUTLINE CMUXMobileRuntime.swift ---' ast-grep outline ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift --view expanded || true echo '--- OUTLINE MobileCoreRPCClient.swift ---' ast-grep outline ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCClient.swift --view expanded || true echo '--- OUTLINE MobileCoreRPCSession.swift ---' ast-grep outline ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCSession.swift --view expanded || true echo '--- SEARCH CMUXMobileRuntime construction sites ---' rg -n -C3 'CMUXMobileRuntime\(' ios/cmuxPackage/Sources || true echo '--- SEARCH trust validator assignments/usages ---' rg -n -C3 'irohEndpointTrustValidator|supportedRouteKinds|supportedKinds|route.kind == \.iroh' ios/cmuxPackage/Sources || trueRepository: manaflow-ai/cmux
Length of output: 10430
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant source sections with line numbers. sed -n '1,260p' ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift | cat -n echo '---' sed -n '1,260p' ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCClient.swift | cat -n echo '---' sed -n '1,260p' ios/cmuxPackage/Sources/cmuxFeature/MobileCoreRPCSession.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 11616
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the runtime file itself and any declared initializers around the validator default. sed -n '1,220p' ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift | cat -n echo '---' rg -n -C4 'init\(|irohEndpointTrustValidator|supportedRouteKinds|supportedKinds' ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swift || true echo '--- ALL REFERENCES ---' rg -n -C4 'irohEndpointTrustValidator|supportedRouteKinds|supportedKinds|route.kind == \.iroh' ios/cmuxPackage/Sources || trueRepository: manaflow-ai/cmux
Length of output: 27535
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- CMUXMobileRuntime construction sites (repo-wide) ---' rg -n -C3 'CMUXMobileRuntime\(' . || true echo '--- explicit irohEndpointTrustValidator assignments (repo-wide) ---' rg -n -C3 'irohEndpointTrustValidator\s*=' . || true echo '--- route-aware transport factories / iroh support mentions ---' rg -n -C3 'supportedKinds|CmxRouteAwareByteTransportFactory|\.iroh|iroh' ios cmuxPackage .github || trueRepository: manaflow-ai/cmux
Length of output: 47566
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for path in Path('.').rglob('*.swift'): try: text = path.read_text(errors='ignore') except Exception: continue if 'CMUXMobileRuntime(' in text or 'irohEndpointTrustValidator' in text or '.iroh' in text: print(path) PYRepository: manaflow-ai/cmux
Length of output: 4693
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show all Swift files that mention the runtime or validator, then the matching lines. python3 - <<'PY' from pathlib import Path hits = [] for path in Path('ios').rglob('*.swift'): try: text = path.read_text(errors='ignore') except Exception: continue if 'CMUXMobileRuntime(' in text or 'irohEndpointTrustValidator' in text or '.iroh' in text: hits.append(str(path)) for p in sorted(set(hits)): print(p) PY echo '--- matching lines ---' rg -n -C2 'CMUXMobileRuntime\(|irohEndpointTrustValidator|\.iroh' ios || trueRepository: manaflow-ai/cmux
Length of output: 15276
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the runtime initializer call paths in the iOS feature tree. rg -n -C4 'CMUXMobileRuntime\(' ios/cmuxPackage/Sources ios/cmuxPackage/Tests || true echo '---' rg -n -C4 'irohEndpointTrustValidator\s*=\s*Self\.makeIrohEndpointTrustValidator|irohEndpointTrustValidator\s*=' ios/cmuxPackage/Sources ios/cmuxPackage/Tests || trueRepository: manaflow-ai/cmux
Length of output: 5198
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- MobileCoreRPCClient outline ---' ast-grep outline Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift --view expanded || true echo '--- MobileCoreRPCClient iroh/trust lines ---' rg -n -C4 'route\.kind == \.iroh|irohEndpointTrustValidator|supportedRouteKinds|supportedKinds|send credentials|token' Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift || true echo '--- CMUXMobileRuntime call site in app ---' sed -n '1,120p' ios/cmux/cmuxApp.swift | cat -n echo '--- CMUXMobileRootScene iroh validator setup ---' sed -n '270,380p' ios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 20194
Default
irohEndpointTrustValidatorto fail closed.
The public initializer still falls back to a no-op, so any iroh-capable caller that misses this injection will silently skip endpoint-trust checks and send credentials. Make the default throwing, and move the no-op behind an explicit insecure test/helper path.🤖 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/Sources/cmuxFeature/CMUXMobileRuntime.swift` at line 27, Update the initializer and default handling for irohEndpointTrustValidator so omitted injection uses a throwing fail-closed validator instead of a no-op. Preserve successful validation for explicitly supplied validators, and move any no-op behavior behind an explicitly named insecure test/helper path.Source: Path instructions
ios/scripts/cloud-testflight.sh (1)
235-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not suppress Iroh build failures before archiving.
|| truelets the production archive continue whenensure-cmux-iroh.shfails, potentially packaging a stale XCFramework. Skip only when intentionally supported; otherwise fail immediately.Proposed fix
- [[ -x "$REPO_ROOT/scripts/ensure-cmux-iroh.sh" ]] && ( cd "$REPO_ROOT" && ./scripts/ensure-cmux-iroh.sh ) || true + [[ -x "$REPO_ROOT/scripts/ensure-cmux-iroh.sh" ]] \ + || die "missing scripts/ensure-cmux-iroh.sh" + ( cd "$REPO_ROOT" && ./scripts/ensure-cmux-iroh.sh ) \ + || die "failed to prepare cmux Iroh FFI"📝 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.[[ -x "$REPO_ROOT/scripts/ensure-cmux-iroh.sh" ]] \ || die "missing scripts/ensure-cmux-iroh.sh" ( cd "$REPO_ROOT" && ./scripts/ensure-cmux-iroh.sh ) \ || die "failed to prepare cmux Iroh FFI"🤖 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/scripts/cloud-testflight.sh` at line 235, Update the ensure-cmux-iroh.sh invocation in the archive preparation flow to stop suppressing failures: remove the unconditional `|| true` while preserving the existing executable check and repository-root working directory. A failure from ensure-cmux-iroh.sh must immediately fail the production archive rather than allowing a stale XCFramework to be packaged.native/cmux-iroh/src/lib.rs (1)
63-71: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== file list ==\n' git ls-files native/cmux-iroh/src/lib.rs native/cmux-iroh -z | tr '\0' '\n' printf '\n== line count ==\n' wc -l native/cmux-iroh/src/lib.rs printf '\n== relevant excerpt ==\n' sed -n '1,220p' native/cmux-iroh/src/lib.rsRepository: manaflow-ai/cmux
Length of output: 6713
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== file count ==\n' git ls-files native/cmux-iroh/src/lib.rs | wc -l printf '\n== excerpt lines 1-260 ==\n' nl -ba native/cmux-iroh/src/lib.rs | sed -n '1,260p'Repository: manaflow-ai/cmux
Length of output: 241
🏁 Script executed:
#!/bin/bash set -euo pipefail nl -ba native/cmux-iroh/src/lib.rs | sed -n '1,260p'Repository: manaflow-ai/cmux
Length of output: 194
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path text = Path('native/cmux-iroh/src/lib.rs').read_text() for needle in ['runtime()', 'CmuxIrohError', 'set_error(', 'clear_error(', 'catch_unwind', 'panic']: print(f'\n== {needle} ==') for i, line in enumerate(text.splitlines(), 1): if needle in line: print(f'{i}: {line}') PYRepository: manaflow-ai/cmux
Length of output: 2738
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path lines = Path('native/cmux-iroh/src/lib.rs').read_text().splitlines() for i,l in enumerate(lines,1): if 'runtime()' in l or 'fn set_error' in l or 'fn clear_error' in l or 'enum ErrorKind' in l: start = max(1, i-8) end = min(len(lines), i+25) print(f'\n--- lines {start}-{end} around {i} ---') for j in range(start, end+1): print(f'{j}: {lines[j-1]}') PYRepository: manaflow-ai/cmux
Length of output: 15580
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path lines = Path('native/cmux-iroh/src/lib.rs').read_text().splitlines() for target in ['fn runtime()', 'fn set_error(', 'fn clear_error(']: print(f'\n== {target} ==') for i, line in enumerate(lines, 1): if target in line: start = max(1, i - 5) end = min(len(lines), i + 35) for j in range(start, end + 1): print(f'{j}: {lines[j-1]}') break print('\n== runtime() call sites ==') for i, line in enumerate(lines, 1): if 'runtime()' in line: print(f'{i}: {line}') PYRepository: manaflow-ai/cmux
Length of output: 4533
Handle Tokio runtime init failures before crossing the FFI boundary (native/cmux-iroh/src/lib.rs:63-71)
runtime()is used by several exportedextern "C"functions, soexpect("tokio runtime should build")can terminate the host process beforeCmuxIrohErroris set. Make runtime creation fallible and propagateErrorKind::Internalinstead.🤖 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 `@native/cmux-iroh/src/lib.rs` around lines 63 - 71, Update runtime() and its exported extern "C" callers to make Tokio runtime creation fallible instead of using expect. Propagate initialization failures as CmuxIrohError with ErrorKind::Internal before crossing the FFI boundary, while preserving the existing successful runtime reuse behavior.Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swift (1)
209-247: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix actor reentrancy: prevent redundant connections by moving async setup into the detached task.
Adding
await transportConnectObserver?(.attempt)introduces a new actor suspension point beforeconnectionTaskis populated. If multiple RPC requests arrive concurrently while no transport exists, they will all pass theif let existing = connectionTaskcheck, suspend at this observer, and then spawn redundantmakeTransport()dials to the Mac.Although the end of the method cleans up losing races, this still causes redundant dials and connection churn. To eliminate the race window and guarantee all concurrent callers join the exact same connection attempt, move the observer and
makeTransport()inside the detachedTaskso you can synchronously assignconnectionTaskimmediately after acquiring the lease.🔒️ Proposed fix to securely capture state and run setup within the Task
- let connectStartedAt = ContinuousClock.now - await transportConnectObserver?(.attempt) - let candidate: any CmxByteTransport - do { - candidate = try makeTransport() - } catch { - await transportConnectObserver?( - .failed( - error: error, - elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) - ) - ) - await connectAttemptRegistry.clearFinishedConnect(lease: connectLease) - throw error - } connectionID = UUID() let transportConnectObserver = transportConnectObserver + let makeTransport = self.makeTransport + let registry = self.connectAttemptRegistry + let capturedLease = connectLease task = Task.detached { + let connectStartedAt = ContinuousClock.now + await transportConnectObserver?(.attempt) + let candidate: any CmxByteTransport + do { + candidate = try makeTransport() + } catch { + await transportConnectObserver?( + .failed( + error: error, + elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) + ) + ) + await registry.clearFinishedConnect(lease: capturedLease) + throw error + } + do { let connected = try await withTaskCancellationHandler { try await candidate.connect()📝 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.connectionID = UUID() let transportConnectObserver = transportConnectObserver let makeTransport = self.makeTransport let registry = self.connectAttemptRegistry let capturedLease = connectLease task = Task.detached { let connectStartedAt = ContinuousClock.now await transportConnectObserver?(.attempt) let candidate: any CmxByteTransport do { candidate = try makeTransport() } catch { await transportConnectObserver?( .failed( error: error, elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) ) ) await registry.clearFinishedConnect(lease: capturedLease) throw error } do { let connected = try await withTaskCancellationHandler { try await candidate.connect() return candidate } onCancel: { Task { await candidate.close() } } await transportConnectObserver?( .connected(elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt)) ) return connected } catch { await transportConnectObserver?( .failed( error: error, elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) ) ) throw error🤖 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/MobileCoreRPCSession.swift` around lines 209 - 247, Move the asynchronous .attempt observer call and makeTransport() setup from the actor-isolated connection path into the detached task created by the connection method. Assign connectionTask synchronously immediately after acquiring the connect lease, ensuring concurrent callers join the same task; preserve failure reporting, lease cleanup, cancellation handling, and connectionID setup using the captured state.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectRoutes.swift (2)
135-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SwiftLint: prefer an empty array over
[CmxAttachRoute]?forfreshReconnectRoutesAfterLocalFailure.The optional return here only ever distinguishes "nothing to try" (nil) from "a non-empty list of fresh routes" (the
guard !refreshed.isEmpty else { return nil }a few lines below guarantees the non-nil case is never empty). Returning[]instead ofniland having the caller checkisEmptywould remove the discouraged optional-collection pattern flagged by SwiftLint without losing any information.🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 139-139: 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/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ReconnectRoutes.swift around lines 135 - 139, Update freshReconnectRoutesAfterLocalFailure to return [CmxAttachRoute] instead of an optional, returning [] when refreshed routes are empty. Adjust every caller to check isEmpty rather than optional binding, preserving the existing behavior for non-empty fresh routes.Source: Linters/SAST tools
282-291: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Two near-duplicate route-key builders disagree on which fields identify a peer route.
reconnectRouteKey(used byreconnectRoutes's dedup and byfreshReconnectRoutesAfterLocalFailure's tried-vs-fresh comparison) includesrelayHintin the.peerkey, whilemergedReconnectRoutes's localendpointKey(used when merging ticket + stored routes) intentionally dropsrelayHintfrom the same case. Two peer routes that differ only inrelayHintare therefore treated as duplicates by one dedup path and as distinct routes by the other, so a route already collapsed bymergedReconnectRoutescan reappear as two separate reconnect candidates viareconnectRoutes, andfreshReconnectRoutesAfterLocalFailure's "did anything really change" comparison can spuriously report a change.Extract a single shared key function (e.g. keep
mergedReconnectRoutes's narrower field set, or intentionally includerelayHintin both) and have both call sites use it.Also applies to: 328-337
🤖 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`+ReconnectRoutes.swift around lines 282 - 291, Unify peer-route identity between reconnectRouteKey and mergedReconnectRoutes’ local endpointKey by extracting one shared key-building function and reusing it in both paths. Ensure the shared key applies the same field set for .peer routes, including or excluding relayHint consistently, so deduplication and freshReconnectRoutesAfterLocalFailure comparisons cannot disagree.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swift (1)
41-45: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use the route owned by this workspace’s Mac.
workspacemay belong to an aggregated secondary Mac, whilestore.activeRouteis the foreground route. Combining workspace-specific status with the foreground transport mislabels the pill and toolbar subtitle. Resolve the transport kind from the workspace’s per-Mac connection snapshot instead.🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailContainer.swift` around lines 41 - 45, Update the activeTransportKind argument in WorkspaceDetailContainer’s WorkspaceDetailView call to use the transport kind from workspace’s per-Mac connection snapshot, rather than store.activeRoute. Keep the workspace-specific connection status and ensure the displayed pill and toolbar subtitle reflect the Mac that owns workspace.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+DerivedState.swift (1)
11-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Base the subtitle on the visible terminal fallback.
Use
selectedTerminalrather than independently resolvingstore.selectedTerminalID; nil or stale IDs currently make the subtitle disagree with the displayed first terminal.Based on learnings, terminal UI selection must consistently use the same first-terminal fallback as
selectedTerminal.Proposed fix
var selectedToolbarSubtitle: String? { - guard let selectedTerminalID = store.selectedTerminalID else { return nil } - let terminalName = workspace.terminals.first { $0.id == selectedTerminalID }?.name + guard let terminalName = selectedTerminal?.name else { return nil } guard connectionStatus == .connected, let activeTransportKind else { return terminalName } return activeTransportKind.mobileToolbarSubtitle(terminalName: terminalName) }📝 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.var selectedToolbarSubtitle: String? { guard let terminalName = selectedTerminal?.name else { return nil } guard connectionStatus == .connected, let activeTransportKind else { return terminalName } return activeTransportKind.mobileToolbarSubtitle(terminalName: terminalName)🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+DerivedState.swift around lines 11 - 18, Update selectedToolbarSubtitle to derive the terminal name from the existing selectedTerminal property instead of resolving store.selectedTerminalID and searching workspace.terminals directly. Preserve the current connection-status and activeTransportKind behavior while ensuring nil or stale selections use the same visible first-terminal fallback.Source: Learnings
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swift (1)
148-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Verify new localization keys have translations for all supported locales in Localizable.xcstrings. KEYS=( "settings.mobile.iOSPairingIrohTransport" "settings.mobile.iOSPairingIrohTransport.subtitleOn" "settings.mobile.iOSPairingIrohTransport.subtitleOff" "settings.mobile.iOSPairingPublishesTailscaleRoutes" "settings.mobile.iOSPairingPublishesTailscaleRoutes.subtitleOn" "settings.mobile.iOSPairingPublishesTailscaleRoutes.subtitleOff" "settings.mobile.activeTransport" "settings.mobile.activeTransport.subtitle" "settings.mobile.activeTransport.none" "settings.mobile.routes.empty" "settings.mobile.routes.title" ) if [ ! -f "Resources/Localizable.xcstrings" ]; then echo "Resources/Localizable.xcstrings not found in the repository root." exit 0 fi # Extract all locales defined in the catalog LOCALES=$(jq -r '.strings | to_entries | .[0].value.localizations | keys | .[]?' Resources/Localizable.xcstrings 2>/dev/null || echo "") if [ -z "$LOCALES" ]; then echo "Could not parse locales from Localizable.xcstrings." exit 0 fi echo "Verifying keys against locales: $LOCALES" for key in "${KEYS[@]}"; do for locale in $LOCALES; do HAS_LOCALE=$(jq -r ".strings[\"$key\"].localizations[\"$locale\"] // empty" Resources/Localizable.xcstrings 2>/dev/null) if [ -z "$HAS_LOCALE" ]; then echo "Missing translation for '$key' in locale '$locale'" fi done doneRepository: manaflow-ai/cmux
Length of output: 16345
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate localization catalogs and inspect the relevant keys/locale structure. fd -a 'Localizable.xcstrings' . || true echo '---' CATALOG=$(fd -a 'Localizable.xcstrings' . | head -n 1) if [ -z "${CATALOG:-}" ]; then exit 0 fi python3 - <<'PY' "$CATALOG" import json, sys from pathlib import Path path = Path(sys.argv[1]) data = json.loads(path.read_text(encoding="utf-8")) strings = data.get("strings", {}) keys = [ "settings.mobile.iOSPairingIrohTransport", "settings.mobile.iOSPairingIrohTransport.subtitleOn", "settings.mobile.iOSPairingIrohTransport.subtitleOff", "settings.mobile.iOSPairingPublishesTailscaleRoutes", "settings.mobile.iOSPairingPublishesTailscaleRoutes.subtitleOn", "settings.mobile.iOSPairingPublishesTailscaleRoutes.subtitleOff", "settings.mobile.activeTransport", "settings.mobile.activeTransport.subtitle", "settings.mobile.activeTransport.none", "settings.mobile.routes.empty", "settings.mobile.routes.title", ] # Collect locales from every string entry. locales = sorted({loc for entry in strings.values() for loc in entry.get("localizations", {}).keys()}) print("Locales:", ", ".join(locales)) print() for key in keys: entry = strings.get(key) if not entry: print(f"MISSING KEY: {key}") continue locs = entry.get("localizations", {}) missing = [loc for loc in locales if loc not in locs] print(f"{key}: present={len(locs)}/{len(locales)} missing={len(missing)}") if missing: print(" missing:", ", ".join(missing)) # show state for each localization if present for loc in locales: if loc in locs: v = locs[loc] state = v.get("state") value = v.get("stringUnit", {}).get("value") if value is None: value = v.get("variations", {}) print(f" {loc}: state={state!r} value={value!r}") print() PYRepository: manaflow-ai/cmux
Length of output: 1959
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the exact structure for one representative key to determine whether # the earlier check is missing nested translation data or simply reading the wrong fields. CATALOG=$(fd -a 'Localizable.xcstrings' . | head -n 1) [ -n "${CATALOG:-}" ] python3 - <<'PY' "$CATALOG" import json, sys from pathlib import Path path = Path(sys.argv[1]) data = json.loads(path.read_text(encoding="utf-8")) key = "settings.mobile.iOSPairingIrohTransport" entry = data["strings"].get(key) print(json.dumps(entry, indent=2, ensure_ascii=False)[:12000]) PYRepository: manaflow-ai/cmux
Length of output: 159
Add the missing localization entries for these settings strings.
Resources/Localizable.xcstringsdoes not contain the new pairing/transport keys, so they have no cataloged translations yet; add them for the catalog’s supported locales.🤖 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/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swift` around lines 148 - 181, Add the missing localization catalog entries in Localizable.xcstrings for every key used by irohTransportRow and publishTailscaleRoutesRow, including both labels and on/off subtitles. Provide entries for all supported locales using the catalog’s existing format and translation conventions.Source: Path instructions
Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransport.swift (2)
125-135: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make
connect()own the in-flight lifecycle.
connect()still suspends after the initial state check, so anotherconnect()orclose()can interleave and leave the actor with duplicate or stale.readystate.receive()also closes the FFI handle on cancellation without transitioningstateto.closed, so later calls can observe a live state backed by a dead connection.Add a
.connectingstate or stored in-flight task, and route cancellation/close through the same transition so the handle is released exactly once.🤖 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/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransport.swift` around lines 125 - 135, Update CmxIrohByteTransport.connect() to track an in-flight connection using a .connecting state or stored task, preventing concurrent connect() and close() operations from producing duplicate or stale .ready state. Ensure cancellation and close transition through the same lifecycle path, and update receive() cancellation handling to set state to .closed when releasing the FFI handle. Guarantee the handle is released exactly once.
136-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Map endpoint-binding failures into
CmxIrohByteTransportError.
boundEndpointBeforeDeadline()can throwCmxIrohFailure, but this call sits outside the mappingdoblock, so callers receive the raw FFI failure instead of.endpointBindFailed. The existingbindFailedhelper is never applied.Proposed fix
- let endpoint = try await boundEndpointBeforeDeadline() + let endpoint: CmxIrohEndpointReference + do { + endpoint = try await boundEndpointBeforeDeadline() + } catch let failure as CmxIrohFailure { + throw CmxIrohByteTransportError.bindFailed(failure) + }📝 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.let connectStartedAt = DispatchTime.now().uptimeNanoseconds let endpoint: CmxIrohEndpointReference do { endpoint = try await boundEndpointBeforeDeadline() } catch let failure as CmxIrohFailure { throw CmxIrohByteTransportError.bindFailed(failure) } let elapsed = DispatchTime.now().uptimeNanoseconds - connectStartedAt guard elapsed < connectTimeoutNanoseconds else { throw CmxIrohByteTransportError.endpointBindFailed( "iroh endpoint key load timed out", .timedOut ) }🤖 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/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohByteTransport.swift` around lines 136 - 144, Move the boundEndpointBeforeDeadline() call into the existing error-mapping do block so any CmxIrohFailure is converted through the bindFailed helper into CmxIrohByteTransportError.endpointBindFailed. Preserve the elapsed-time check and timeout behavior, while ensuring callers do not receive the raw FFI failure.Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohFFIClient.swift (1)
6-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release native handles when Swift ownership ends.
Both wrappers release only through explicit
close(). Dropping an unclosed reference leaks its native endpoint or connection. Add an idempotent destruction fallback.Proposed fix
final class CmxIrohEndpointReference: `@unchecked` Sendable { + deinit { + close() + } } final class CmxIrohConnectionReference: `@unchecked` Sendable { + deinit { + close() + } }Also applies to: 68-78
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 6-6: Classes should have an explicit deinit method
(required_deinit)
🤖 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/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxIrohFFIClient.swift` around lines 6 - 16, Update CmxIrohEndpointReference and the corresponding connection wrapper to add an idempotent deinitialization fallback that releases the native handle when Swift ownership ends, while preserving explicit close() behavior and synchronization with active uses. Ensure destruction cannot release either handle more than once.Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift (2)
48-55: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep implementation diagnostics behind a typed, localized error boundary.
Transport-generated English and raw error descriptions currently flow into UI-facing ping results, preventing complete localization and risking disclosure of FFI/provider internals.
Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift#L48-L55: keep public failure contracts typed rather than carrying presentation copy.Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift#L693-L709: remove hard-coded user-facing English from the Network.framework mapping.Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkRoutePinger.swift#L54-L56: map unexpected TCP errors to a safe typed failure.Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkRoutePinger.swift#L90-L92: map unexpected Iroh errors similarly and retain details only in sanitized diagnostics.As per coding guidelines and path instructions, user-facing errors must be localized and must not expose raw upstream or internal details.
📍 Affects 2 files
Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift#L48-L55(this comment)Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift#L693-L709Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkRoutePinger.swift#L54-L56Packages/Shared/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkRoutePinger.swift#L90-L92🤖 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/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift` around lines 48 - 55, Replace String-based associated values in CmxNetworkByteTransport’s connectionFailed, receiveFailed, and sendFailed cases with typed, localizable failure values. In CmxNetworkByteTransport.swift lines 693-709, map Network.framework failures to those typed cases without hard-coded English or raw descriptions. In CmxNetworkRoutePinger.swift lines 54-56 and 90-92, map unexpected TCP and Iroh errors to safe typed failures while retaining upstream details only in sanitized diagnostics.Sources: Coding guidelines, Path instructions
565-572: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the send slot occupied until the canceled write completes.
Cancellation clears
sendContinuation, but it does not cancel the underlyingNWConnection.send. A second send can therefore overlap it, and the first callback—including a connection error—is discarded because its operation ID no longer matches.Proposed fix
private func cancelSend(operationID: UUID) { if let pending = sendContinuation, pending.id == operationID { - sendContinuation = nil - cancelledOperationIDs.insert(operationID) + // Keep the slot occupied until Network.framework completes the write. + sendContinuation = (id: pending.id, continuation: nil) pending.continuation?.resume(throwing: CancellationError()) } else { cancelledOperationIDs.insert(operationID) } }📝 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.private func cancelSend(operationID: UUID) { if let pending = sendContinuation, pending.id == operationID { // Keep the slot occupied until Network.framework completes the write. sendContinuation = (id: pending.id, continuation: nil) pending.continuation?.resume(throwing: CancellationError()) } else { cancelledOperationIDs.insert(operationID) } }🤖 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/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swift` around lines 565 - 572, Update cancelSend to leave the matching sendContinuation occupied until the underlying NWConnection.send completion callback finishes; mark the operation canceled and resume its waiting continuation, but do not clear the slot there. Adjust the completion handling to recognize the canceled operation, clear the slot only after the callback, and preserve any connection error rather than discarding it when the operation ID matches the canceled send.Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift (1)
143-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the packaged XCFramework path instead of silently skipping this test.
Package.swiftuses package-localCmuxIrohFFIBinary.xcframework, but this checks repository-rootCmuxIrohFFI.xcframework. The guard therefore bypasses the native loopback test.Proposed fix
- let xcframeworkURL = packageURL.appending(path: "../../../CmuxIrohFFI.xcframework").standardizedFileURL + let xcframeworkURL = packageURL.appending(path: "CmuxIrohFFIBinary.xcframework") guard FileManager.default.fileExists(atPath: xcframeworkURL.path) else { + Issue.record("Missing packaged Iroh XCFramework at \(xcframeworkURL.path)") return }📝 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.let packageURL = URL(filePath: `#filePath`) .deletingLastPathComponent() .deletingLastPathComponent() .deletingLastPathComponent() let xcframeworkURL = packageURL.appending(path: "CmuxIrohFFIBinary.xcframework") guard FileManager.default.fileExists(atPath: xcframeworkURL.path) else { Issue.record("Missing packaged Iroh XCFramework at \(xcframeworkURL.path)") return🤖 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/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift` around lines 143 - 149, Update the xcframeworkURL construction in CmxIrohByteTransportTests to target the package-local CmuxIrohFFIBinary.xcframework path used by Package.swift, rather than the repository-root CmuxIrohFFI.xcframework location. Keep the existing file-existence guard and native loopback test flow unchanged once it resolves the packaged framework correctly.Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift (1)
68-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make concurrent transport tests event-driven rather than scheduler-dependent.
Both tests can pass or fail based on task scheduling instead of the intended transport invariant.
Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift#L68-L73: await an explicit receive-start signal before closing.Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift#L220-L233: await the send event without racing it against a 250 ms sleep.As per coding guidelines, tests must use real completion signals rather than fixed-duration or scheduler-dependent waits.
📍 Affects 2 files
Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift#L68-L73(this comment)Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift#L220-L233🤖 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/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift` around lines 68 - 73, Replace the scheduler-dependent synchronization in the concurrent transport tests with explicit event signals: in Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swift:68-73, await a receive-start signal before calling transport.close; in Packages/Shared/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxIrohByteTransportTests.swift:220-233, await the send-completion event directly and remove the race against the 250 ms sleep. Keep each test’s existing transport behavior and assertions unchanged.Source: Coding guidelines
reports.md (1)
68-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the test heading.
Insert an empty line after Line 70 so the Markdown heading is separated from the following list.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 70-70: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
🤖 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 `@reports.md` around lines 68 - 70, Add a blank line after the TailscaleStatusTests.swift test heading staleEvaluationCannotOverwriteFresherRefresh in reports.md, separating the heading from the following list.Source: Linters/SAST tools
Resources/Localizable.xcstrings (1)
123525-123537: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include LAN in the missing-route message.
The product supports Tailscale/LAN fallback routes, but this string says no route is available when only an LAN route may be present. Use “No Iroh, Tailscale, or LAN route is available” or a generic “No fallback route is available” wording.
🤖 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 `@Resources/Localizable.xcstrings` around lines 123525 - 123537, Update the localized value for the mobile.pairing.req.route.missing key to mention LAN alongside Iroh and Tailscale, using wording such as “No Iroh, Tailscale, or LAN route is available.” Apply the equivalent meaning consistently to the affected localization.scripts/ensure-cmux-iroh.sh (2)
50-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the build environment in
BUILD_KEY.The cache currently survives Rust/Xcode/SDK or deployment-target changes, allowing an incompatible static archive to be reused. Hash the toolchain versions, target list, SDK versions, and effective
IPHONEOS_DEPLOYMENT_TARGET/MACOSX_DEPLOYMENT_TARGET.🤖 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/ensure-cmux-iroh.sh` around lines 50 - 62, Update hash_sources in ensure-cmux-iroh.sh to include the build environment in the BUILD_KEY: hash Rust and Xcode toolchain versions, configured target list, SDK versions, and effective IPHONEOS_DEPLOYMENT_TARGET and MACOSX_DEPLOYMENT_TARGET values before hashing the source files. Reuse the script’s existing environment and toolchain variables where available, and preserve the current source-content hashing behavior.
115-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the unbounded polling lock.
If a builder is killed before cleanup,
LOCK_DIRremains and every future build sleeps forever. Use an OS-backed advisory lock or race-safe independent temporary builds with atomic publication.As per path instructions, “For production runtime, script, and build changes, flag fixed sleeps, timers, delayed dispatch, polling, or wall-clock waits used as synchronization.”
🤖 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/ensure-cmux-iroh.sh` around lines 115 - 123, Replace the unbounded mkdir/sleep polling loop around LOCK_DIR with an OS-backed advisory lock or race-safe independent temporary build strategy followed by atomic publication. Ensure stale locks cannot block future builds indefinitely, while preserving the cached XCFramework fast path through link_local_xcframework and the existing successful-build behavior.Source: Path instructions
Sources/Mobile/MobileHostIrohSecretKeyStore.swift (1)
21-29: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make secret-key creation atomic so the persisted EndpointId cannot change during concurrent startup.
Two callers can both load
nil, generate different keys, and return different values because the duplicate-add path overwrites the first key. A detached startup surviving cancellation can therefore replace the key backing an already-bound endpoint and invalidate client pins.
Sources/Mobile/MobileHostIrohSecretKeyStore.swift#L21-L29: use an atomic store-if-absent operation that returns the persisted winner.Sources/Mobile/MobileHostIrohSecretKeyStore.swift#L67-L87: onerrSecDuplicateItem, load and return the established key; reserve replacement for an explicit rotation path.As per coding guidelines, persisted identity must have one owner and invalid shared-state combinations must not be representable.
📍 Affects 1 file
Sources/Mobile/MobileHostIrohSecretKeyStore.swift#L21-L29(this comment)Sources/Mobile/MobileHostIrohSecretKeyStore.swift#L67-L87🤖 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/MobileHostIrohSecretKeyStore.swift` around lines 21 - 29, Make secret-key creation atomic in secretKey() by using the store-if-absent operation and returning the persisted winner rather than allowing concurrent generated keys to replace each other. Update the duplicate-item handling in the persistence helper at Sources/Mobile/MobileHostIrohSecretKeyStore.swift lines 67-87 to load and return the established key on errSecDuplicateItem; keep replacement available only through an explicit rotation path. Both affected sites are in the same file: lines 21-29 require the atomic creation flow, and lines 67-87 require duplicate handling to preserve the single persisted identity.Source: Coding guidelines
Sources/Mobile/MobileHostService.swift (3)
1009-1024: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive the active lane and advertised route from one authoritative current route.
Startup marks the lane active even when no route was obtained, while later lookup returns stale cached route data after an authoritative FFI lookup failure.
Sources/Mobile/MobileHostService.swift#L1009-L1024: require a valid normalized route before transitioning to.active; otherwise close the endpoint and fail closed.Sources/Mobile/MobileHostService.swift#L1103-L1112: clear and withdraw the cached route when current lookup fails instead of returning the previous value.As per path instructions, Iroh route availability must use one authoritative source and fail closed when that source is absent.
📍 Affects 1 file
Sources/Mobile/MobileHostService.swift#L1009-L1024(this comment)Sources/Mobile/MobileHostService.swift#L1103-L1112🤖 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 1009 - 1024, Use one authoritative, normalized route source for Iroh availability: in finishIrohStartup, require a valid route before setting the lane active or starting the accept loop; if absent, close the endpoint and fail closed. In Sources/Mobile/MobileHostService.swift lines 1103-1112, clear and withdraw the cached route when the current FFI lookup fails instead of returning stale route data.Source: Path instructions
1060-1090: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Route accept-loop termination through the service owner instead of polling.
The 500 ms timed accept continuously polls. Worse,
.endpointClosedexits while the service still advertises an active endpoint, and persistent errors immediately spin and flood logs. Use a cancellable/close-unblocked accept contract and a generation-checked owner transition that withdraws the route and marks or restarts the lane.As per coding guidelines, runtime lifecycle synchronization must not rely on polling, and state transitions need one explicit owner.
🤖 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 1060 - 1090, Update the iroh accept loop in the service owner around irohAcceptTask and acceptByteConnectionOffMain to use a cancellable, close-unblocked accept operation instead of the 500 ms timeout polling. Route endpoint closure and persistent accept failures through a generation-checked MobileHostService transition that withdraws the active route and marks or restarts the lane, ensuring only the owner performs lifecycle state changes and preventing immediate error-spin log flooding.Sources: Coding guidelines, Path instructions
1854-1861: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the new test-only lifecycle seam from production Sources.
debugWaitForIrohStartupForTesting()adds a DEBUG-only, test-named production API. Expose an internal lifecycle signal through@testable import, or observe the existing status transition from the test target.As per path instructions, production
Sources/files must not add test/debug-only accessors.🤖 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 1854 - 1861, Remove the DEBUG-only debugWaitForIrohStartupForTesting() accessor from MobileHostService in production Sources. Update the tests to use an internal lifecycle signal accessible via `@testable` import or observe the existing status transition instead, while preserving the startup-wait behavior.Sources: Coding guidelines, Path instructions
web/messages/en.json (1)
4-15: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check if updated keys in en.json exist in other locale files. # Extract a sample of updated keys (e.g., ios.metaDescription, ios.subtitle, ios.byoNetwork) KEYS=("metaDescription" "subtitle" "byoNetwork") for locale_file in web/messages/*.json; do if [[ "$locale_file" == "web/messages/en.json" ]]; then continue; fi echo "Checking $locale_file..." for key in "${KEYS[@]}"; do if ! jq -e ".ios.\"$key\"" "$locale_file" >/dev/null 2>&1; then echo "Missing key: ios.$key in $locale_file" fi done doneRepository: manaflow-ai/cmux
Length of output: 3679
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map supported locales and inspect the ios namespace in the English catalog. python3 - <<'PY' import json, pathlib routing = json.loads(pathlib.Path("web/i18n/routing.ts").read_text()) print("routing.ts exists:", pathlib.Path("web/i18n/routing.ts").exists()) PY echo "--- supported locales ---" sed -n '1,220p' web/i18n/routing.ts echo "--- en.json ios keys ---" python3 - <<'PY' import json, pathlib data = json.loads(pathlib.Path("web/messages/en.json").read_text()) ios = data.get("ios", {}) for k in ios.keys(): print(k) PY echo "--- sample locale structure (ja.json) ---" python3 - <<'PY' import json, pathlib data = json.loads(pathlib.Path("web/messages/ja.json").read_text()) ios = data.get("ios", {}) print("ios keys:", list(ios.keys())[:20]) print("has metaDescription:", "metaDescription" in ios) print("has subtitle:", "subtitle" in ios) print("has byoNetwork:", "byoNetwork" in ios) PYRepository: manaflow-ai/cmux
Length of output: 754
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "--- supported locales ---" sed -n '1,220p' web/i18n/routing.ts echo "--- en.json ios keys ---" python3 - <<'PY' import json, pathlib data = json.loads(pathlib.Path("web/messages/en.json").read_text()) ios = data.get("ios", {}) for k in ios.keys(): print(k) PY echo "--- sample locale structure (ja.json) ---" python3 - <<'PY' import json, pathlib data = json.loads(pathlib.Path("web/messages/ja.json").read_text()) ios = data.get("ios", {}) print("ios keys:", list(ios.keys())[:20]) print("has metaDescription:", "metaDescription" in ios) print("has subtitle:", "subtitle" in ios) print("has byoNetwork:", "byoNetwork" in ios) PYRepository: manaflow-ai/cmux
Length of output: 1962
Add the new iOS strings to every locale file.
ios.metaDescription,ios.subtitle, andios.byoNetworkare missing from mostweb/messages/*.jsonlocale files, so those locales will fall back to English.🤖 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 `@web/messages/en.json` around lines 4 - 15, Add the iOS translation keys metaDescription, subtitle, and byoNetwork to every locale file under web/messages, preserving each file’s existing locale structure and providing translated values where applicable. Ensure all locales define these keys so they do not fall back to English.Source: Path instructions
1ac66cd to
4a50a3c
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Greptile SummaryThis PR adds ordered
Confidence Score: 5/5Safe to merge; the diagnostic plumbing is purely additive and the cancellation fix is well-covered by new tests. The cancellation guard — which was the one correctness concern raised in the previous round — is now handled by two independent checks: a direct No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix(mobile-rpc): suppress cancelled conn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch { | ||
| await transportConnectObserver?( | ||
| .failed( | ||
| error: error, | ||
| elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) | ||
| ) | ||
| ) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
When tearDown cancels the in-flight connection task, Task.checkCancellation() or candidate.connect() throws CancellationError, which falls into the catch block and calls transportConnectObserver?(.failed(error: CancellationError, ...)). At the consumer side, mobileDialFailureKind has no CancellationError branch, so every intentional cancellation is logged as failure_kind=transport_error:CancellationError. This inflates observed transport-failure counts in the dial log whenever the user navigates away or a session tears down mid-connect. The fix is to rethrow without calling the observer when the error is a cancellation, so .attempt without a corresponding .connected or .failed implicitly signals that the attempt was abandoned.
| } catch { | |
| await transportConnectObserver?( | |
| .failed( | |
| error: error, | |
| elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) | |
| ) | |
| ) | |
| throw error | |
| } | |
| } | |
| } catch { | |
| if !(error is CancellationError) { | |
| await transportConnectObserver?( | |
| .failed( | |
| error: error, | |
| elapsedMilliseconds: Self.elapsedMilliseconds(since: connectStartedAt) | |
| ) | |
| ) | |
| } | |
| throw error | |
| } |
x90skysn3k
changed the title
2 participants
Summary
mobile.dial.attempt,mobile.dial.connected, andmobile.dial.faileddiagnostics around the underlying transport connect lifecycleconnect()to finish with a domain error instead ofCancellationErrormain's authoritative authenticatedCmuxIrohTransportimplementation from Run cmux iOS over authenticated Iroh transport #7908; an authenticated Iroh route remains fail-closed and never downgrades to raw TailscaleThe earlier duplicate native-FFI transport implementation was dropped during the rebase because #7908 now owns production Iroh transport, broker admission, relay policy, and authenticated path-hint fallback.
Regression evidence
baa62fdfb4:callerCancellationClosedErrorDoesNotEmitFailedAndAllowsRetryfailed exactly because the cancelled closed connect emitted one.failedevent8fb97e2648: normalizesTask.isCancelledbefore observer notification; the new regression and existing cancellation retry test both passVerification
swift test --package-path Packages/iOS/CmuxMobileShell --filter MobileDialDiagnosticsTests— 2 tests passedxcodebuild test ... -only-testing:cmuxFeatureTestson an iOS Simulator at051310aebb— 188 tests passed, includingterminalInputResyncsOutputWhenMacSequenceIsAhead; final commits touch only MobileRPC cancellation diagnostics and their focused testsscripts/reload.sh --tag iroh-rebase --launchat8fb97e2648— tagged macOS build succeeded; tag-bound CLI workspace smoke test passed8fb97e2648succeeded, was signed, and installed on a physical iPhone 16 Pro Max; automatic launch awaits device unlockCMUXAuthEnvironment=production, display namecmux BETA Iroh, and Stack OAuth callback schemestack-auth-mobile-oauth-urlReview
origin/main...8fb97e2648found no concrete blockers