Skip to content

feat(account): Clerk identity + ade login (OAuth PKCE loopback)#815

Merged
arul28 merged 5 commits into
mainfrom
ade/accounts-identity-81059156
Jul 15, 2026
Merged

feat(account): Clerk identity + ade login (OAuth PKCE loopback)#815
arul28 merged 5 commits into
mainfrom
ade/accounts-identity-81059156

Conversation

@arul28

@arul28 arul28 commented Jul 14, 2026

Copy link
Copy Markdown
Owner

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 code and PIN pairing keep working with zero login. Signing in only unlocks reaching remote machines + the account directory.

What it does

  • ade login runs OAuth Authorization-Code + PKCE (S256) via the system browser and a 127.0.0.1:0 loopback; hardened callback (constant-time state check, CSP / nosniff / no-store). ade logout + ade auth status too.
  • Daemon owns the session (encrypted credential store, key 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.
  • New account daemon action domain (startLogin / pollLogin / status / signOut / getToken). getToken + login/signout are CTO-role-only; status is token-free — a subagent/external caller cannot exfiltrate the bearer.
  • Projectless account.call machine RPC so login/logout/status need no project.
  • Config reads ADE project secrets first, then env fallback (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 status proving no projects.add; action registry). ade-cli + desktop typecheck + built-binary verification green.

Pending

A human browser ade login smoke test (the loopback flow needs a real browser) — everything up to the token exchange is unit-covered.

🤖 Generated with Claude Code

ADE   Open in ADE  ·  ade/accounts-identity-81059156 branch  ·  PR #815

Summary by CodeRabbit

  • New Features

    • Added account sign-in, sign-out, and authentication-status commands.
    • Added browser-based login with completion polling and configurable wait timeouts.
    • Added account actions for runtime integrations, including status checks and token access.
    • Added support for machine-scoped account commands in headless mode.
    • Added role-based access controls for sensitive account actions.
  • Documentation

    • Updated CLI help to describe login, logout, and authentication-status commands.
  • Tests

    • Added coverage for login flows, cancellation, token refresh, authorization rules, and headless account commands.

Greptile Summary

This PR adds an optional shared ADE account layer for machine-scoped sign-in. The main changes are:

  • ade login, ade logout, and ade auth status CLI flows.
  • OAuth PKCE loopback login handled by the daemon.
  • Encrypted shared session storage with refresh and cancellation handling.
  • Projectless account.call machine RPC for account actions.
  • CTO-only role gating for credential-bearing 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.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex executed the requested verification and reported that local artifact references were not uploaded.

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
apps/ade-cli/src/cli.ts Adds account login/logout/status CLI plans, machine-runtime account calls, login polling, and account auth formatting.
apps/ade-cli/src/multiProjectRpcServer.ts Adds projectless account.call, shared account service wiring, config-root registration, and CTO-only account action enforcement.
apps/ade-cli/src/services/account/accountAuthService.ts Implements the shared account auth service, PKCE loopback login, credential persistence, token refresh, and account action adapter.
apps/ade-cli/src/services/account/sharedAccountAuthService.ts Adds singleton shared account auth service backed by encrypted credentials and project/env OAuth config resolution.
apps/desktop/src/main/services/adeActions/registry.ts Registers the account action domain and marks credential-bearing account actions as CTO-only.

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
Loading
%%{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 status
Loading

Reviews (5): Last reviewed commit: "ship: iteration 4 — prioritize invoking ..." | Re-trigger Greptile

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>
@vercel

vercel Bot commented Jul 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview Jul 15, 2026 12:31am

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Account authentication

Layer / File(s) Summary
OAuth account service
apps/ade-cli/src/services/account/accountAuthService.ts, apps/ade-cli/src/services/account/accountAuthService.test.ts
Defines account authentication contracts and implements PKCE login, callback handling, token refresh, persistence, cancellation, sign-out, and account action dispatch with comprehensive tests.
Shared runtime authentication wiring
apps/ade-cli/src/services/account/sharedAccountAuthService.ts, apps/ade-cli/src/bootstrap.ts
Caches account services by secrets directory, resolves OAuth settings from registered project roots or environment variables, and exposes the service on ADE runtimes.
Desktop account action domain
apps/desktop/src/main/services/adeActions/registry.ts, apps/desktop/src/main/services/adeActions/registry.test.ts
Registers account actions, input contracts, allowlists, CTO-only rules, runtime delegation, and corresponding tests.
Account RPC authorization
apps/ade-cli/src/multiProjectRpcServer.ts, apps/ade-cli/src/multiProjectRpcServer.test.ts, apps/ade-cli/src/adeRpcServer.ts, apps/ade-cli/src/runtimeRoles.ts
Adds account.call, account capability advertisement, project-root registration, role-gated dispatch, centralized session-role resolution, and authorization tests.
CLI account commands and connections
apps/ade-cli/src/cli.ts, apps/ade-cli/src/cli.test.ts
Adds login, logout, and account status planning and execution, account input validation, interactive OAuth polling, account formatting, machine-only connection controls, help text, and CLI tests.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • arul28/ADE#247: Both changes modify parseInitializeIdentity in adeRpcServer.ts, though this PR changes role resolution while that PR changes identity fallback fields.

Suggested labels: desktop

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: adding account identity support and ade login via Clerk OAuth PKCE loopback.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade/accounts-identity-81059156

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@arul28

arul28 commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@copilot review but do not make fixes

Comment thread apps/ade-cli/src/multiProjectRpcServer.ts
… (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>
@arul28

arul28 commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/ade-cli/src/cli.ts
Comment on lines +17633 to +17636
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 };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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>
@arul28

arul28 commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/ade-cli/src/cli.ts
Comment on lines +17580 to +17582
connection = await createConnection(
{ ...options, headless: false, role: "cto" },
{ autoRegisterProject: false, machineRuntimeOnly: true },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
apps/ade-cli/src/multiProjectRpcServer.test.ts (1)

180-215: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Cover pollLogin in 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 win

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae1734 and 8f2fc01.

📒 Files selected for processing (12)
  • apps/ade-cli/src/adeRpcServer.ts
  • apps/ade-cli/src/bootstrap.ts
  • apps/ade-cli/src/cli.test.ts
  • apps/ade-cli/src/cli.ts
  • apps/ade-cli/src/multiProjectRpcServer.test.ts
  • apps/ade-cli/src/multiProjectRpcServer.ts
  • apps/ade-cli/src/runtimeRoles.ts
  • apps/ade-cli/src/services/account/accountAuthService.test.ts
  • apps/ade-cli/src/services/account/accountAuthService.ts
  • apps/ade-cli/src/services/account/sharedAccountAuthService.ts
  • apps/desktop/src/main/services/adeActions/registry.test.ts
  • apps/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 }> = [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment thread apps/ade-cli/src/cli.ts
Comment thread apps/ade-cli/src/services/account/accountAuthService.ts
Comment thread apps/ade-cli/src/services/account/sharedAccountAuthService.ts
Comment on lines +734 to +759
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",
},
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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>
@arul28

arul28 commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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>
@arul28

arul28 commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +461 to +464
const config = normalizeOAuthConfig(await args.getOAuthConfig());
const token = await postTokenForm({
fetchImpl,
tokenUrl: `${config.issuer}/oauth/token`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@arul28
arul28 merged commit 4eb8ce8 into main Jul 15, 2026
33 checks passed
@arul28

arul28 commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

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 ade login flows from different projects) — an esoteric multi-config race on a flow already flagged for a pending human browser smoke-test; not a blocker. Filing here for the record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant