Repository navigation
Device registry P1: auto-pair the phone on reload - #5626
Merged
Merged
Conversation
Server-side device registry so a phone can auto-pair on reload instead of re-scanning a QR. Two-level model: `devices` (a physical Mac/host, keyed by a cmux-generated persisted UUID) -> `device_app_instances` (one running cmux build/tag, holding the attach routes). Both carry a Stack `team_id` and a flexible `labels` jsonb. Registry is a best-effort rendezvous layer: a phone keeps its local paired-Mac store and falls back to it when the registry is down, so pairing survives the cloud dying. - web/db/schema.ts: devices + device_app_instances tables, key-pinning seam documented (device id will later anchor a pinned key for revoke). - web/app/api/devices/route.ts: POST register (idempotent per device / per (device, tag)), GET list, DELETE; team-scoped via X-Cmux-Team-Id and rejects teams the caller is not a member of; per-team cap with an advisory lock. - migration generated via drizzle-kit (v1 snapshot chain). - tests/devices-route.test.ts: register+list, route-refresh on re-register (the auto-pair path), non-member team 403, delete cascade. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mac (Sources/Cloud/DeviceRegistryClient.swift): registers this Mac + its app instance's attach routes in the team registry, observing MobileHostService.statusUpdates() and POSTing /api/devices whenever the route set changes (moved networks / new port). Mirrors PhonePushClient auth (Bearer + X-Stack-Refresh-Token + X-Cmux-Team-Id, AuthEnvironment.vmAPIBaseURL), best-effort and non-blocking. Gated implicitly on non-empty routes, so it only registers once the user has enabled mobile pairing (no separate opt-in). A pure shouldReRegister() skips redundant POSTs on connection-only status ticks; wired in AppDelegate next to PhonePushClient/MobileHostService configure. iOS (Packages/CmuxMobileShell): MobileDeviceIdentity persists a cmux-generated device UUID (mirrors the Mac's MobileHostIdentity, not a hardware fingerprint). DeviceRegistryService reads /api/devices for a Mac's fresh routes; injected into MobileShellComposite as DeviceRegistryRefreshing. On reconnect, a detached non-blocking refresh fetches registry routes and, via the pure selectReconnectRoutes() (registry-fresh wins, falls back to local when the registry is empty/unreachable), writes fresher routes into MobilePairedMacStore so the next reconnect trigger reaches a moved Mac. The connect itself still uses local routes with zero added latency on the common case. CMUXMobileRootScene builds the service over the AuthCoordinator (tokens + resolvedTeamID). Phone self-registration as a device row is deferred to the key-pinning phase. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a device registry end-to-end: backend device/instance storage and API, macOS client publishing/upsert logic, iOS fetch/parsing service, reconnect background refresh, route-selection policies, identity persistence, and tests exercising parsing, dedupe, authorization, and migration-backed DB behavior. ChangesDevice Registry: Complete Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4efa7c20cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // routes) reconnects immediately with no network round-trip. If the Mac | ||
| // moved networks / changed port, the refreshed routes land in the store | ||
| // and the next reconnect trigger (network change or Retry) uses them. | ||
| refreshRoutesFromRegistry(for: mac, stackUserID: stackUserID) |
There was a problem hiding this comment.
When the locally stored route is stale but /api/devices has the fresh address (the Mac moved networks or rebound to a new port), this starts the registry lookup in the background and then immediately reconnects with mac.routes below. The background task only upserts the store and reloads the paired-Mac list; it does not retry the in-flight launch reconnect, so the reload still fails until the user presses Retry or a network-change trigger happens, which misses the auto-pair-on-reload case this registry path is meant to rescue.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryIntroduces a team-scoped device registry (server schema +
Confidence Score: 5/5Safe to merge; the registry is deliberately best-effort and all critical write-back paths have correct lifecycle guards. The ownership model (per-team advisory lock, POST/DELETE scoped to the registering user) is solid. The iOS background refresh guards against sign-out, account switch, and active-Mac changes before writing. All writes are idempotent. The two findings are minor: a misleading doc comment about UserDefaults surviving reinstall, and a fire-and-forget Task that can't be cancelled on rapid reconnects. Neither affects correctness or security. DeviceRegistryService.swift (doc comment about UserDefaults and reinstall) and MobileShellComposite.swift (untracked refresh Task handle). Important Files Changed
Sequence DiagramsequenceDiagram
participant Mac as Mac (DeviceRegistryClient)
participant API as /api/devices
participant DB as Postgres
participant Phone as iOS (DeviceRegistryService)
participant Store as MobilePairedMacStore
Note over Mac: Route set changes (network move / port change)
Mac->>API: "POST /api/devices {deviceId, tag, routes}"
API->>DB: advisory lock(teamId)
API->>DB: SELECT existing device row
DB-->>API: existing row (or null)
alt Different user owns row
API-->>Mac: 403 device_not_owned
else Cap exceeded
API-->>Mac: 429 too_many_devices / too_many_instances
else OK
API->>DB: INSERT ON CONFLICT DO UPDATE (routes, lastSeenAt)
DB-->>API: deviceRowId
API-->>Mac: "200 {ok, deviceId, teamId, tag}"
Mac->>Mac: "lastRegistration = current scope"
end
Note over Phone: App reload / reconnect triggered
Phone->>Store: read activeMac (local routes)
Phone->>Phone: connect immediately on local routes
Phone-->>Phone: refreshRoutesFromRegistry (detached)
Phone->>API: GET /api/devices (Bearer + teamId header)
API->>DB: SELECT devices + device_app_instances WHERE teamId
DB-->>API: device rows + instance rows
API-->>Phone: "{teamId, devices:[{deviceId, instances:[{tag, routes}]}]}"
Phone->>Phone: selectReconnectRoutes(local, registry)
alt Registry has fresher routes AND lifecycle guards pass
Phone->>Store: upsert(macDeviceID, routes, markActive:true)
Phone->>Phone: loadPairedMacs()
Note over Phone: Next reconnect uses updated routes
else Registry empty / unavailable / same routes
Note over Phone: Local routes unchanged (fallback)
end
Reviews (8): Last reviewed commit: "Resolve registry team after auth bootstr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+176
to
+187
| .onConflictDoUpdate({ | ||
| target: devices.id, | ||
| set: { | ||
| teamId: team.teamId, | ||
| userId: user.id, | ||
| platform, | ||
| displayName, | ||
| labels, | ||
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
The advisory lock key is derived from team.teamId, so two transactions registering the same deviceId under different teams acquire different locks and can run concurrently. If teams A and B both race to register the same (freshly-minted) UUID before either transaction commits, both read existing = null, both pass the conflict check, and team A inserts first — then team B's ON CONFLICT DO UPDATE SET teamId = teamB silently reassigns ownership.
The fix is to exclude teamId from the conflict update set (it should never change once the row is created), relying on the pre-insert conflict check to enforce cross-team exclusivity. With teamId locked out of the update, the second team's INSERT cannot overwrite the winning owner.
Comment on lines
+109
to
+114
| } else { | ||
| NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode) | ||
| } | ||
| } | ||
| } catch { | ||
| // best-effort; registry must never disrupt the Mac. |
Contributor
There was a problem hiding this comment.
NSLog bypasses the unified logging system. The rest of cmux uses os.Logger — this failure path should too. Add import os at the top of the file and declare a private let deviceRegistryLog = Logger(subsystem: "...", category: "DeviceRegistry") to match the iOS client's pattern.
Suggested change
| } else { | |
| NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode) | |
| } | |
| } | |
| } catch { | |
| // best-effort; registry must never disrupt the Mac. | |
| } else { | |
| deviceRegistryLog.error("register failed status=\(http.statusCode, privacy: .public)") | |
| } | |
| } | |
| } catch { | |
| // best-effort; registry must never disrupt the Mac. |
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileDeviceIdentity.swift`:
- Around line 19-26: The deviceID(defaults:) function currently does a
non-atomic check-then-set and accepts any non-empty string; make it atomic and
self-healing by serializing the check-and-write and validating stored values as
real UUIDs: create a private serial queue or use objc_sync_enter/exit around the
body of deviceID(defaults:) (reference deviceID(defaults:) and deviceIDKey) so
concurrent callers cannot race, read the stored string, validate it with
UUID(uuidString:) (and normalize with .lowercased()), and if missing or invalid
generate a new UUID().uuidString.lowercased(), persist it to defaults under
deviceIDKey, and return that value. Ensure you always overwrite invalid values
instead of returning them.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1017-1023: The background refresh should not re-activate an old
Mac; change the pairedMacStore.upsert call (the one passing macDeviceID and
markActive: true) so it does not force-mark active during a background refresh.
Instead either pass markActive: false or first query the current active device
via pairedMacStore (e.g., getActiveMacDeviceID/currentActive) and only set
markActive: true when macDeviceID equals that current active or when the
operation is an explicit user activation. Update the upsert invocation
surrounding macDeviceID/displayName/routes to use this conditional logic so
stale background tasks cannot flip the active row.
In `@web/app/api/devices/route.ts`:
- Around line 61-84: The readBoundedJson function currently calls request.text()
and checks raw.length (UTF-16 code units) after the fact; change it to stream
the request body and enforce MAX_REQUEST_BYTES before buffering: if
Content-Length exists and exceeds MAX_REQUEST_BYTES return 413 immediately,
otherwise use request.body/getReader() to read Uint8Array chunks, summing bytes
read (track totalBytes) and aborting with 413 as soon as totalBytes >
MAX_REQUEST_BYTES, accumulate chunks into a byte buffer, then decode once
(TextDecoder) and JSON.parse the decoded string; remove reliance on
request.text() and raw.length and keep existing error handling paths (return 400
on parse errors) while referencing readBoundedJson and MAX_REQUEST_BYTES.
- Around line 106-325: Extract the device registry workflow (auth, team
resolution, request parsing/validation, DB transaction and payload assembly, and
error-to-HTTP mapping) out of the HTTP handlers into a dedicated Effect service
with clear operations (e.g. registerDevice, listDevices, unregisterDevice). Move
the logic currently in the POST/GET/DELETE functions (including calls to
verifyRequest, resolveTeam, readBoundedJson, cloudDb, the transaction that
inserts into devices and deviceAppInstances, the device listing logic building
devicesPayload, and the delete call) into these Effect functions, have them
return typed domain errors (device_team_conflict, too_many_devices,
invalid_request, invalid_device_id, invalid_platform, etc.), and keep route.ts
handlers thin — they should only translate the incoming Request to the service
input, run the Effect, and map the service's typed errors to the same JSON HTTP
responses (status codes and shapes) you already use. Ensure unique symbols to
change: registerDevice (new Effect for POST logic), listDevices (new Effect for
GET logic), unregisterDevice (new Effect for DELETE logic); keep
verifyRequest/resolveTeam/readBoundedJson usage inside the service or pass
already-validated inputs from the route if you prefer; preserve existing
behavior and response shapes when mapping errors.
In `@web/db/schema.ts`:
- Around line 228-252: The device_app_instances table duplicates teamId without
enforcing it matches the parent devices row; update the schema for
deviceAppInstances to enforce a composite foreign key (deviceId, teamId)
referencing devices (id, team_id) so mismatched child rows cannot be inserted,
or alternatively remove the teamId column and update callers (e.g.,
web/app/api/devices/route.ts) to join through devices for team scoping; locate
the deviceAppInstances pgTable definition and either add a composite FK
constraint on (deviceId, teamId) -> (devices.id, devices.team_id) or drop
deviceAppInstances.teamId and fix reads to derive team from devices.
In `@web/tests/devices-route.test.ts`:
- Around line 68-176: Add two dbTest cases that exercise the 409 and 429
branches: (1) a test that seeds the DB with an existing device row owned by a
different team (use sql to insert into devices/device_app_instances for DEVICE_A
with team_id = "team-not-mine"), then call POST(registerRequest({...},
"team-a")) and assert status 409 to hit device_team_conflict; (2) a test that
seeds the DB to push the team over the device limit (use sql to insert many
devices for "team-a" or manipulate the device count threshold) then call
POST(registerRequest({ deviceId: NEW_DEVICE, ... })) and assert status 429 to
exercise too_many_devices/advisory-lock path; use the existing helpers
(registerRequest, POST, authHeaders, sql) and model the assertions after the
existing tests.
🪄 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: fefb1eec-ea41-4f4f-9208-d1650c7852d7
📒 Files selected for processing (15)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryRefreshing.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileDeviceIdentity.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceRegistryRouteSelectionTests.swiftSources/AppDelegate.swiftSources/Cloud/DeviceRegistryClient.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DeviceRegistryClientTests.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftweb/app/api/devices/route.tsweb/db/migrations/20260608044827_device_registry/migration.sqlweb/db/migrations/20260608044827_device_registry/snapshot.jsonweb/db/schema.tsweb/tests/devices-route.test.ts
Comment on lines
+19
to
+26
| public static func deviceID(defaults: UserDefaults = .standard) -> String { | ||
| if let existing = defaults.string(forKey: deviceIDKey), | ||
| !existing.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return existing | ||
| } | ||
| let generated = UUID().uuidString.lowercased() | ||
| defaults.set(generated, forKey: deviceIDKey) | ||
| return generated |
There was a problem hiding this comment.
Make device ID initialization atomic and self-healing.
Line 20 through Line 26 performs a non-atomic check-then-set, so concurrent first lookups can return different UUIDs. It also accepts any non-empty persisted value, which can propagate invalid IDs across the registry boundary.
💡 Suggested fix
public enum MobileDeviceIdentity {
private static let deviceIDKey = "cmux.deviceRegistry.iosDeviceID"
+ private static let lock = NSLock()
/// The persisted device UUID, generating and storing one on first use.
/// - Parameter defaults: Persistence store (injected for tests).
public static func deviceID(defaults: UserDefaults = .standard) -> String {
- if let existing = defaults.string(forKey: deviceIDKey),
- !existing.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
- return existing
+ lock.lock()
+ defer { lock.unlock() }
+
+ if let raw = defaults.string(forKey: deviceIDKey) {
+ let trimmed = raw.trimmingCharacters(in: .whitespacesAndNewlines)
+ if let uuid = UUID(uuidString: trimmed) {
+ let canonical = uuid.uuidString.lowercased()
+ if canonical != raw {
+ defaults.set(canonical, forKey: deviceIDKey)
+ }
+ return canonical
+ }
}
+
let generated = UUID().uuidString.lowercased()
defaults.set(generated, forKey: deviceIDKey)
return generated
}
}📝 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.
Suggested change
| public static func deviceID(defaults: UserDefaults = .standard) -> String { | |
| if let existing = defaults.string(forKey: deviceIDKey), | |
| !existing.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | |
| return existing | |
| } | |
| let generated = UUID().uuidString.lowercased() | |
| defaults.set(generated, forKey: deviceIDKey) | |
| return generated | |
| public enum MobileDeviceIdentity { | |
| private static let deviceIDKey = "cmux.deviceRegistry.iosDeviceID" | |
| // NSLock exception: UserDefaults is shared mutable state accessed concurrently on first-use. | |
| // Lock ensures atomic check-then-generate-and-persist to prevent duplicate device IDs. | |
| private static let lock = NSLock() | |
| /// The persisted device UUID, generating and storing one on first use. | |
| /// - Parameter defaults: Persistence store (injected for tests). | |
| public static func deviceID(defaults: UserDefaults = .standard) -> String { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| if let raw = defaults.string(forKey: deviceIDKey) { | |
| let trimmed = raw.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if let uuid = UUID(uuidString: trimmed) { | |
| let canonical = uuid.uuidString.lowercased() | |
| if canonical != raw { | |
| defaults.set(canonical, forKey: deviceIDKey) | |
| } | |
| return canonical | |
| } | |
| } | |
| let generated = UUID().uuidString.lowercased() | |
| defaults.set(generated, forKey: deviceIDKey) | |
| return generated | |
| } | |
| } |
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileDeviceIdentity.swift`
around lines 19 - 26, The deviceID(defaults:) function currently does a
non-atomic check-then-set and accepts any non-empty string; make it atomic and
self-healing by serializing the check-and-write and validating stored values as
real UUIDs: create a private serial queue or use objc_sync_enter/exit around the
body of deviceID(defaults:) (reference deviceID(defaults:) and deviceIDKey) so
concurrent callers cannot race, read the stored string, validate it with
UUID(uuidString:) (and normalize with .lowercased()), and if missing or invalid
generate a new UUID().uuidString.lowercased(), persist it to defaults under
deviceIDKey, and return that value. Ensure you always overwrite invalid values
instead of returning them.
Comment on lines
+61
to
+84
| async function readBoundedJson( | ||
| request: Request, | ||
| ): Promise<{ ok: true; value: Record<string, unknown> } | { ok: false; status: number }> { | ||
| const lengthHeader = request.headers.get("content-length"); | ||
| if (lengthHeader && Number(lengthHeader) > MAX_REQUEST_BYTES) { | ||
| return { ok: false, status: 413 }; | ||
| } | ||
| let raw: string; | ||
| try { | ||
| raw = await request.text(); | ||
| } catch { | ||
| return { ok: false, status: 400 }; | ||
| } | ||
| if (raw.length > MAX_REQUEST_BYTES) return { ok: false, status: 413 }; | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(raw); | ||
| } catch { | ||
| return { ok: false, status: 400 }; | ||
| } | ||
| if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { | ||
| return { ok: false, status: 400 }; | ||
| } | ||
| return { ok: true, value: parsed as Record<string, unknown> }; |
There was a problem hiding this comment.
Enforce the body limit before buffering the whole request.
Line 70 reads the entire body into memory before rejecting it, and Line 74 compares UTF-16 code units instead of bytes. If Content-Length is missing or inaccurate, an authenticated caller can still push a payload well above MAX_REQUEST_BYTES before this path fails.
Proposed fix
async function readBoundedJson(
request: Request,
): Promise<{ ok: true; value: Record<string, unknown> } | { ok: false; status: number }> {
const lengthHeader = request.headers.get("content-length");
if (lengthHeader && Number(lengthHeader) > MAX_REQUEST_BYTES) {
return { ok: false, status: 413 };
}
- let raw: string;
+ const reader = request.body?.getReader();
+ if (!reader) {
+ return { ok: false, status: 400 };
+ }
+ const decoder = new TextDecoder();
+ let bytes = 0;
+ let raw = "";
try {
- raw = await request.text();
+ while (true) {
+ const { done, value } = await reader.read();
+ if (done) break;
+ bytes += value.byteLength;
+ if (bytes > MAX_REQUEST_BYTES) {
+ return { ok: false, status: 413 };
+ }
+ raw += decoder.decode(value, { stream: true });
+ }
+ raw += decoder.decode();
} catch {
return { ok: false, status: 400 };
}
- if (raw.length > MAX_REQUEST_BYTES) return { ok: false, status: 413 };
let parsed: unknown;🤖 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/app/api/devices/route.ts` around lines 61 - 84, The readBoundedJson
function currently calls request.text() and checks raw.length (UTF-16 code
units) after the fact; change it to stream the request body and enforce
MAX_REQUEST_BYTES before buffering: if Content-Length exists and exceeds
MAX_REQUEST_BYTES return 413 immediately, otherwise use request.body/getReader()
to read Uint8Array chunks, summing bytes read (track totalBytes) and aborting
with 413 as soon as totalBytes > MAX_REQUEST_BYTES, accumulate chunks into a
byte buffer, then decode once (TextDecoder) and JSON.parse the decoded string;
remove reliance on request.text() and raw.length and keep existing error
handling paths (return 400 on parse errors) while referencing readBoundedJson
and MAX_REQUEST_BYTES.
Comment on lines
+106
to
+325
| export async function POST(request: Request): Promise<Response> { | ||
| const user = await verifyRequest(request, { | ||
| requestedTeamId: requestedVmTeamIdFromRequest(request), | ||
| allowCookie: false, | ||
| }); | ||
| if (!user) return unauthorized(); | ||
|
|
||
| const team = resolveTeam(request, user); | ||
| if (!team.ok) return team.response; | ||
|
|
||
| const body = await readBoundedJson(request); | ||
| if (!body.ok) return jsonResponse({ error: "invalid_request" }, body.status); | ||
|
|
||
| const deviceId = trimmedString(body.value.deviceId).toLowerCase(); | ||
| const platform = trimmedString(body.value.platform).toLowerCase(); | ||
| const displayName = trimmedString(body.value.displayName) || null; | ||
| const labels = recordOrEmpty(body.value.labels); | ||
| const tag = trimmedString(body.value.tag) || "default"; | ||
| const routes = routesArray(body.value.routes); | ||
| const instanceLabels = recordOrEmpty(body.value.instanceLabels); | ||
|
|
||
| if (!UUID_RE.test(deviceId)) { | ||
| return jsonResponse({ error: "invalid_device_id" }, 400); | ||
| } | ||
| if (!ALLOWED_PLATFORMS.has(platform)) { | ||
| return jsonResponse({ error: "invalid_platform" }, 400); | ||
| } | ||
|
|
||
| const db = cloudDb(); | ||
| const now = new Date(); | ||
|
|
||
| const registered = await db.transaction(async (tx) => { | ||
| // Serialize concurrent registrations for the same team so the per-team cap | ||
| // is enforced without a race (mirrors the device-tokens advisory lock). | ||
| await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${team.teamId}, 7))`); | ||
|
|
||
| const [existing] = await tx | ||
| .select({ id: devices.id, teamId: devices.teamId }) | ||
| .from(devices) | ||
| .where(eq(devices.id, deviceId)) | ||
| .limit(1); | ||
|
|
||
| // A device id is global (cmux-generated UUID). If it already exists under a | ||
| // different team, the caller cannot claim it (prevents cross-team takeover). | ||
| if (existing && existing.teamId !== team.teamId) { | ||
| return { error: "device_team_conflict" as const }; | ||
| } | ||
|
|
||
| if (!existing) { | ||
| const [{ total }] = await tx | ||
| .select({ total: sql<number>`count(*)::int` }) | ||
| .from(devices) | ||
| .where(eq(devices.teamId, team.teamId)); | ||
| if (Number(total) >= MAX_DEVICES_PER_TEAM) { | ||
| return { error: "too_many_devices" as const }; | ||
| } | ||
| } | ||
|
|
||
| await tx | ||
| .insert(devices) | ||
| .values({ | ||
| id: deviceId, | ||
| teamId: team.teamId, | ||
| userId: user.id, | ||
| platform, | ||
| displayName, | ||
| labels, | ||
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: devices.id, | ||
| set: { | ||
| teamId: team.teamId, | ||
| userId: user.id, | ||
| platform, | ||
| displayName, | ||
| labels, | ||
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }, | ||
| }); | ||
|
|
||
| await tx | ||
| .insert(deviceAppInstances) | ||
| .values({ | ||
| deviceId, | ||
| teamId: team.teamId, | ||
| tag, | ||
| routes, | ||
| labels: instanceLabels, | ||
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [deviceAppInstances.deviceId, deviceAppInstances.tag], | ||
| set: { | ||
| teamId: team.teamId, | ||
| routes, | ||
| labels: instanceLabels, | ||
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }, | ||
| }); | ||
|
|
||
| return { error: null }; | ||
| }); | ||
|
|
||
| if (registered.error === "device_team_conflict") { | ||
| return jsonResponse({ error: "device_team_conflict" }, 409); | ||
| } | ||
| if (registered.error === "too_many_devices") { | ||
| return jsonResponse({ error: "too_many_devices" }, 429); | ||
| } | ||
|
|
||
| return jsonResponse({ ok: true, deviceId, teamId: team.teamId, tag }); | ||
| } | ||
|
|
||
| type DeviceListRow = { | ||
| id: string; | ||
| platform: string; | ||
| displayName: string | null; | ||
| labels: Record<string, unknown>; | ||
| lastSeenAt: Date; | ||
| }; | ||
|
|
||
| /** | ||
| * List the team's registered devices and their app instances, so a phone can | ||
| * find the Mac it last paired with and refresh routes on reload. | ||
| */ | ||
| export async function GET(request: Request): Promise<Response> { | ||
| const user = await verifyRequest(request, { | ||
| requestedTeamId: requestedVmTeamIdFromRequest(request), | ||
| allowCookie: false, | ||
| }); | ||
| if (!user) return unauthorized(); | ||
|
|
||
| const team = resolveTeam(request, user); | ||
| if (!team.ok) return team.response; | ||
|
|
||
| const db = cloudDb(); | ||
|
|
||
| const deviceRows = (await db | ||
| .select({ | ||
| id: devices.id, | ||
| platform: devices.platform, | ||
| displayName: devices.displayName, | ||
| labels: devices.labels, | ||
| lastSeenAt: devices.lastSeenAt, | ||
| }) | ||
| .from(devices) | ||
| .where(eq(devices.teamId, team.teamId)) | ||
| .orderBy(desc(devices.lastSeenAt))) as DeviceListRow[]; | ||
|
|
||
| const instanceRows = await db | ||
| .select({ | ||
| deviceId: deviceAppInstances.deviceId, | ||
| tag: deviceAppInstances.tag, | ||
| routes: deviceAppInstances.routes, | ||
| labels: deviceAppInstances.labels, | ||
| lastSeenAt: deviceAppInstances.lastSeenAt, | ||
| }) | ||
| .from(deviceAppInstances) | ||
| .where(eq(deviceAppInstances.teamId, team.teamId)) | ||
| .orderBy(desc(deviceAppInstances.lastSeenAt)); | ||
|
|
||
| const instancesByDevice = new Map<string, typeof instanceRows>(); | ||
| for (const row of instanceRows) { | ||
| const list = instancesByDevice.get(row.deviceId) ?? []; | ||
| list.push(row); | ||
| instancesByDevice.set(row.deviceId, list); | ||
| } | ||
|
|
||
| const devicesPayload = deviceRows.map((device) => ({ | ||
| deviceId: device.id, | ||
| platform: device.platform, | ||
| displayName: device.displayName, | ||
| labels: device.labels, | ||
| lastSeenAt: device.lastSeenAt.toISOString(), | ||
| instances: (instancesByDevice.get(device.id) ?? []).map((instance) => ({ | ||
| tag: instance.tag, | ||
| routes: instance.routes, | ||
| labels: instance.labels, | ||
| lastSeenAt: instance.lastSeenAt.toISOString(), | ||
| })), | ||
| })); | ||
|
|
||
| return jsonResponse({ teamId: team.teamId, devices: devicesPayload }); | ||
| } | ||
|
|
||
| /** | ||
| * Unregister a device (e.g. when the user forgets/decommissions a Mac). Removes | ||
| * the machine row and cascades its app instances. Team-scoped so a caller can | ||
| * only delete devices in a team they belong to. | ||
| */ | ||
| export async function DELETE(request: Request): Promise<Response> { | ||
| const user = await verifyRequest(request, { | ||
| requestedTeamId: requestedVmTeamIdFromRequest(request), | ||
| allowCookie: false, | ||
| }); | ||
| if (!user) return unauthorized(); | ||
|
|
||
| const team = resolveTeam(request, user); | ||
| if (!team.ok) return team.response; | ||
|
|
||
| const body = await readBoundedJson(request); | ||
| if (!body.ok) return jsonResponse({ error: "invalid_request" }, body.status); | ||
|
|
||
| const deviceId = trimmedString(body.value.deviceId).toLowerCase(); | ||
| if (!UUID_RE.test(deviceId)) { | ||
| return jsonResponse({ error: "invalid_device_id" }, 400); | ||
| } | ||
|
|
||
| const db = cloudDb(); | ||
| await db | ||
| .delete(devices) | ||
| .where(and(eq(devices.id, deviceId), eq(devices.teamId, team.teamId))); | ||
|
|
||
| return jsonResponse({ ok: true }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the file with context around the shown lines
sed -n '1,240p' web/app/api/devices/route.ts
echo "----"
sed -n '240,420p' web/app/api/devices/route.ts
echo "---- Searching for Effect usage in this file ----"
rg -n "Effect\.|from 'effect'|`@effect`|Effect" web/app/api/devices/route.ts || true
echo "---- Searching for existing devices service modules ----"
fd -t f "devices.*\.(ts|tsx)" web | head -n 50
rg -n "devices" web/services web/app/api | head -n 200Repository: manaflow-ai/cmux
Length of output: 13343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Searching for Effect usage in web/app/api and web/services ----"
rg -n "from ['\"]effect['\"]|Effect\." web/app/api web/services || true
echo "---- Inspecting web/app/api/device-tokens/route.ts (closest sibling) ----"
sed -n '1,240p' web/app/api/device-tokens/route.ts
echo "---- Inspecting web/app/api/notifications/push/route.ts (calls forwarding) ----"
sed -n '1,260p' web/app/api/notifications/push/route.ts
echo "---- Looking for an existing Effect service pattern under web/services ----"
fd -t f web/services | head -n 200
rg -n "runEffect|mapError|Effect\.gen|Effect\.runPromise|tryCatch|typed errors" web/services || trueRepository: manaflow-ai/cmux
Length of output: 19721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Listing web/services files (top-level, limited) ----"
fd . web/services -t f | head -n 200
echo "---- Find any devices-registry or deviceAppInstances usage outside the route ----"
rg -n "deviceAppInstances|MAX_DEVICES_PER_TEAM|routesArray\\(|invalid_device_id" web --glob='*.ts' || true
echo "---- Inspect routeHandler / effect boundary patterns ----"
rg -n "with[A-Za-z]*ApiRoute|routeHandler|Effect\\.runPromise|mapError|typed errors" web/services web/app/api --glob='*.ts' || true
# Print the routeHandler implementation if it exists
fd -t f "routeHandler*.*" web/services | head -n 20 | cat
fd -t f "routeHandler*.*" web/services | head -n 1 | xargs -I{} sh -lc 'echo "---- {} ----"; sed -n "1,220p" "{}"'Repository: manaflow-ai/cmux
Length of output: 6223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Inspecting route helper used by vm routes ----"
sed -n '1,220p' web/services/vms/routeHelpers.ts
echo "---- Inspecting a vm API route that calls a service/workflow ----"
sed -n '1,140p' web/app/api/vm/route.ts
echo "---- Inspecting vm/[id]/attach-endpoint route (example boundary) ----"
sed -n '1,220p' web/app/api/vm/[id]/attach-endpoint/route.ts
echo "---- Searching for Effect.runPromise usage in web/app/api routes ----"
rg -n "Effect\.runPromise|Effect\.run" web/app/api --glob='*.ts' || true
echo "---- Inspecting vms/workflows error/typed-mapping pattern ----"
sed -n '1,220p' web/services/vms/workflows.tsRepository: manaflow-ai/cmux
Length of output: 22942
Extract the device registry workflow from web/app/api/devices/route.ts into an Effect service
POST/GET/DELETE currently inline auth+team resolution, request validation, the DB transaction/building payloads, and HTTP error mapping—so route.ts becomes the workflow boundary. Repo guidance for {web/app/api/**, web/routes/**}/*.ts is to keep handlers thin and run a single backend Effect program at the boundary, mapping typed errors to HTTP responses and treating unexpected defects separately.
🤖 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/app/api/devices/route.ts` around lines 106 - 325, Extract the device
registry workflow (auth, team resolution, request parsing/validation, DB
transaction and payload assembly, and error-to-HTTP mapping) out of the HTTP
handlers into a dedicated Effect service with clear operations (e.g.
registerDevice, listDevices, unregisterDevice). Move the logic currently in the
POST/GET/DELETE functions (including calls to verifyRequest, resolveTeam,
readBoundedJson, cloudDb, the transaction that inserts into devices and
deviceAppInstances, the device listing logic building devicesPayload, and the
delete call) into these Effect functions, have them return typed domain errors
(device_team_conflict, too_many_devices, invalid_request, invalid_device_id,
invalid_platform, etc.), and keep route.ts handlers thin — they should only
translate the incoming Request to the service input, run the Effect, and map the
service's typed errors to the same JSON HTTP responses (status codes and shapes)
you already use. Ensure unique symbols to change: registerDevice (new Effect for
POST logic), listDevices (new Effect for GET logic), unregisterDevice (new
Effect for DELETE logic); keep verifyRequest/resolveTeam/readBoundedJson usage
inside the service or pass already-validated inputs from the route if you
prefer; preserve existing behavior and response shapes when mapping errors.
Sources: Coding guidelines, Learnings
Comment on lines
+68
to
+176
| describe("device registry route", () => { | ||
| dbTest("registers a Mac and its app instance, then lists it for the team", async () => { | ||
| if (!sql) throw new Error("test database not initialized"); | ||
|
|
||
| const register = await POST( | ||
| registerRequest({ | ||
| deviceId: DEVICE_A, | ||
| platform: "mac", | ||
| displayName: "Lawrence's Mac", | ||
| tag: "stable", | ||
| routes: [{ id: "r1", kind: "tailscale", priority: 0, endpoint: { host: "100.1.2.3", port: 51001 } }], | ||
| }), | ||
| ); | ||
| expect(register.status).toBe(200); | ||
|
|
||
| const listResponse = await GET( | ||
| new Request("https://cmux.test/api/devices", { method: "GET", headers: authHeaders() }), | ||
| ); | ||
| expect(listResponse.status).toBe(200); | ||
| const list = (await listResponse.json()) as { | ||
| teamId: string; | ||
| devices: Array<{ | ||
| deviceId: string; | ||
| displayName: string | null; | ||
| platform: string; | ||
| instances: Array<{ tag: string; routes: unknown[] }>; | ||
| }>; | ||
| }; | ||
| expect(list.teamId).toBe("team-a"); | ||
| expect(list.devices).toHaveLength(1); | ||
| expect(list.devices[0].deviceId).toBe(DEVICE_A); | ||
| expect(list.devices[0].displayName).toBe("Lawrence's Mac"); | ||
| expect(list.devices[0].instances).toHaveLength(1); | ||
| expect(list.devices[0].instances[0].tag).toBe("stable"); | ||
| expect(list.devices[0].instances[0].routes).toHaveLength(1); | ||
| }); | ||
|
|
||
| dbTest("re-registering the same (device, tag) refreshes routes in place (auto-pair path)", async () => { | ||
| if (!sql) throw new Error("test database not initialized"); | ||
|
|
||
| await POST( | ||
| registerRequest({ | ||
| deviceId: DEVICE_A, | ||
| platform: "mac", | ||
| tag: "stable", | ||
| routes: [{ id: "old", kind: "tailscale", priority: 0, endpoint: { host: "100.0.0.1", port: 1 } }], | ||
| }), | ||
| ); | ||
| // Mac moved networks / restarted on a new port: re-register with fresh routes. | ||
| await POST( | ||
| registerRequest({ | ||
| deviceId: DEVICE_A, | ||
| platform: "mac", | ||
| tag: "stable", | ||
| routes: [{ id: "new", kind: "tailscale", priority: 0, endpoint: { host: "100.9.9.9", port: 51999 } }], | ||
| }), | ||
| ); | ||
|
|
||
| const [{ total }] = await sql<{ total: number }[]>` | ||
| select count(*)::int as total from device_app_instances where device_id = ${DEVICE_A} | ||
| `; | ||
| expect(total).toBe(1); | ||
|
|
||
| const list = (await ( | ||
| await GET(new Request("https://cmux.test/api/devices", { method: "GET", headers: authHeaders() })) | ||
| ).json()) as { devices: Array<{ instances: Array<{ routes: Array<{ endpoint: { host: string } }> }> }> }; | ||
| expect(list.devices[0].instances[0].routes[0].endpoint.host).toBe("100.9.9.9"); | ||
| }); | ||
|
|
||
| dbTest("rejects a team the caller is not a member of", async () => { | ||
| if (!sql) throw new Error("test database not initialized"); | ||
|
|
||
| const response = await POST( | ||
| registerRequest( | ||
| { deviceId: DEVICE_A, platform: "mac", routes: [] }, | ||
| "team-not-mine", | ||
| ), | ||
| ); | ||
| expect(response.status).toBe(403); | ||
|
|
||
| const [{ total }] = await sql<{ total: number }[]>`select count(*)::int as total from devices`; | ||
| expect(total).toBe(0); | ||
| }); | ||
|
|
||
| dbTest("delete removes the device and cascades its instances", async () => { | ||
| if (!sql) throw new Error("test database not initialized"); | ||
|
|
||
| await POST(registerRequest({ deviceId: DEVICE_A, platform: "mac", tag: "stable", routes: [] })); | ||
| await POST(registerRequest({ deviceId: DEVICE_B, platform: "mac", tag: "stable", routes: [] })); | ||
|
|
||
| const del = await DELETE( | ||
| new Request("https://cmux.test/api/devices", { | ||
| method: "DELETE", | ||
| headers: authHeaders(), | ||
| body: JSON.stringify({ deviceId: DEVICE_A }), | ||
| }), | ||
| ); | ||
| expect(del.status).toBe(200); | ||
|
|
||
| const [{ devicesTotal }] = await sql<{ devicesTotal: number }[]>` | ||
| select count(*)::int as "devicesTotal" from devices | ||
| `; | ||
| expect(devicesTotal).toBe(1); | ||
| const [{ instancesTotal }] = await sql<{ instancesTotal: number }[]>` | ||
| select count(*)::int as "instancesTotal" from device_app_instances where device_id = ${DEVICE_A} | ||
| `; | ||
| expect(instancesTotal).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add coverage for the two POST safety branches that matter most here.
This suite never exercises device_team_conflict or too_many_devices, so the cross-team takeover guard and the advisory-lock/cap path can regress without a failing test. Please add DB-backed cases for the 409 and 429 branches.
🤖 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/tests/devices-route.test.ts` around lines 68 - 176, Add two dbTest cases
that exercise the 409 and 429 branches: (1) a test that seeds the DB with an
existing device row owned by a different team (use sql to insert into
devices/device_app_instances for DEVICE_A with team_id = "team-not-mine"), then
call POST(registerRequest({...}, "team-a")) and assert status 409 to hit
device_team_conflict; (2) a test that seeds the DB to push the team over the
device limit (use sql to insert many devices for "team-a" or manipulate the
device count threshold) then call POST(registerRequest({ deviceId: NEW_DEVICE,
... })) and assert status 429 to exercise too_many_devices/advisory-lock path;
use the existing helpers (registerRequest, POST, authHeaders, sql) and model the
assertions after the existing tests.
…lish Addresses autoreview findings on the new registry path: - Cap app instances per device (MAX_INSTANCES_PER_DEVICE) in the same advisory- lock transaction, mirroring the per-team device cap. `tag` is client-supplied and the instance key is (deviceId, tag), so without this one device could create unbounded rows by varying the tag. Re-registering an existing tag stays an update. Also bound tag length. - Server stores only structurally valid route entries (plain objects, bounded by MAX_ROUTES); scalars/arrays are dropped. Semantic CmxAttachRoute validation stays with the typed clients so the server is forward-compatible with new route kinds. - iOS parser decodes each route failably and per-element, so one malformed or unknown-kind route from any instance (even another Mac's) is skipped instead of nil-ing the whole /api/devices response and disabling refresh for every Mac. - Mac DeviceRegistryClient.shouldReRegister now fires once on the nonempty->empty transition (pairing turned off), publishing the empty route set so the registry stops advertising stale routes; the phone already skips empty-route instances. Initial-empty and repeated-empty ticks stay no-ops. Tests: web instance-cap + route-filtering cases; iOS malformed-sibling and malformed-within-target skip cases; Mac clear-publishes-once and still-empty no-op cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
So the device registry route's behavioral coverage (register/list, team-scope 403, instance cap, route filtering, delete cascade) gates in CI, not just locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| let routes = instance.routes.compactMap(\.value) | ||
| if !routes.isEmpty { return routes } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Stale routes from older tags
Medium Severity
When resolving registry routes for a Mac, the client walks instances in recency order and returns the first with any decodable routes. If the newest instance has empty routes (e.g. pairing turned off) but an older (deviceId, tag) row still has routes, those stale endpoints are returned and can overwrite local routes via selectReconnectRoutes, undermining the Mac’s empty-route publish.
Reviewed by Cursor Bugbot for commit 90fc5e4. Configure here.
| lastSeenAt: now, | ||
| updatedAt: now, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Cross-team device registration race
Medium Severity
POST serializes registrations with a per-teamId advisory lock, but device ownership is keyed globally by deviceId. Concurrent first-time registrations for the same deviceId from two teams can both miss an existing row and the onConflictDoUpdate path can overwrite team_id, moving a device between teams without returning device_team_conflict.
Reviewed by Cursor Bugbot for commit 90fc5e4. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a86cb466a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+195
to
+198
| .onConflictDoUpdate({ | ||
| target: devices.id, | ||
| set: { | ||
| teamId: team.teamId, |
There was a problem hiding this comment.
When two different teams register the same deviceId concurrently, the per-team advisory lock does not serialize those transactions: both can observe no existing row, then the loser of the insert race hits this conflict handler and updates teamId to its own team. That bypasses the cross-team takeover check above and can move an already-created device row between teams; the conflict path needs to avoid changing ownership or re-check the existing team_id atomically.
Useful? React with 👍 / 👎.
Comment on lines
+1013
to
+1030
| local: localRoutes, | ||
| registry: registryRoutes | ||
| ) else { return } | ||
| do { | ||
| try await pairedMacStore.upsert( | ||
| macDeviceID: macDeviceID, | ||
| displayName: displayName, | ||
| routes: updated, | ||
| markActive: true, | ||
| stackUserID: stackUserID | ||
| ) | ||
| } catch { | ||
| mobileShellLog.debug("registry route refresh upsert failed: \(String(describing: error), privacy: .public)") | ||
| return | ||
| } | ||
| await self?.loadPairedMacs() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
refreshRoutesFromRegistry runs in a detached Task and calls pairedMacStore.upsert(markActive: true, ...). markActive: true deactivates every other Mac for the user (via UPDATE paired_macs SET is_active = 0 WHERE stack_user_id IS ?) and then sets this Mac as active. If the user switches to a different Mac between when the reconnect starts and when the network response lands (e.g., a slow registry round-trip), the background upsert silently reverts the switch: the user's newly selected Mac is deactivated and the original Mac is re-activated.
The goal of this path is route freshness only — markActive: false updates the stored routes without touching the active-Mac selection, which is the correct invariant here.
…n on iOS Addresses round-2 autoreview lifecycle findings: - Mac DeviceRegistryClient deduped on routes alone, so an account/team switch with unchanged routes never registered the Mac in the newly selected team. Dedup now keys on a (teamID, tag, routes) scope, resolved before the skip decision, so a team switch re-registers even when routes are identical. Leaving the old team's row behind is acceptable by design (best-effort, stale-tolerant registry); registering in the new team is the fix. - iOS registry refresh ran a detached upsert(markActive: true) after the network await without re-checking lifecycle, so signing out, forgetting the Mac, or switching the active Mac while freshRoutes was in flight could resurrect or reactivate the removed pairing (and expose it to the next user on a shared device). It now re-reads the active Mac and applies a pure shouldApplyRegistryRefresh guard (still signed in, same user, same active Mac) before writing, mirroring the existing user-switch guard in loadPairedMacs. Tests: Mac team-switch-with-unchanged-routes fires; iOS shouldApplyRegistryRefresh sign-out / user-switch / forgotten / active-mac-switched all reject. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Cloud/DeviceRegistryClient.swift (1)
118-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Logger instead of NSLog for production diagnostics.
The project's Swift logging guidelines prohibit
NSLogin production app/runtime code. Useos.Loggerinstead for this diagnostic logging.📝 Suggested fix
Add a Logger constant at file scope:
import os private let deviceRegistryLog = Logger(subsystem: "com.cmuxterm.app", category: "DeviceRegistry")Then replace the NSLog call:
} else { - NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode) + deviceRegistryLog.debug("register failed status=\(http.statusCode, privacy: .public)") }As per coding guidelines
.github/review-bot-rules/swift-logging.md, production Swift code must use Apple's unified logging system (Logger) instead of NSLog. The exception for NSLog applies only to Sources/Providers/ fetcher files, not Sources/Cloud/.🤖 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/Cloud/DeviceRegistryClient.swift` at line 118, Replace the NSLog call with os.Logger: add `import os` and a file‑scope Logger constant (e.g. `private let deviceRegistryLog = Logger(subsystem: "com.cmuxterm.app", category: "DeviceRegistry")`) in DeviceRegistryClient.swift, then change the `NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode)` line to use `deviceRegistryLog.error` with string interpolation to include `http.statusCode` (e.g. a single error log call that includes the status code).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Cloud/DeviceRegistryClient.swift`:
- Line 118: Replace the NSLog call with os.Logger: add `import os` and a
file‑scope Logger constant (e.g. `private let deviceRegistryLog =
Logger(subsystem: "com.cmuxterm.app", category: "DeviceRegistry")`) in
DeviceRegistryClient.swift, then change the `NSLog("cmux.deviceRegistry register
failed status=%d", http.statusCode)` line to use `deviceRegistryLog.error` with
string interpolation to include `http.statusCode` (e.g. a single error log call
that includes the status code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 926226da-b044-4ea8-9747-108ccfebbec2
📒 Files selected for processing (7)
.github/workflows/ci.ymlPackages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceRegistryRouteSelectionTests.swiftSources/Cloud/DeviceRegistryClient.swiftcmuxTests/DeviceRegistryClientTests.swiftweb/app/api/devices/route.tsweb/tests/devices-route.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1030-1044:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis stale-refresh guard is still racy across the awaited write.
The lifecycle check happens before
try await pairedMacStore.upsert(...), sosignOut(),forgetMac(...), ordisconnectAndForgetActiveMac()can run while this task is suspended and the old refresh will still recreate/reactivate the row withmarkActive: true. This needs to be enforced inside the store mutation itself (for example, a conditional update keyed on the expected active Mac / user / generation), not as a preflight check on the caller side.🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1030 - 1044, The preflight lifecycle check using DeviceRegistryRouteSelection.shouldApplyRegistryRefresh is racy because the task can suspend before try await pairedMacStore.upsert(...), so move the guard into the mutation by making the store perform a conditional upsert: change or overload pairedMacStore.upsert to accept expectedActiveMacID/expectedStackUserID (or an expected generation token) and have the store apply the write only if those expectations still match the current row; if the condition fails, return a clear no-op/error so callers know the update was skipped instead of reactivating/stale-marking the MAC as active.
🤖 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 `@Sources/Cloud/DeviceRegistryClient.swift`:
- Around line 128-135: Replace the NSLog call in the HTTP response handling
block with the project’s unified Logger: locate the response handling inside
DeviceRegistryClient (the block that checks if let http = response as?
HTTPURLResponse and updates lastRegistration = registration) and replace
NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode) with a
Logger-based call (e.g., using a shared or instance Logger) that logs the same
message and http.statusCode; ensure you import OSLog/Logging if needed and use
the existing Logger instance or add one to the DeviceRegistryClient so logging
follows the repo’s unified logging path.
---
Duplicate comments:
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1030-1044: The preflight lifecycle check using
DeviceRegistryRouteSelection.shouldApplyRegistryRefresh is racy because the task
can suspend before try await pairedMacStore.upsert(...), so move the guard into
the mutation by making the store perform a conditional upsert: change or
overload pairedMacStore.upsert to accept expectedActiveMacID/expectedStackUserID
(or an expected generation token) and have the store apply the write only if
those expectations still match the current row; if the condition fails, return a
clear no-op/error so callers know the update was skipped instead of
reactivating/stale-marking the MAC as active.
🪄 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: d5cb5682-2b71-409c-9889-da4dd899ed58
📒 Files selected for processing (5)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryRefreshing.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceRegistryRouteSelectionTests.swiftSources/Cloud/DeviceRegistryClient.swiftcmuxTests/DeviceRegistryClientTests.swift
Comment on lines
+128
to
+135
| if let http = response as? HTTPURLResponse { | ||
| if (200...299).contains(http.statusCode) { | ||
| // Only remember the scope once the server accepted it, so a | ||
| // transient failure retries on the next status tick. | ||
| lastRegistration = registration | ||
| } else { | ||
| NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode) | ||
| } |
There was a problem hiding this comment.
Replace this NSLog with unified logging.
New production Swift runtime code should not introduce NSLog; route this through Logger instead so it stays on the repo’s supported logging path.
Suggested fix
+import OSLog
+
`@MainActor`
final class DeviceRegistryClient {
static let shared = DeviceRegistryClient()
+ private let logger = Logger(
+ subsystem: Bundle.main.bundleIdentifier ?? "dev.cmux.mac",
+ category: "device-registry"
+ )
private let session: URLSession = .shared
@@
if (200...299).contains(http.statusCode) {
// Only remember the scope once the server accepted it, so a
// transient failure retries on the next status tick.
lastRegistration = registration
} else {
- NSLog("cmux.deviceRegistry register failed status=%d", http.statusCode)
+ logger.error("device registry register failed status=\(http.statusCode, privacy: .public)")
}Based on learnings, production Swift runtime code must not use NSLog; use Apple’s unified logging system (Logger) 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 `@Sources/Cloud/DeviceRegistryClient.swift` around lines 128 - 135, Replace the
NSLog call in the HTTP response handling block with the project’s unified
Logger: locate the response handling inside DeviceRegistryClient (the block that
checks if let http = response as? HTTPURLResponse and updates lastRegistration =
registration) and replace NSLog("cmux.deviceRegistry register failed status=%d",
http.statusCode) with a Logger-based call (e.g., using a shared or instance
Logger) that logs the same message and http.statusCode; ensure you import
OSLog/Logging if needed and use the existing Logger instance or add one to the
DeviceRegistryClient so logging follows the repo’s unified logging path.
Source: Learnings
Addresses round-3 autoreview findings on the registry's correctness for supported multi-team and multi-build cmux workflows: - Device identity is now modeled per team. `devices` gains a surrogate primary key and a `device_uuid` column (the cmux-generated UUID, still the global device identity) with a unique `(team_id, device_uuid)` index, so a Mac that belongs to two teams registers a row in each. This unblocks the Mac's team-switch re-registration, which the old global device PK rejected with `device_team_conflict`. That guard is removed: per-team rows make cross-team takeover structurally impossible (team B's row can't touch team A's). GET returns `device_uuid` as `deviceId` so the phone's macDeviceID match is unchanged; instance cap and FK are unchanged (the instances FK already references the surrogate id). DELETE keys on `(team_id, device_uuid)`. Migration regenerated as a single clean migration. - iOS route selection no longer silently substitutes across tagged app instances. A Mac may run stable + a debug build, each its own `(deviceId, tag)` instance; the phone has no tag to match in P1, so `routes(forMacDeviceID:)` now returns routes only when exactly one instance is advertising any, and otherwise returns nil to fall back to local routes rather than risk connecting to the wrong build's workspaces. Tag-aware matching is a follow-up (alongside key pinning). Tests: web multi-team registration (same UUID in team A and B -> two rows, each team sees only its own); iOS multiple-non-empty-instances -> nil and single-non-empty-among-empty -> used. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cab19faca4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+77
to
+79
| for await status in MobileHostService.shared.statusUpdates() { | ||
| if Task.isCancelled { break } | ||
| await self?.registerIfRoutesChanged(routes: status.routes) |
There was a problem hiding this comment.
Because this observer only calls registerIfRoutesChanged from MobileHostService.statusUpdates(), a Mac that already has stable advertised routes will not POST them after the user signs in or switches teams unless the listener/connection state also changes. I checked that statusUpdates() is driven by mobile-host status notifications, while AuthCoordinator.selectedTeamID only persists the selection, so in that scenario the new team never gets a device row and a phone scoped to that team cannot auto-pair until the host happens to emit another status tick.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/api/devices/route.ts (1)
286-316: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider a SQL JOIN to fetch devices with instances in one query.
The current implementation fetches devices and instances separately then joins in memory. While this is bounded by the per-team caps (max ~5,000 rows) and functionally correct, a single SQL LEFT JOIN query would eliminate the round-trip, reduce memory allocation, and simplify the grouping logic.
♻️ Proposed SQL JOIN approach
const db = cloudDb(); - const deviceRows = (await db + const joined = await db .select({ + deviceId: devices.id, + deviceUuid: devices.deviceUuid, + platform: devices.platform, + displayName: devices.displayName, + deviceLabels: devices.labels, + deviceLastSeenAt: devices.lastSeenAt, + instanceTag: deviceAppInstances.tag, + instanceRoutes: deviceAppInstances.routes, + instanceLabels: deviceAppInstances.labels, + instanceLastSeenAt: deviceAppInstances.lastSeenAt, - id: devices.id, - deviceUuid: devices.deviceUuid, - platform: devices.platform, - displayName: devices.displayName, - labels: devices.labels, - lastSeenAt: devices.lastSeenAt, }) .from(devices) + .leftJoin(deviceAppInstances, eq(deviceAppInstances.deviceId, devices.id)) .where(eq(devices.teamId, team.teamId)) - .orderBy(desc(devices.lastSeenAt))) as DeviceListRow[]; + .orderBy(desc(devices.lastSeenAt), desc(deviceAppInstances.lastSeenAt)); - const instanceRows = await db - .select({ - deviceId: deviceAppInstances.deviceId, - tag: deviceAppInstances.tag, - routes: deviceAppInstances.routes, - labels: deviceAppInstances.labels, - lastSeenAt: deviceAppInstances.lastSeenAt, - }) - .from(deviceAppInstances) - .where(eq(deviceAppInstances.teamId, team.teamId)) - .orderBy(desc(deviceAppInstances.lastSeenAt)); - - const instancesByDevice = new Map<string, typeof instanceRows>(); - for (const row of instanceRows) { - const list = instancesByDevice.get(row.deviceId) ?? []; - list.push(row); - instancesByDevice.set(row.deviceId, list); - } + // Group rows by device in a single pass + const deviceMap = new Map<string, { device: DeviceListRow; instances: Array<...> }>(); + for (const row of joined) { + if (!deviceMap.has(row.deviceId)) { + deviceMap.set(row.deviceId, { + device: { id: row.deviceId, deviceUuid: row.deviceUuid, ... }, + instances: [], + }); + } + if (row.instanceTag) { + deviceMap.get(row.deviceId)!.instances.push({ tag: row.instanceTag, ... }); + } + } - const devicesPayload = deviceRows.map((device) => ({ + const devicesPayload = Array.from(deviceMap.values()).map(({ device, instances }) => ({ deviceId: device.deviceUuid, platform: device.platform, displayName: device.displayName, - labels: device.labels, - lastSeenAt: device.lastSeenAt.toISOString(), - instances: (instancesByDevice.get(device.id) ?? []).map((instance) => ({ - tag: instance.tag, - routes: instance.routes, - labels: instance.labels, - lastSeenAt: instance.lastSeenAt.toISOString(), - })), + labels: device.deviceLabels, + lastSeenAt: device.deviceLastSeenAt.toISOString(), + instances, }));🤖 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/app/api/devices/route.ts` around lines 286 - 316, Fetch devices and their instances in one DB query instead of two: replace the separate db.select(...) calls for deviceRows and instanceRows with a single query using a LEFT JOIN between devices and deviceAppInstances (use devices and deviceAppInstances in the SELECT and join ON devices.deviceUuid/deviceId or devices.id/deviceAppInstances.deviceId as appropriate) ordering by devices.lastSeenAt or deviceAppInstances.lastSeenAt, then iterate the joined result to build the same structure previously produced by instancesByDevice; update the code that referenced deviceRows and instancesByDevice to consume the combined result set (use the same field names like deviceId, deviceUuid, platform, displayName, labels, tag, routes, lastSeenAt) so you remove the extra in-memory grouping loop and the duplicate round-trip to db.web/tests/devices-route.test.ts (1)
69-265: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd coverage for the per-team device cap enforcement.
The test suite exercises the per-device instance cap (lines 138-169) but not the per-team device cap. Line 172-179 of
web/app/api/devices/route.tsenforcesMAX_DEVICES_PER_TEAMand returns HTTP 429 when exceeded, but no test verifies this path.🧪 Proposed test case
dbTest("rejects registration when the team device cap is exceeded", async () => { if (!sql) throw new Error("test database not initialized"); // Seed the DB with 200 devices (the cap) for team-a. const deviceIds = Array.from({ length: 200 }, (_, i) => `${(i + 1).toString().padStart(8, '0')}-0000-4000-8000-000000000000` ); for (const deviceId of deviceIds) { await sql` insert into devices (team_id, device_uuid, user_id, platform) values ('team-a', ${deviceId}, 'registry-user-1', 'mac') `; } // Attempt to register a 201st device. const response = await POST( registerRequest({ deviceId: "99999999-9999-4999-8999-999999999999", platform: "mac", routes: [], }) ); expect(response.status).toBe(429); const body = await response.json(); expect(body.error).toBe("too_many_devices"); });🤖 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/tests/devices-route.test.ts` around lines 69 - 265, Add a new dbTest that seeds the database with MAX_DEVICES_PER_TEAM devices for "team-a" and then attempts one more registration to assert the 429 "too_many_devices" response: use the existing helpers registerRequest and POST to perform the attempted register, use the test sql helper to insert rows directly into the devices table (insert into devices (team_id, device_uuid, user_id, platform) values ... ) to create MAX_DEVICES_PER_TEAM entries, and assert response.status === 429 and response.json().error === "too_many_devices"; reference MAX_DEVICES_PER_TEAM (from web/app/api/devices/route.ts), and the test helpers registerRequest, POST, and sql so the new dbTest mirrors the style of the existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/app/api/devices/route.ts`:
- Around line 286-316: Fetch devices and their instances in one DB query instead
of two: replace the separate db.select(...) calls for deviceRows and
instanceRows with a single query using a LEFT JOIN between devices and
deviceAppInstances (use devices and deviceAppInstances in the SELECT and join ON
devices.deviceUuid/deviceId or devices.id/deviceAppInstances.deviceId as
appropriate) ordering by devices.lastSeenAt or deviceAppInstances.lastSeenAt,
then iterate the joined result to build the same structure previously produced
by instancesByDevice; update the code that referenced deviceRows and
instancesByDevice to consume the combined result set (use the same field names
like deviceId, deviceUuid, platform, displayName, labels, tag, routes,
lastSeenAt) so you remove the extra in-memory grouping loop and the duplicate
round-trip to db.
In `@web/tests/devices-route.test.ts`:
- Around line 69-265: Add a new dbTest that seeds the database with
MAX_DEVICES_PER_TEAM devices for "team-a" and then attempts one more
registration to assert the 429 "too_many_devices" response: use the existing
helpers registerRequest and POST to perform the attempted register, use the test
sql helper to insert rows directly into the devices table (insert into devices
(team_id, device_uuid, user_id, platform) values ... ) to create
MAX_DEVICES_PER_TEAM entries, and assert response.status === 429 and
response.json().error === "too_many_devices"; reference MAX_DEVICES_PER_TEAM
(from web/app/api/devices/route.ts), and the test helpers registerRequest, POST,
and sql so the new dbTest mirrors the style of the existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e2bd8d0-8e48-4bb9-a7e9-e7c8381439eb
📒 Files selected for processing (7)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceRegistryRouteSelectionTests.swiftweb/app/api/devices/route.tsweb/db/migrations/20260608055850_device_registry/migration.sqlweb/db/migrations/20260608055850_device_registry/snapshot.jsonweb/db/schema.tsweb/tests/devices-route.test.ts
Round-4 autoreview: GET exposes device UUIDs to every team member, and POST upserted by UUID for the team, so a co-member could overwrite another member's device routes (redirecting that user's phone reconnect at the attacker's host). The POST path now rejects (403 device_not_owned) when an existing device row was registered by a different user, so route population stays owned by the registering user, matching the pre-registry trust boundary. Cryptographic proof-of-possession is the deferred key-pinning phase. Consciously accepted (not fixed) from the same review, with rationale: - Initial-empty / post-kill off-state: lastRegistration is in-memory, so a clear during process death can leave the user's own last routes advertised. With the ownership guard those are the user's own routes, so the worst case is one failed connect then local fallback (no security impact). Persisting registration state to publish an off-state after death is follow-up. - Team switch with unchanged routes only re-registers on the next host status tick (registration is route-driven). Documented as a known limitation; teamID stays in the dedup key so it is correct once triggered. An explicit auth/team-change trigger is a follow-up. Test: a second same-team user POSTing another user's device UUID is rejected and the routes are unchanged; the owner can still update. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b61ace60e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // never another team's row for the same physical Mac. | ||
| const db = cloudDb(); | ||
| await db | ||
| .delete(devices) |
There was a problem hiding this comment.
When two users are members of the same team, any member can take a deviceId returned by GET /api/devices and delete another member's registered Mac because this deletion only filters by team and device UUID, unlike the POST path's owner check. That cascades the other user's app instances and removes their registry routes, so their phone loses the auto-pair refresh until the Mac re-registers; add the same userId ownership predicate or an explicit admin authorization check here.
Useful? React with 👍 / 👎.
Comment on lines
+357
to
+383
| export async function DELETE(request: Request): Promise<Response> { | ||
| const user = await verifyRequest(request, { | ||
| requestedTeamId: requestedVmTeamIdFromRequest(request), | ||
| allowCookie: false, | ||
| }); | ||
| if (!user) return unauthorized(); | ||
|
|
||
| const team = resolveTeam(request, user); | ||
| if (!team.ok) return team.response; | ||
|
|
||
| const body = await readBoundedJson(request); | ||
| if (!body.ok) return jsonResponse({ error: "invalid_request" }, body.status); | ||
|
|
||
| const deviceUuid = trimmedString(body.value.deviceId).toLowerCase(); | ||
| if (!UUID_RE.test(deviceUuid)) { | ||
| return jsonResponse({ error: "invalid_device_id" }, 400); | ||
| } | ||
|
|
||
| // Delete only this team's row for the device (the (teamId, deviceUuid) row), | ||
| // never another team's row for the same physical Mac. | ||
| const db = cloudDb(); | ||
| await db | ||
| .delete(devices) | ||
| .where(and(eq(devices.deviceUuid, deviceUuid), eq(devices.teamId, team.teamId))); | ||
|
|
||
| return jsonResponse({ ok: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
POST explicitly rejects requests where existingDevice.userId !== user.id with a clear rationale: "GET exposes device UUIDs to every team member, so without this a co-member could POST another member's device UUID and overwrite its attach routes." The same reasoning applies to DELETE — but it's absent here.
A malicious team member can: (1) call GET /api/devices to learn the victim's deviceUuid, (2) DELETE the row, (3) immediately POST to register the same deviceUuid under their own userId. After step 3, the legitimate Mac gets a 403 device_not_owned on every subsequent registration attempt and can no longer update its own routes in the registry. The phone's refresh then consistently fetches the attacker's routes for the victim's device ID.
Add the same ownership guard used in POST: fetch the row's userId and return 403 if it doesn't match user.id.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/api/devices/route.ts (1)
357-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDELETE bypasses the new ownership guard.
GETexposesdeviceIdto every team member, and this handler deletes any(teamId, deviceUuid)row without checkingdevices.userId. A co-member can therefore delete another user's device row and then re-register the same UUID under their ownuserId, which defeats the newdevice_not_ownedprotection entirely. Apply the same owner check here (or an explicit admin-only policy) before deleting.Suggested fix
export async function DELETE(request: Request): Promise<Response> { const user = await verifyRequest(request, { requestedTeamId: requestedVmTeamIdFromRequest(request), allowCookie: false, }); if (!user) return unauthorized(); @@ const deviceUuid = trimmedString(body.value.deviceId).toLowerCase(); if (!UUID_RE.test(deviceUuid)) { return jsonResponse({ error: "invalid_device_id" }, 400); } - // Delete only this team's row for the device (the (teamId, deviceUuid) row), - // never another team's row for the same physical Mac. const db = cloudDb(); + const [existingDevice] = await db + .select({ userId: devices.userId }) + .from(devices) + .where(and(eq(devices.deviceUuid, deviceUuid), eq(devices.teamId, team.teamId))) + .limit(1); + + if (existingDevice && existingDevice.userId !== user.id) { + return jsonResponse({ error: "device_not_owned" }, 403); + } + await db .delete(devices) .where(and(eq(devices.deviceUuid, deviceUuid), eq(devices.teamId, team.teamId))); return jsonResponse({ ok: true }); }🤖 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/app/api/devices/route.ts` around lines 357 - 380, The DELETE handler currently deletes the (teamId, deviceUuid) row without checking ownership; update the DELETE function to enforce the same ownership guard as GET by first checking devices.userId against the authenticated user (from verifyRequest) or an explicit admin allowlist before proceeding: query the devices table for a row matching devices.deviceUuid and devices.teamId, verify devices.userId === user.userId (or that user has admin rights), return a 403/json error if not authorized, and only then perform the delete (or alternatively include eq(devices.userId, user.userId) in the delete WHERE clause) so deletion cannot be performed by co-members.Sources/Cloud/DeviceRegistryClient.swift (1)
74-84:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRegister on team changes, not just host-status ticks.
A mid-session team switch with unchanged routes will not POST into the new team until
MobileHostServiceemits another status update. That leaves this Mac registered only under the old team, so the new team cannot rediscover it until some unrelated route/connection event happens. Wire the same registration path to auth/team-selection changes and push the currentstatusSnapshot().routesimmediately when the team changes.🤖 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/Cloud/DeviceRegistryClient.swift` around lines 74 - 84, The current startObserving() only listens to MobileHostService.shared.statusUpdates() so a mid-session team change with unchanged routes never triggers registerIfRoutesChanged(routes:), leaving the device registered to the old team; subscribe to the auth/team-selection change events in the same Task (or a sibling Task on `@MainActor`) and, when a team-change is observed, immediately call registerIfRoutesChanged(routes: MobileHostService.shared.statusSnapshot().routes) (respecting Task cancellation and using await on self?) so the registration path is invoked on team switches as well as route updates; ensure the new listener is cancelled along with observeTask and uses the same weak self pattern to avoid retain cycles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Cloud/DeviceRegistryClient.swift`:
- Around line 74-84: The current startObserving() only listens to
MobileHostService.shared.statusUpdates() so a mid-session team change with
unchanged routes never triggers registerIfRoutesChanged(routes:), leaving the
device registered to the old team; subscribe to the auth/team-selection change
events in the same Task (or a sibling Task on `@MainActor`) and, when a
team-change is observed, immediately call registerIfRoutesChanged(routes:
MobileHostService.shared.statusSnapshot().routes) (respecting Task cancellation
and using await on self?) so the registration path is invoked on team switches
as well as route updates; ensure the new listener is cancelled along with
observeTask and uses the same weak self pattern to avoid retain cycles.
In `@web/app/api/devices/route.ts`:
- Around line 357-380: The DELETE handler currently deletes the (teamId,
deviceUuid) row without checking ownership; update the DELETE function to
enforce the same ownership guard as GET by first checking devices.userId against
the authenticated user (from verifyRequest) or an explicit admin allowlist
before proceeding: query the devices table for a row matching devices.deviceUuid
and devices.teamId, verify devices.userId === user.userId (or that user has
admin rights), return a 403/json error if not authorized, and only then perform
the delete (or alternatively include eq(devices.userId, user.userId) in the
delete WHERE clause) so deletion cannot be performed by co-members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79c9e198-ff58-4f67-8c6d-8f98c52d2353
📒 Files selected for processing (3)
Sources/Cloud/DeviceRegistryClient.swiftweb/app/api/devices/route.tsweb/tests/devices-route.test.ts
Round-5 autoreview: - DeviceRegistryClient.shouldReRegister is pure but inherited @mainactor from the enclosing class, so the non-@mainactor cmuxTests suite calling it synchronously would fail to compile the test bundle (only CI compiles it). Marked the policy method `nonisolated`. - DELETE only scoped by (team, deviceUuid), so a co-member who learns a device UUID via GET could delete another user's Mac and break their reconnect. Now also scopes by userId, mirroring the POST ownership guard. The delete stays an idempotent no-op (200) when the row is not the caller's. Test: a second same-team user's DELETE of another user's device is a no-op and the row survives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nventions)
package-conventions-lint flags standalone caseless namespace enums. Folded the
two standalone enums into DeviceRegistryService as static members:
- MobileDeviceIdentity.deviceID(defaults:) -> DeviceRegistryService.deviceID(defaults:)
- DeviceRegistryRouteSelection.{selectReconnectRoutes,shouldApplyRegistryRefresh}
-> static methods on DeviceRegistryService
DeviceRegistryRefreshing keeps only the protocol. Call sites in
MobileShellComposite, CMUXMobileRootScene, and the tests updated. Behavior and
test coverage unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ration DeviceRegistryClient read auth.resolvedTeamID before awaiting currentTokens(). resolvedTeamID derives from availableTeams, which is empty until launch auth bootstrap completes, so on launch it could resolve nil even with a persisted selected team and publish the Mac into the Stack-default team. The team is now resolved after the currentTokens() await (which gates on signed-in and waits for bootstrap) and used for both the dedup key and the X-Cmux-Team-Id header. After bootstrap currentTokens() returns the cached token, so awaiting it per status tick is cheap. The residual team-handling limitation (a mid-session team switch with unchanged routes only re-registers on the next host-status tick, since registration is route-driven) is documented in startObserving; an explicit auth/team-change trigger is a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17562bc. Configure here.
| ) -> [CmxAttachRoute]? { | ||
| guard let registry, !registry.isEmpty else { return nil } | ||
| guard registry != local else { return nil } | ||
| return registry |
There was a problem hiding this comment.
Stale registry overwrites local routes
Medium Severity
When a device row belongs to another team member, the Mac cannot POST updated routes, but GET still returns that row to everyone on the team. selectReconnectRoutes treats any differing registry routes as fresher and the shell persists them, so phones can replace good locally paired routes with outdated registry endpoints after an account switch or stale registration.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 17562bc. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17562bc629
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+67
to
+71
| // Treat "never registered" as an empty-routes baseline in the same scope | ||
| // so an initial empty set (pairing off at launch) is a no-op, but a later | ||
| // clear, or any team/tag change, still fires. | ||
| let baseline = previous ?? Registration(teamID: current.teamID, tag: current.tag, routes: []) | ||
| return baseline != current |
There was a problem hiding this comment.
When mobile pairing is disabled on startup, treating previous == nil as already-empty skips the POST that would clear any registry row left from a prior successful registration. If the user turns pairing off while the registry POST is unavailable, or the app exits before the empty-route transition is accepted, the next launch with empty routes will hit this no-op path and leave the old non-empty routes in /api/devices, so phones keep discovering stale routes until pairing is enabled again. Persist the last advertised state or send one empty-route update when the server state is unknown.
Useful? React with 👍 / 👎.
lawrencecchen
added a commit
that referenced
this pull request
Jun 10, 2026
…evice registry (#5648) * iOS: hierarchical device tree (device → tags → workspaces) over the device registry Render the merged #5626 device registry as a hierarchical tree: each registered device (Mac/host) expands to its cmux app instances (tags), and a tag expands to that build's workspaces; tapping a workspace opens it via the existing path. Surfaces the registry list to the UI (DeviceRegistryRefreshing.listDevices), adds a RegistryDevice/RegistryAppInstance value model, store.registryDevices + loadRegistryDevices + connectToRegistryInstance (connect-on-tap a non-connected tag via its routes), and a DeviceTreeView reachable from Settings. Keeps the flat workspace list and the multi-Mac switcher as the fallback paths. Online state: the connected device shows live macConnectionStatus; others show registry last-seen (best-effort, no per-host ping yet; the attach ticket carries no tag, so per-tag liveness is a TODO). Expansion persists via @AppStorage. Localized en+ja. Pure decode + expansion-codec tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: fix account-switch race + multi-tag wrong-tag workspaces (autoreview P1s) P1-1: loadRegistryDevices now captures the requesting user id and discards a result that lands after a sign-out + different-user sign-in, so a slow registry load can't leak a previous user's team devices into the new user's tree (mirrors loadPairedMacs's user guard). P1-2: attribute live workspaces to the ONE instance whose route matches the live connection (instanceMatchesActiveRoute), not every tag on the connected device. A multi-tag Mac now shows workspaces only under the connected build; the other tags offer Connect instead of mirroring the wrong build's workspaces, so a workspace can no longer be opened under the wrong tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back failed registry connect + paired-Mac fallback (autoreview) P1: connectToRegistryInstance now captures the previously-active Mac and, when the destructive connect fails to land on the target route, reconnects it (mirrors switchToMac). Tapping a stale/offline registry tag no longer drops a healthy live session; the user is left where they were. P2: add store.deviceTreeDevices, which honors the documented best-effort fallback: the registry list when loaded, otherwise the locally paired Macs synthesized into the same device→instance shape. The tree now sources from it and loads paired Macs first, so the Devices sheet stays usable (and connectable) during a registry outage instead of showing the empty state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back to the active Mac even on a same-device tag switch (autoreview P1) The previous rollback excluded the tapped device id (copied from switchToMac), which is wrong here: a Mac runs multiple tagged builds, so tapping another tag on the currently-connected device must still be able to reconnect that device's active route when the new tag is stale/offline. Capture the active paired Mac regardless of device id so a same-device tag-switch failure restores the live session instead of stranding the user disconnected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: clear team-scoped registry data on auth rejection (autoreview P1) listDevices() now returns a 3-way outcome (ok / authRejected / transientFailure) instead of an optional, so the store can distinguish a transient blip (keep the tree) from a 401/403 auth/scope rejection (clear it). The registry is team-scoped, so a token/scope change must not leave a previous scope's team-device names/tags/ routes visible; on authRejected the store clears registryDevices and the tree falls back to local paired Macs. Transient failures (5xx, network, malformed body) keep the current tree to avoid blip-blanking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: present from workspace list (single sheet) + recognize manual-ticket active device (autoreview P2) P2 (sheet stack): move the device tree to a top-level sheet on the workspace list (a Devices toolbar button) instead of nesting it under the Settings sheet, so selecting a workspace dismisses straight back to the workspace shell and reveals the opened workspace rather than leaving Settings covering it. Removes the duplicate Settings 'Devices' entry. P2 (manual-ticket active): connectedMacDeviceID now falls back to the active paired Mac's real device id when the live ticket is a synthetic manual one (host without mobile.attach_ticket.create). The registry connect path persists the real device as active, so the tree now marks it connected and shows its live workspaces instead of hiding them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: refresh MobileShellComposite file length budget for device-tree growth * Guard authRejected registry blanking on the requesting user still being current Mirrors the .ok path's account-switch guard: a stale 401 from a signed-out session that lands after a different user signed in no longer blanks the new user's device tree. Addresses the Greptile P1 on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: refresh file-length budget for the authRejected guard growth Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 10, 2026
* iOS: hierarchical device tree (device → tags → workspaces) over the device registry Render the merged #5626 device registry as a hierarchical tree: each registered device (Mac/host) expands to its cmux app instances (tags), and a tag expands to that build's workspaces; tapping a workspace opens it via the existing path. Surfaces the registry list to the UI (DeviceRegistryRefreshing.listDevices), adds a RegistryDevice/RegistryAppInstance value model, store.registryDevices + loadRegistryDevices + connectToRegistryInstance (connect-on-tap a non-connected tag via its routes), and a DeviceTreeView reachable from Settings. Keeps the flat workspace list and the multi-Mac switcher as the fallback paths. Online state: the connected device shows live macConnectionStatus; others show registry last-seen (best-effort, no per-host ping yet; the attach ticket carries no tag, so per-tag liveness is a TODO). Expansion persists via @AppStorage. Localized en+ja. Pure decode + expansion-codec tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: fix account-switch race + multi-tag wrong-tag workspaces (autoreview P1s) P1-1: loadRegistryDevices now captures the requesting user id and discards a result that lands after a sign-out + different-user sign-in, so a slow registry load can't leak a previous user's team devices into the new user's tree (mirrors loadPairedMacs's user guard). P1-2: attribute live workspaces to the ONE instance whose route matches the live connection (instanceMatchesActiveRoute), not every tag on the connected device. A multi-tag Mac now shows workspaces only under the connected build; the other tags offer Connect instead of mirroring the wrong build's workspaces, so a workspace can no longer be opened under the wrong tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back failed registry connect + paired-Mac fallback (autoreview) P1: connectToRegistryInstance now captures the previously-active Mac and, when the destructive connect fails to land on the target route, reconnects it (mirrors switchToMac). Tapping a stale/offline registry tag no longer drops a healthy live session; the user is left where they were. P2: add store.deviceTreeDevices, which honors the documented best-effort fallback: the registry list when loaded, otherwise the locally paired Macs synthesized into the same device→instance shape. The tree now sources from it and loads paired Macs first, so the Devices sheet stays usable (and connectable) during a registry outage instead of showing the empty state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back to the active Mac even on a same-device tag switch (autoreview P1) The previous rollback excluded the tapped device id (copied from switchToMac), which is wrong here: a Mac runs multiple tagged builds, so tapping another tag on the currently-connected device must still be able to reconnect that device's active route when the new tag is stale/offline. Capture the active paired Mac regardless of device id so a same-device tag-switch failure restores the live session instead of stranding the user disconnected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: clear team-scoped registry data on auth rejection (autoreview P1) listDevices() now returns a 3-way outcome (ok / authRejected / transientFailure) instead of an optional, so the store can distinguish a transient blip (keep the tree) from a 401/403 auth/scope rejection (clear it). The registry is team-scoped, so a token/scope change must not leave a previous scope's team-device names/tags/ routes visible; on authRejected the store clears registryDevices and the tree falls back to local paired Macs. Transient failures (5xx, network, malformed body) keep the current tree to avoid blip-blanking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: present from workspace list (single sheet) + recognize manual-ticket active device (autoreview P2) P2 (sheet stack): move the device tree to a top-level sheet on the workspace list (a Devices toolbar button) instead of nesting it under the Settings sheet, so selecting a workspace dismisses straight back to the workspace shell and reveals the opened workspace rather than leaving Settings covering it. Removes the duplicate Settings 'Devices' entry. P2 (manual-ticket active): connectedMacDeviceID now falls back to the active paired Mac's real device id when the live ticket is a synthetic manual one (host without mobile.attach_ticket.create). The registry connect path persists the real device as active, so the tree now marks it connected and shows its live workspaces instead of hiding them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * iOS: mobile browser panes P1 (WKWebView surface) Add a phone-local WKWebView browser pane as a sibling of the terminal surface in the iOS companion app. New `CmuxMobileBrowser` package owns the browser surface state, URL resolver, store, and the WKWebView host; `CmuxMobileShellUI` presents it from the same workspace toolbar menu that creates terminals ("New Browser"), and a close action returns to the terminal. Browser state lives in a dedicated workspace-keyed `BrowserSurfaceStore` injected from the app root, not in `MobileShellComposite`, because a browser has no Mac-side counterpart and must survive workspace.updated re-syncs (unlike terminals). P1 ships: address bar, navigate, back/forward/reload/stop, page title, determinate loading progress, default persistent WKWebsiteDataStore. P2 (cookie/localStorage sync with the Mac) and P3 (passkeys) are deferred; see plans/feat-ios-mobile-browser/DESIGN.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Make BrowserURLResolver an uninstantiable struct per package conventions * chore: refresh MobileShellComposite file length budget for device-tree growth * Guard authRejected registry blanking on the requesting user still being current Mirrors the .ok path's account-switch guard: a stale 401 from a signed-out session that lands after a different user signed in no longer blanks the new user's device tree. Addresses the Greptile P1 on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: refresh file-length budget for the authRejected guard growth Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
Jun 10, 2026
…flow.ai on tailnet, else email) (#5653) * iOS: hierarchical device tree (device → tags → workspaces) over the device registry Render the merged #5626 device registry as a hierarchical tree: each registered device (Mac/host) expands to its cmux app instances (tags), and a tag expands to that build's workspaces; tapping a workspace opens it via the existing path. Surfaces the registry list to the UI (DeviceRegistryRefreshing.listDevices), adds a RegistryDevice/RegistryAppInstance value model, store.registryDevices + loadRegistryDevices + connectToRegistryInstance (connect-on-tap a non-connected tag via its routes), and a DeviceTreeView reachable from Settings. Keeps the flat workspace list and the multi-Mac switcher as the fallback paths. Online state: the connected device shows live macConnectionStatus; others show registry last-seen (best-effort, no per-host ping yet; the attach ticket carries no tag, so per-tag liveness is a TODO). Expansion persists via @AppStorage. Localized en+ja. Pure decode + expansion-codec tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: fix account-switch race + multi-tag wrong-tag workspaces (autoreview P1s) P1-1: loadRegistryDevices now captures the requesting user id and discards a result that lands after a sign-out + different-user sign-in, so a slow registry load can't leak a previous user's team devices into the new user's tree (mirrors loadPairedMacs's user guard). P1-2: attribute live workspaces to the ONE instance whose route matches the live connection (instanceMatchesActiveRoute), not every tag on the connected device. A multi-tag Mac now shows workspaces only under the connected build; the other tags offer Connect instead of mirroring the wrong build's workspaces, so a workspace can no longer be opened under the wrong tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back failed registry connect + paired-Mac fallback (autoreview) P1: connectToRegistryInstance now captures the previously-active Mac and, when the destructive connect fails to land on the target route, reconnects it (mirrors switchToMac). Tapping a stale/offline registry tag no longer drops a healthy live session; the user is left where they were. P2: add store.deviceTreeDevices, which honors the documented best-effort fallback: the registry list when loaded, otherwise the locally paired Macs synthesized into the same device→instance shape. The tree now sources from it and loads paired Macs first, so the Devices sheet stays usable (and connectable) during a registry outage instead of showing the empty state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: roll back to the active Mac even on a same-device tag switch (autoreview P1) The previous rollback excluded the tapped device id (copied from switchToMac), which is wrong here: a Mac runs multiple tagged builds, so tapping another tag on the currently-connected device must still be able to reconnect that device's active route when the new tag is stale/offline. Capture the active paired Mac regardless of device id so a same-device tag-switch failure restores the live session instead of stranding the user disconnected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: clear team-scoped registry data on auth rejection (autoreview P1) listDevices() now returns a 3-way outcome (ok / authRejected / transientFailure) instead of an optional, so the store can distinguish a transient blip (keep the tree) from a 401/403 auth/scope rejection (clear it). The registry is team-scoped, so a token/scope change must not leave a previous scope's team-device names/tags/ routes visible; on authRejected the store clears registryDevices and the tree falls back to local paired Macs. Transient failures (5xx, network, malformed body) keep the current tree to avoid blip-blanking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Device tree: present from workspace list (single sheet) + recognize manual-ticket active device (autoreview P2) P2 (sheet stack): move the device tree to a top-level sheet on the workspace list (a Devices toolbar button) instead of nesting it under the Settings sheet, so selecting a workspace dismisses straight back to the workspace shell and reveals the opened workspace rather than leaving Settings covering it. Removes the duplicate Settings 'Devices' entry. P2 (manual-ticket active): connectedMacDeviceID now falls back to the active paired Mac's real device id when the live ticket is a synthetic manual one (host without mobile.attach_ticket.create). The registry connect path persists the real device as active, so the tree now marks it connected and shows its live workspaces instead of hiding them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * iOS: Release-safe Send Feedback (privileged direct-to-agent for @manaflow.ai on tailnet, else email) Adds a Release-safe "Send Feedback" affordance to the iOS terminal menu that routes two ways and always stamps build type + version + device: - Privileged direct-to-agent: when the signed-in email ends with @manaflow.ai AND there is an active mobile-host connection to a paired Mac (the on-tailnet proxy, since that transport runs over Tailscale). Reuses the existing dogfood.feedback.submit RPC + bundle so the Mac watcher under ~/.cache/cmux-dogfood-feedback/ still catches it. The phone delivery, the Mac sink, and the dogfood.v1 capability are un-#if-DEBUG-gated together so the path works on Release (beta/prod) for the team; the sink keeps its same-account Stack-auth gate. - Email otherwise: POSTs to the existing web /api/feedback route (Resend), prefilled + editable reply-to. The route now takes a buildType field and stamps it into the subject. Build type is derived once from #if DEBUG (dev) + bundle id (dev.cmux.app.beta => beta, else prod). Pure routing + stamp formatting live in CmuxMobileShellModel with Swift Testing coverage; the email multipart builder is unit tested too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Gate privileged feedback route on the Mac's dogfood.v1 capability Autoreview P2: a newer phone connected to an older Mac that does not expose dogfood.feedback.submit would still take the agent route and fail with method_not_found instead of emailing. Record supportsDogfoodFeedback from the host status capabilities (mirroring supportsWorkspaceActions) and require it in the pure routing decision, so version skew falls back to the email inbox. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * iOS: mobile browser panes P1 (WKWebView surface) Add a phone-local WKWebView browser pane as a sibling of the terminal surface in the iOS companion app. New `CmuxMobileBrowser` package owns the browser surface state, URL resolver, store, and the WKWebView host; `CmuxMobileShellUI` presents it from the same workspace toolbar menu that creates terminals ("New Browser"), and a close action returns to the terminal. Browser state lives in a dedicated workspace-keyed `BrowserSurfaceStore` injected from the app root, not in `MobileShellComposite`, because a browser has no Mac-side counterpart and must survive workspace.updated re-syncs (unlike terminals). P1 ships: address bar, navigate, back/forward/reload/stop, page title, determinate loading progress, default persistent WKWebsiteDataStore. P2 (cookie/localStorage sync with the Mac) and P3 (passkeys) are deferred; see plans/feat-ios-mobile-browser/DESIGN.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Enforce feedback privilege at the Mac trust boundary; fix Release agent copy Autoreview P1: the un-gated dogfood.feedback.submit sink accepted any same-account caller, with the only @manaflow.ai check on the phone. A crafted RPC could bypass the privileged gate. Now the Mac handler rejects with "unauthorized" unless this Mac's authenticated Stack email is @manaflow.ai (same-account means the caller is the Mac's own account). Adds currentAuthenticatedLocalUserEmail() and a local privileged-domain check mirroring isManaflowEmail; the phone keeps its route gate as defense-in-depth. Autoreview P2: the Release store has no DiagnosticLog (it is DEBUG-only in the app composition), so the structured event blob is empty in Release while the UI promised "events". The agent-path copy no longer promises the event trace; the bundle still carries the note, visible terminal, mobile debug log, and stamp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Autoreview style: split MobileFeedbackEmailError to its own file; DocC on client submit Addresses Aziz file-organization + documentation findings: one major type per file, and DocC on the concrete email-client submit. The nested FeedbackSubmissionOutcome and the testable static multipartBody are intentional. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Fall back to email when the privileged agent feedback path fails Defense-in-depth: if the agent sink rejects or the RPC can't be delivered, route the report to the email inbox instead of dead-ending with .failed. The composer prefills the signed-in email, so a @manaflow.ai user always has a valid reply-to for the fallback. Extracts a shared submitFeedbackEmail helper so the direct email route and the fallback deliver identically. Confirmed CMUXAuthIdentityStore JSON round-trips the full CMUXAuthUser (incl. primaryEmail), so the Mac privilege gate reads a populated email on the restored path too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Make BrowserURLResolver an uninstantiable struct per package conventions * Scope feedback routing and stamp factory onto their owning types per package conventions * chore: refresh file length budgets for send-feedback growth * chore: refresh MobileShellComposite file length budget for device-tree growth * Guard authRejected registry blanking on the requesting user still being current Mirrors the .ok path's account-switch guard: a stale 401 from a signed-out session that lands after a different user signed in no longer blanks the new user's device tree. Addresses the Greptile P1 on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Reset isSubmittingFeedback when reopening the feedback composer Dismissing the sheet mid-send left the flag stuck true, so a reopened composer rendered Send disabled until the prior task timed out (up to 30s on the email path). Addresses the Greptile P1 on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: refresh file-length budget for the authRejected guard growth Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server-side device registry so the phone re-pairs to its Mac automatically on reload instead of re-scanning a QR. This is the central, team-scoped record of which Macs are connected and how to reach them.
Model
Two levels: a
device(a physical Mac/host, keyed by a cmux-generated persisted UUID, not IOPlatformUUID) and adevice_app_instance(one running cmux build/tag on that machine, holding its attach routes). Both carry a Stackteam_idand a flexiblelabelsjsonb. The registry is a best-effort rendezvous layer, not an authority on pairing. The phone keeps its localMobilePairedMacStoreand falls back to it when the registry is unreachable, so pairing survives the cloud registry being down.Web (
web/)db/schema.ts:devices+device_app_instancestables following thedeviceTokenspattern. Thedevice.id(cmux UUID) is documented as the seam the later key-pinning phase pins a per-device key to for revoke.app/api/devices/route.ts:POSTregister (idempotent per device, and per(device, tag)for the instance),GETlist,DELETE. Team-scoped viaX-Cmux-Team-Id(or?teamId=), defaulting to the Stack-selected team. Rejects a team the caller is not a member of with 403. Per-team cap behind a Postgres advisory lock, mirroring the device-tokens route.drizzle-kit(v1 snapshot chain).tests/devices-route.test.ts: register+list, route-refresh on re-register (the auto-pair path), non-member team 403, delete cascade. Passes against an isolated test Postgres.Mac (
Sources/Cloud/DeviceRegistryClient.swift)Registers this Mac and its app instance's attach routes by observing
MobileHostService.statusUpdates()and POSTing/api/deviceswhenever the route set changes (moved networks, new port). Auth mirrorsPhonePushClient(Bearer +X-Stack-Refresh-Token+X-Cmux-Team-Id,AuthEnvironment.vmAPIBaseURL), best-effort and non-blocking. Gating falls out of the routes:MobileHostServiceadvertises none until the user enabled pairing, so an empty set never registers (no separate opt-in flag). A pureshouldReRegister()skips redundant POSTs on connection-only status ticks. Wired inAppDelegatenext to the existingPhonePushClient/MobileHostServiceconfigure.iOS (
Packages/CmuxMobileShell)MobileDeviceIdentity: persists a cmux-generated device UUID (mirrors the Mac'sMobileHostIdentity), plumbed for the key-pinning phase.DeviceRegistryServicereads/api/devicesfor a Mac's fresh routes, injected intoMobileShellCompositeasDeviceRegistryRefreshing. Built over theAuthCoordinator(tokens +resolvedTeamID) inCMUXMobileRootScene.selectReconnectRoutes()(registry-fresh wins, falls back to local when the registry is empty or unreachable), writes fresher routes intoMobilePairedMacStore. The next reconnect trigger then reaches a moved Mac. The connect itself still uses local routes, so the common case has zero added latency.The Mac registers under
MobileHostIdentity.deviceID()and the phone looks up by its storedmac.macDeviceID(the same value from the attach ticket); that id match is the auto-pair linchpin.Deferred to the key-pinning phase
Per-device key pinning and revoke. The phone registering itself as a
devicerow is also deferred (a phone row only matters once it anchors a pinned key);device.idis the clean seam.Verification
tsc --noEmitandeslintclean;devices-route.test.ts(and sibling DB tests, no regression) pass on an isolated test Postgres.CmuxMobileShellpackage builds; 19 tests pass (7 new on the route-selection/parse/identity seams).cmuxFeaturebuilds for the iOS simulator.cmuxapp BUILD SUCCEEDED (Mac client + AppDelegate + test target); pbxproj test-wiring lint passes.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches authenticated cloud APIs and mobile reconnect path (route overwrite), with mitigations (local fallback, lifecycle guards, owner-only POST/DELETE); requires DB migration.
Overview
Adds a team-scoped device registry so phones can discover fresher Mac attach routes on reload without re-scanning a QR, while reconnect still uses local paired-Mac routes first and treats the cloud as a best-effort rendezvous.
Web: New
devices/device_app_instancesschema and migration;/api/deviceswith Stack auth, team scoping (X-Cmux-Team-Id), POST/GET/DELETE, per-team caps, route sanitization, and only the registering user may update or delete a device’s routes. CI runsdevices-route.test.tsin the DB job.Mac:
DeviceRegistryClientPOSTs to the registry whenMobileHostServiceroute ads change (deduped by team + tag + routes, including clearing routes when pairing turns off), wired fromAppDelegate.iOS:
DeviceRegistryService+DeviceRegistryRefreshingread the registry;MobileShellCompositekicks off a non-blocking background refresh on stored-Mac reconnect, appliesselectReconnectRoutes/shouldApplyRegistryRefreshso stale routes update without resurrecting forgotten pairings, and skips ambiguous multi-tag instances.CMUXMobileRootSceneinjects the client from auth.Tests: Swift route-selection/parse/identity and Mac re-register policy tests; web DB tests for registry API behavior.
Reviewed by Cursor Bugbot for commit 17562bc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a team‑scoped device registry so the phone auto‑pairs to its Mac on reload without re‑scanning a QR. Now models device identity per team, avoids cross‑tag route mixing, and limits updates/deletes to the registering user.
New Features
devicesanddevice_app_instancestables; per‑team identity via(team_id, device_uuid);/api/deviceswith POST (idempotent), GET, DELETE; Stack auth; team scoping viaX-Cmux-Team-Idor?teamId=; cap instances per device; boundtag; store only structurally valid routes (drop scalars/arrays; enforceMAX_ROUTES).MobileHostService.statusUpdates()and registers on route changes; skips empty routes; publishes a one‑time empty update on nonempty→empty to clear stale routes; wired inAppDelegate.MobilePairedMacStoreviaselectReconnectRoutes();MobileDeviceIdentitypersists a cmux UUID; safe cross‑tag selection uses registry routes only when exactly one instance advertises any; per‑route failible decoding skips malformed/unknown kinds; scopeddeviceIDand route‑selection helpers ontoDeviceRegistryService(call sites updated, no behavior change).devices-route.test.tsin theweb-db-migrationsjob.20260608055850_device_registry.Bug Fixes
device_not_owned); DELETE is also owner‑scoped (non‑owners are a no‑op).(teamId, tag, routes)so switching teams re‑registers even when routes are unchanged; marks the re‑register policynonisolatedto fix test actor isolation; resolves the team after auth bootstrap before registering to avoid wrong‑team registration at launch (team used in both the dedup key andX-Cmux-Team-Id).Written for commit 17562bc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Chores