feat(account): Clerk identity + ade login (OAuth PKCE loopback)#815
Conversation
Adds an optional, machine-scoped ADE account layer. `ade login` runs an OAuth Authorization-Code + PKCE (S256) flow via the system browser and a 127.0.0.1 loopback; the daemon owns the encrypted session (account.session.v1) and refreshes it (authEpoch-guarded, deduped), so desktop, CLI, and TUI share one login. New `account` daemon action domain (startLogin/pollLogin/status/signOut/getToken); getToken + login/signout are gated CTO-only, `status` is token-free. Projectless `account.call` machine RPC so `ade login`/`ade logout`/`ade auth status` need no project. Local-first is preserved: no local path gains a login gate; login only unlocks reaching remote machines + the account directory. PIN pairing and local `ade code` keep working with zero login. 343 tests, ade-cli + desktop typecheck, built-binary verification green. Manual browser `ade login` smoke pending. Phase 2 of the accounts/machine-directory initiative. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds account OAuth authentication with persisted sessions, shared runtime and desktop action wiring, account RPC authorization, CLI login/logout/status commands, machine-runtime connection handling, and centralized session-role resolution. ChangesAccount authentication
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 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 |
|
@copilot review but do not make fixes |
… (Greptile P1) - multiProjectRpcServer: move cachedRuntimeBuildHash into the handler factory (per-handler memoization) so tests mutating process.argv[1] recompute a fresh hash — fixes the order-dependent test-ade-cli build-hash failure. - multiProjectRpcServer: gate cto-only account actions (getToken/startLogin/ pollLogin/signOut) on account.call, resolving caller role from the initialize identity clamped to the ADE_DEFAULT_ROLE ceiling — mirrors the run_ade_action gate; status stays open. Closes the token-exfiltration bypass Greptile flagged. - runtimeRoles: hoist resolveSessionRole + canDefaultRoleServeRequestedRole out of adeRpcServer so both RPC servers share one role-ceiling source of truth. - cli: ade login/logout connect at cto (their startLogin/signOut are cto-only); wire plan.connectRole through executePlan. auth status (open) unchanged. - Add gate regression tests (status open for agent; getToken/startLogin/signOut rejected for agent + no-identity; getToken allowed for cto). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c8aa8981
ℹ️ 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".
| if (Number.isFinite(deadlineMs) && Date.now() >= deadlineMs) { | ||
| process.stderr.write("ADE account sign-in timed out.\n"); | ||
| const status = await runAccountAction("status"); | ||
| return { output: formatOutput(status, options, "account-auth"), exitCode: 1 }; |
There was a problem hiding this comment.
Cancel pending login on max-wait timeout
When ade login --max-wait expires before the OAuth session's five-minute TTL, this path reports failure but leaves the daemon's loopback login session alive. If the browser tab completes after the CLI has timed out, handleLoopbackRequest can still exchange the code and persist the account session, so a command that exited with a timeout can sign the machine in later. Please cancel the pending login session before returning from this timeout path, or add a service action that closes just this session without signing out an existing account.
Useful? React with 👍 / 👎.
`ade login --max-wait` timing out left the daemon's loopback login session alive, so a browser tab completing after the CLI gave up could still exchange the code and silently sign the machine in. Add a gated `cancelLogin` account action that closes just that pending loopback listener (no signOut, no authEpoch bump, no persisted-session wipe), and call it best-effort on the timeout path. - accountAuthService: cancelLogin(sessionId) → finishPendingSession + delete; wired through domain service, ACCOUNT_ACTION_NAMES, callAccountAction. - registry: cancelLogin is cto-only (ADE_ACTION_CTO_ONLY.account) and allowlisted. - cli: runAccountLogin timeout branch cancels the session before returning. - Tests: cancelLogin closes the loopback + leaves any signed-in account intact; gate rejects cancelLogin for non-cto callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f2fc018da
ℹ️ 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".
| connection = await createConnection( | ||
| { ...options, headless: false, role: "cto" }, | ||
| { autoRegisterProject: false, machineRuntimeOnly: true }, |
There was a problem hiding this comment.
Register the current root for account login config
When ade login is run from a project that has CLERK_ISSUER / CLERK_OAUTH_CLIENT_ID only in that project's ADE secrets and the project has not already been added to the machine registry, this path skips project registration entirely. The daemon-side account.call only registers roots from projectRegistry.list(), and sharedAccountAuthService reads project secrets only from those registered roots before falling back to env, so startLogin reports account login as unconfigured even though the invoking project's secrets exist. Pass or register the current project root for account config before starting login without necessarily adding it to the machine project catalog.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/ade-cli/src/multiProjectRpcServer.test.ts (1)
180-215: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover
pollLoginin the CTO-only regression matrix.The denied-action loop omits
pollLogin, so its authorization policy can regress unnoticed.Proposed test update
- for (const action of ["getToken", "startLogin", "cancelLogin", "signOut"]) { + for (const action of [ + "getToken", + "startLogin", + "pollLogin", + "cancelLogin", + "signOut", + ]) {🤖 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/ade-cli/src/multiProjectRpcServer.test.ts` around lines 180 - 215, Extend the CTO-only denied-action matrix in the “rejects cto-only account actions for a non-cto caller” test to include “pollLogin”. Preserve the existing authorization assertion and verify accountAuthService.pollLogin is not called for the denied action.apps/ade-cli/src/cli.test.ts (1)
2689-2749: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd desktop socket-backed routing coverage.
This only exercises the headless RPC socket. Add coverage confirming how these machine-only account commands behave when a desktop socket is available—particularly that they do not unexpectedly fall back to it.
As per coding guidelines, “For ADE CLI changes, verify both headless mode and the desktop socket-backed ADE RPC path.” <coding_guidelines>
🤖 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/ade-cli/src/cli.test.ts` around lines 2689 - 2749, Extend the account-status routing tests around runCli and startHeadlessRpcSocketServer to cover a desktop socket being available, verifying machine-only account commands remain on the headless/local path and do not fall back to the desktop RPC socket. Assert the desktop socket receives no unexpected requests while preserving the existing signed-out response and exit 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.
Inline comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Line 2692: Remove the duplicate const requests declaration in the test scope,
keeping a single requests variable for the surrounding test logic.
In `@apps/ade-cli/src/cli.ts`:
- Around line 17606-17670: Update the login flow around the startLogin polling
block to track the active sessionId and best-effort cancelLogin from finally on
every unsuccessful exit, including polling and formatting errors. Clear the
pending-session marker only after successful sign-in or after an explicit
timeout cancellation succeeds, while preserving connection closure and avoiding
cancellation errors masking the original result.
In `@apps/ade-cli/src/services/account/accountAuthService.ts`:
- Around line 147-165: Update normalizeOAuthConfig to allow http:// issuers only
when the parsed URL hostname is a loopback or local-development host; require
https:// for all other hosts. Preserve the existing URL validation and error
message behavior, rejecting non-HTTP(S) schemes and non-local plaintext issuers.
In `@apps/ade-cli/src/services/account/sharedAccountAuthService.ts`:
- Around line 41-55: Update resolveOAuthConfig to resolve CLERK_ISSUER and
CLERK_OAUTH_CLIENT_ID atomically from each projectRoot, returning the first
project that provides a complete pair. Only fall back to environment values
after checking project roots, and avoid combining values sourced from different
projects; preserve the existing empty-string defaults for unresolved fields.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 734-759: Add the missing cancelLogin entry to
ADE_ACTION_INPUT_CONTRACTS.account in the action registry, documenting its
no-input contract consistently with the other account actions. Keep the existing
allowlisting and gating unchanged, and update the corresponding registry test
coverage to assert the new contract.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 2689-2749: Extend the account-status routing tests around runCli
and startHeadlessRpcSocketServer to cover a desktop socket being available,
verifying machine-only account commands remain on the headless/local path and do
not fall back to the desktop RPC socket. Assert the desktop socket receives no
unexpected requests while preserving the existing signed-out response and exit
code.
In `@apps/ade-cli/src/multiProjectRpcServer.test.ts`:
- Around line 180-215: Extend the CTO-only denied-action matrix in the “rejects
cto-only account actions for a non-cto caller” test to include “pollLogin”.
Preserve the existing authorization assertion and verify
accountAuthService.pollLogin is not called for the denied action.
🪄 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: 02eb8437-32f5-49dd-8379-49033f11d201
📒 Files selected for processing (12)
apps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/runtimeRoles.tsapps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/sharedAccountAuthService.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.ts
| posixIt("reports signed-out account status over the machine socket in headless mode", async () => { | ||
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "ade-cli-account-status-sock-")); | ||
| const socketPath = path.join(root, "ade.sock"); | ||
| const requests: Array<{ method: string; params?: unknown }> = []; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate requests declaration.
The identical const requests declaration appears twice in the same scope, causing a compile-time redeclaration error.
const requests: Array<{ method: string; params?: unknown }> = [];
-const requests: Array<{ method: string; params?: unknown }> = [];🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/ade-cli/src/cli.test.ts` at line 2692, Remove the duplicate const
requests declaration in the test scope, keeping a single requests variable for
the surrounding test logic.
| account: { | ||
| startLogin: { | ||
| description: "Start the machine-owned ADE account OAuth PKCE login flow.", | ||
| input: "no input", | ||
| example: "ade login", | ||
| }, | ||
| pollLogin: { | ||
| description: "Poll an in-memory ADE account login session.", | ||
| input: "object { sessionId: string }", | ||
| example: "ade actions run account.pollLogin --input-json '{\"sessionId\":\"...\"}'", | ||
| }, | ||
| status: { | ||
| description: "Read the machine-owned ADE account sign-in status without exposing tokens.", | ||
| input: "no input", | ||
| example: "ade auth status --text", | ||
| }, | ||
| signOut: { | ||
| description: "Clear the machine-owned ADE account session.", | ||
| input: "no input", | ||
| example: "ade logout", | ||
| }, | ||
| getToken: { | ||
| description: "Internal bearer-token accessor for ADE remote services; refreshes near expiry.", | ||
| input: "no input", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing cancelLogin entry in ADE_ACTION_INPUT_CONTRACTS.account.
cancelLogin is allowlisted (line 186) and CTO-gated (line 139) but has no documented input contract here, unlike every other account action. See consolidated comment for the full cross-file fix (this file + registry.test.ts).
🤖 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/adeActions/registry.ts` around lines 734 -
759, Add the missing cancelLogin entry to ADE_ACTION_INPUT_CONTRACTS.account in
the action registry, documenting its no-input contract consistently with the
other account actions. Keep the existing allowlisting and gating unchanged, and
update the corresponding registry test coverage to assert the new contract.
CI: update registry.test.ts allowlist assertion for the new cancelLogin action. Review sweep (Codex + CodeRabbit, login-flow class): - cli: cancel the pending loopback session on EVERY unsuccessful login exit (timeout/poll-error/throw) via a tracked sessionId cleared on success, not just the timeout path (CodeRabbit Major, security). - cli + multiProjectRpcServer: pass the invoking project root to startLogin and register it as an account-config source (no projects.add) so `ade login` can read that project's CLERK_* secrets (Codex P2). - sharedAccountAuthService: resolve CLERK issuer+clientId as an atomic pair per project root, never cross-mixing halves from different projects (CodeRabbit Major, functional). New atomic-pair tests. - accountAuthService: require https for CLERK_ISSUER except loopback/local-dev hosts, so a misconfigured http issuer can't leak the code/tokens in plaintext (CodeRabbit Major, security). New issuer-scheme tests. - registry: add cancelLogin input-contract doc entry (CodeRabbit Minor). Skipped: CodeRabbit "duplicate const requests" (cli.test.ts:2692) — false positive, the two declarations are in separate posixIt scopes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef21cbc9f
ℹ️ 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".
| const startProjectRoot = | ||
| typeof startArgs.projectRoot === "string" ? startArgs.projectRoot.trim() : ""; | ||
| if (startProjectRoot) { | ||
| registerAccountConfigProjectRoot(startProjectRoot); |
There was a problem hiding this comment.
Prioritize the current project for OAuth config
When the machine daemon has already registered another project with CLERK_* secrets, this only appends the invoking projectRoot to the shared Set; resolveOAuthConfig later walks that set in insertion order and uses the first root that has either secret. In a multi-project brain, running ade login from project B after project A was registered can therefore build the Clerk authorize URL with project A's issuer/client instead of B's, despite passing projectRoot specifically for the invoking project. Make the current root take precedence for this login or pass its config directly into startLogin.
Useful? React with 👍 / 👎.
…fig (Codex P2)
Completes iteration 3's project-root wiring: registerAccountConfigProjectRoot now
takes a { prioritize } option that re-seats the root at the FRONT of the ordered
config set, and `ade login`'s startLogin path uses it. resolveOAuthConfig walks
the set in insertion order, so without this a project registered earlier (via
registerAccountProjects) would shadow the invoking project's CLERK_* secrets in a
multi-project brain — building the authorize URL with the wrong Clerk app. New
prioritization test proves the invoking root wins over an earlier-registered one.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c8cc2fcb8
ℹ️ 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".
| const config = normalizeOAuthConfig(await args.getOAuthConfig()); | ||
| const token = await postTokenForm({ | ||
| fetchImpl, | ||
| tokenUrl: `${config.issuer}/oauth/token`, |
There was a problem hiding this comment.
Preserve OAuth config per login session
When two ade login flows overlap from different project roots (or the project secrets change while the browser is open), startLogin builds the authorize URL with the config that was active then, but the callback exchange re-reads getOAuthConfig() here. Because account.call startLogin can prioritize a new project root before the first callback arrives, the first authorization code can be sent to a different issuer/client than the one that issued it, causing that otherwise valid login to fail. Store the normalized issuer/clientId on the pending session and use that for the token exchange.
Useful? React with 👍 / 👎.
|
Shipped via /ship. Merged green (squash 4eb8ce8): Greptile P1 (account.call CTO-role bypass) fixed, plus CodeRabbit's cancel-on-every-exit + https-only issuer + atomic issuer/clientId pair, and Codex's config-root registration + invoking-project prioritization. One consciously-deferred nit: Codex #3583624764 (preserve OAuth config per session across two concurrently overlapping |
Phase 2 · Clerk identity +
ade login(OAuth PKCE loopback)Part of "sign in once → see all your machines." Adds an optional, machine-scoped ADE account layer. Local-first is non-negotiable and preserved: no local path gains a login gate;
ade codeand PIN pairing keep working with zero login. Signing in only unlocks reaching remote machines + the account directory.What it does
ade loginruns OAuth Authorization-Code + PKCE (S256) via the system browser and a127.0.0.1:0loopback; hardened callback (constant-timestatecheck, CSP / nosniff / no-store).ade logout+ade auth statustoo.account.session.v1) and refreshes it (2-min skew, deduped,authEpoch-guarded so a signout / newer-login can't be clobbered). Desktop, CLI, and TUI share one login.accountdaemon action domain (startLogin/pollLogin/status/signOut/getToken).getToken+ login/signout are CTO-role-only;statusis token-free — a subagent/external caller cannot exfiltrate the bearer.account.callmachine RPC so login/logout/status need no project.CLERK_ISSUER,CLERK_OAUTH_CLIENT_ID).Testing
343 scoped tests (OAuth/loopback service incl. concurrency + expiry; machine RPC; CLI incl. a socket-backed headless
auth statusproving noprojects.add; action registry). ade-cli + desktop typecheck + built-binary verification green.Pending
A human browser
ade loginsmoke test (the loopback flow needs a real browser) — everything up to the token exchange is unit-covered.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Greptile Summary
This PR adds an optional shared ADE account layer for machine-scoped sign-in. The main changes are:
ade login,ade logout, andade auth statusCLI flows.account.callmachine RPC for account actions.Confidence Score: 5/5
Safe to merge with low risk from the reviewed changes.
The account flows include role gating, loopback callback hardening, cancellation behavior, and broad tests across CLI, machine RPC, auth service, shared config, and desktop registry paths. No blocking correctness or security issues were identified.
No files require special attention.
What T-Rex did
Important Files Changed
account.call, shared account service wiring, config-root registration, and CTO-only account action enforcement.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant CLI as ade CLI participant Daemon as Machine daemon participant Auth as AccountAuthService participant Browser participant Clerk User->>CLI: ade login CLI->>Daemon: "account.call(startLogin, {projectRoot})" Daemon->>Auth: startLogin() Auth-->>Daemon: sessionId + authorizeUrl + expiresAt Daemon-->>CLI: account action result CLI->>Browser: open authorizeUrl Browser->>Clerk: OAuth authorize with PKCE challenge Clerk->>Auth: GET 127.0.0.1 callback(code,state) Auth->>Auth: constant-time state check Auth->>Clerk: token request with code_verifier Clerk-->>Auth: access/refresh tokens Auth->>Auth: persist encrypted session loop until completion or timeout CLI->>Daemon: "account.call(pollLogin, {sessionId})" Daemon->>Auth: pollLogin(sessionId) Auth-->>Daemon: pending or signed_in status Daemon-->>CLI: account action result end CLI-->>User: account-auth status%%{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 User participant CLI as ade CLI participant Daemon as Machine daemon participant Auth as AccountAuthService participant Browser participant Clerk User->>CLI: ade login CLI->>Daemon: "account.call(startLogin, {projectRoot})" Daemon->>Auth: startLogin() Auth-->>Daemon: sessionId + authorizeUrl + expiresAt Daemon-->>CLI: account action result CLI->>Browser: open authorizeUrl Browser->>Clerk: OAuth authorize with PKCE challenge Clerk->>Auth: GET 127.0.0.1 callback(code,state) Auth->>Auth: constant-time state check Auth->>Clerk: token request with code_verifier Clerk-->>Auth: access/refresh tokens Auth->>Auth: persist encrypted session loop until completion or timeout CLI->>Daemon: "account.call(pollLogin, {sessionId})" Daemon->>Auth: pollLogin(sessionId) Auth-->>Daemon: pending or signed_in status Daemon-->>CLI: account action result end CLI-->>User: account-auth statusReviews (5): Last reviewed commit: "ship: iteration 4 — prioritize invoking ..." | Re-trigger Greptile