feat(account): ship production machine publishing and paired ADE Code remote#827
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
@copilot review but do not make fixes |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe pull request adds production account-directory deployment configuration, machine registration and pairing across CLI and desktop, bounded remote connection cancellation, multi-issuer webhook verification, CI coverage, documentation updates, and iOS release entitlements. ChangesAccount machine discovery and pairing
Bounded remote launching
Secondary Clerk relay verification
Deployment and platform configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.ts (1)
553-564: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLoop keeps opening new endpoints after cancellation is requested.
Once
options.signalaborts mid-loop, the.catchswallows the abort error andcontinues, so every remaining candidate inaccountRelayEndpointsstill gets a freshopenSyncEnvelopeConnectionattempt (which constructs a real WebSocket before its internal abort check runs) instead of stopping immediately. This defeats the purpose of the bounded-cancellation signal for multi-endpoint account pairing.🔌 Proposed fix: stop the loop once cancellation is requested
const failures: string[] = []; for (const endpoint of accountRelayEndpoints) { + if (options.signal?.aborted) break; const connection = await openSyncEnvelopeConnection({ endpoint, connectTimeoutMs: options.connectTimeoutMs, signal: options.signal, createWebSocket: options.createWebSocket, }).catch((error) => { failures.push(error instanceof Error ? error.message : String(error)); return null; });Also applies to: 632-637
🤖 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 `@apps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.ts` around lines 553 - 564, Stop the endpoint iteration immediately when cancellation is requested: update the connection failure handling in the loop around openSyncEnvelopeConnection, including the analogous block near the second referenced location, to detect an aborted options.signal and exit before continuing to another endpoint. Preserve recording non-cancellation failures and existing retry behavior when the signal is not aborted.apps/desktop/src/shared/accountDirectory.ts (1)
291-298: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDistinguish cancellation from timeout in the error message.
If the caller cancels the request via the provided
args.signal,controller.signal.abortedbecomes true, and this block currently surfaces a misleading "Machine directory timed out" message. Prioritize a check forargs.signal?.abortedto accurately report cancellations.💡 Proposed fix
} catch { return { state: "unavailable", machines: [], - message: controller.signal.aborted - ? "Machine directory timed out." - : "Couldn't reach the machine directory.", + message: args.signal?.aborted + ? "Machine directory request was cancelled." + : controller.signal.aborted + ? "Machine directory timed out." + : "Couldn't reach the machine directory.", };🤖 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 `@apps/desktop/src/shared/accountDirectory.ts` around lines 291 - 298, Update the catch block in the account-directory request flow to check args.signal?.aborted before controller.signal.aborted, returning a cancellation-specific message when the caller aborted the request and retaining the timeout message only for controller-triggered aborts.apps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.ts (1)
234-248: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCheck
signal.abortedbefore creating the WebSocket, not after.
new WebSocket(endpoint)(or the custom factory) runs before the abort check, so an already-cancelled call still opens a connection attempt just to immediately close it. This partially defeats the point of the new cancellation support, especially for callers likepairWithAccountMachinethat loop over multiple relay endpoints under one shared signal — each remaining iteration after cancellation still spins up and tears down a socket instead of short-circuiting.🔧 Proposed fix: reorder the abort check ahead of WebSocket creation
const endpoint = normalizeSyncEndpoint(options.endpoint); const timeoutMs = normalizeTimeout(options.connectTimeoutMs, DEFAULT_CONNECT_TIMEOUT_MS); - const ws = options.createWebSocket?.(endpoint) ?? new WebSocket(endpoint); - const connection = createConnection(endpoint, ws); - if (options.signal?.aborted) { - connection.close(1000, "Sync connection cancelled."); throw options.signal.reason instanceof Error ? options.signal.reason : new Error("Sync connection cancelled."); } + const ws = options.createWebSocket?.(endpoint) ?? new WebSocket(endpoint); + const connection = createConnection(endpoint, ws); if (ws.readyState === WebSocket.OPEN) return connection;🤖 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 `@apps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.ts` around lines 234 - 248, Move the existing options.signal?.aborted check in openSyncEnvelopeConnection before invoking createWebSocket or new WebSocket, and throw the same abort reason without creating a connection. Keep endpoint normalization and timeout setup as needed, while preserving normal connection creation and handling for non-aborted signals.
🤖 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 `@apps/ade-cli/src/multiProjectRpcServer.ts`:
- Around line 803-806: Use the shared account-directory resolution path for both
affected call sites: update multiProjectRpcServer.ts lines 803-806 to include
the project-secret ADE_ACCOUNT_DIRECTORY_URL tier, and update remoteLauncher.ts
lines 1265-1268 to pass the same directory-base resolver instead of relying on
the service default. Ensure account.listMachines/pairMachine, ade code remote,
and ade login resolve the same project directory.
In `@apps/desktop/src/main/services/account/accountBridge.ts`:
- Around line 175-185: Update the machine-listing flow around
directoryService().listMachines() to apply the existing AccountAuthStatus source
guard, allowing sessions with status.source equal to "env-token" to proceed
instead of returning a false signed_out result. Reuse the typed
AccountAuthStatus.source value directly without adding a cast, while preserving
the existing unavailable warning and auth_expired response handling.
---
Outside diff comments:
In `@apps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.ts`:
- Around line 553-564: Stop the endpoint iteration immediately when cancellation
is requested: update the connection failure handling in the loop around
openSyncEnvelopeConnection, including the analogous block near the second
referenced location, to detect an aborted options.signal and exit before
continuing to another endpoint. Preserve recording non-cancellation failures and
existing retry behavior when the signal is not aborted.
In `@apps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.ts`:
- Around line 234-248: Move the existing options.signal?.aborted check in
openSyncEnvelopeConnection before invoking createWebSocket or new WebSocket, and
throw the same abort reason without creating a connection. Keep endpoint
normalization and timeout setup as needed, while preserving normal connection
creation and handling for non-aborted signals.
In `@apps/desktop/src/shared/accountDirectory.ts`:
- Around line 291-298: Update the catch block in the account-directory request
flow to check args.signal?.aborted before controller.signal.aborted, returning a
cancellation-specific message when the caller aborted the request and retaining
the timeout message only for controller-triggered aborts.
🪄 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: CHILL
Plan: Pro
Run ID: 264d06c8-bb1e-453a-a8a1-8f3bf3c5d71c
⛔ Files ignored due to path filters (7)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (38)
.github/workflows/ci.ymlapps/account-directory/README.mdapps/account-directory/package.jsonapps/account-directory/wrangler.jsoncapps/ade-cli/README.mdapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/account/sharedAccountAuthService.test.tsapps/ade-cli/src/services/account/sharedAccountAuthService.tsapps/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.test.tsapps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/remoteTargets/AccountMachineRow.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.account.test.tsapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.tsapps/desktop/src/renderer/webclient/account/client.test.tsapps/desktop/src/renderer/webclient/account/client.tsapps/desktop/src/shared/accountDirectory.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/account.tsapps/ios/ADE/ADE.Release.entitlementsapps/ios/ADE/Services/AccountDirectory.swiftapps/webhook-relay/README.mdapps/webhook-relay/src/relay.tsapps/webhook-relay/test/account.test.ts
|
@codex review |
![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: 0cc683f6d8
ℹ️ 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".
| 26e2ed03a0fd341ce244fc2cd86d9103d0ad348a:apps/desktop/src/shared/accountDirectory.ts:generic-api-key:21 | ||
| 26e2ed03a0fd341ce244fc2cd86d9103d0ad348a:apps/desktop/src/shared/accountDirectory.ts:generic-api-key:26 |
There was a problem hiding this comment.
Checked the CI secret-scan job in .github/workflows/ci.yml: it runs gitleaks on the checked-out branch. These allowlist fingerprints are for commit 26e2ed03..., but this squashed commit adds the same Clerk OAuth client IDs at apps/desktop/src/shared/accountDirectory.ts:21 and :26 under c0b1fe3d...; the existing allowlist format includes the commit hash, so the new commit will still report both generic-api-key findings and block CI unless the c0b1 fingerprints are allowlisted too.
Useful? React with 👍 / 👎.
| const matches = listed.machines.filter((machine) => | ||
| accountMachineMatchesRemoteTarget(machine, target), | ||
| ); | ||
| if (matches.length !== 1) return target; |
There was a problem hiding this comment.
When the directory returns ok, a zero-match case (for example the account machine was deleted/expired) or a multi-match ambiguity for an isUnverifiedLegacyAccountCandidate(target) reaches this return and the caller later opens the saved target through SSH. That bypasses the fail-closed behavior above, so legacy account-created targets whose directory record is missing or ambiguous can silently downgrade to SSH instead of being rejected.
Useful? React with 👍 / 👎.
| // A relay-only online machine has no direct SSH route, so it must not sit in | ||
| // AVAILABLE as a dead (unconnectable) row. | ||
| if (machine.online && accountMachineHasDirectRoute(machine)) { | ||
| if (accountMachineConnectionState(machine) === "available") { |
There was a problem hiding this comment.
In desktop environments using a self-hosted relay via ADE_TUNNEL_RELAY_URL, account machines publish relay URLs for that host and pairMachine accepts them through defaultRelayUrl(), but this renderer-side call uses only the compiled default relay allowlist. Relay-only machines on the configured relay are therefore placed in UNAVAILABLE with no Connect button even though the main pairing path can connect.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
No beta or release was built.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR ships production account identity and paired account-machine remote support. The main changes are:
Confidence Score: 5/5
This PR appears safe to merge.
No accepted bug findings remain. The reviewed account bearer destinations, paired endpoint identity checks, secondary Clerk verification, legacy SSH fallback, cancellation, and publisher disposal paths have explicit validation in the changed code.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Brain as ADE brain participant Auth as Shared account auth participant Dir as Account directory Worker participant Desktop as Desktop/ADE Code client participant Host as Paired runtime host Brain->>Auth: read signed-in account token Brain->>Brain: build validated machine registration Brain->>Dir: POST /account/machines/register Desktop->>Auth: get account token in main/CLI Desktop->>Dir: GET /account/machines Dir-->>Desktop: online machines + verified endpoints Desktop->>Host: WSS account hello with DPoP proof Host-->>Desktop: hello_ok + accountPairing Desktop->>Desktop: save paired credentials Desktop->>Host: open runtime RPC channel%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Brain as ADE brain participant Auth as Shared account auth participant Dir as Account directory Worker participant Desktop as Desktop/ADE Code client participant Host as Paired runtime host Brain->>Auth: read signed-in account token Brain->>Brain: build validated machine registration Brain->>Dir: POST /account/machines/register Desktop->>Auth: get account token in main/CLI Desktop->>Dir: GET /account/machines Dir-->>Desktop: online machines + verified endpoints Desktop->>Host: WSS account hello with DPoP proof Host-->>Desktop: hello_ok + accountPairing Desktop->>Desktop: save paired credentials Desktop->>Host: open runtime RPC channelReviews (2): Last reviewed commit: "ship: iteration 1 - fix secret-scan and ..." | Re-trigger Greptile