fix(oauth): route assistant oauth connect --callback-transport=gateway through daemon IPC#29596
Conversation
…ay` through daemon IPC (#29570) * fix(oauth): route assistant oauth connect --callback-transport=gateway through daemon IPC * fix(oauth-connect): pass lint, type-check, and openapi-check - Sort imports per simple-import-sort/imports rule (autofix) - Add eslint-disable for the intentionally-let resolvedState binding - Narrow OAuthConnectResult union before reading .error (split into two if branches) - Narrow OAuthConnectStatusResponse before reading .error in CLI poll loop - Regenerate openapi.yaml to include the 2 new internal routes * fix: use operationId and structured params for cliIpcCall (Devin/Codex P1 review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: thread grantedScopes through IPC path to avoid empty-scopes inconsistency (Devin review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(oauth): align IPC poll timeout with 5-min loopback OAuth window (Codex review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: credence-the-bot <credence@vellum.ai> Co-authored-by: credence-the-bot[bot] <183148327+credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ansport test and policy tests (#29590) Co-authored-by: credence-the-bot <credence@vellum.ai>
…auth_url (#29593) Co-authored-by: credence-the-bot <credence@vellum.ai>
…3) (#29595) Co-authored-by: credence-the-bot <credence@vellum.ai> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: c10f6e5c50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (opts.browser !== false) { | ||
| await openInHostBrowser(auth_url); | ||
|
|
||
| log.info("Waiting for authorization in browser... (press Ctrl+C to cancel)"); |
There was a problem hiding this comment.
When assistant oauth connect ... --json succeeds via the new IPC path, this unconditional log.info(...) writes a plain-text line to stdout before the JSON payload. Machine consumers that parse stdout as JSON (including scripts relying on a single JSON object) will fail even though the command succeeded. The managed path already guards informational logs behind !jsonMode, so this BYO IPC path should do the same to keep JSON mode output parseable.
Useful? React with 👍 / 👎.
| export function getOAuthConnectState(state: string): OAuthConnectState | null { | ||
| clearExpiredOAuthConnectStates(); | ||
| return activeOAuthConnectFlows.get(state) ?? null; |
There was a problem hiding this comment.
State eviction only runs inside getOAuthConnectState(), so flows that are started but never polled (for example --no-browser usage, abandoned flows, or callers that stop polling) leave entries in activeOAuthConnectFlows indefinitely. Over time, repeated starts can accumulate stale in-memory state in the daemon process. Expiration should be driven independently (e.g., timer/sweeper or cleanup on writes) rather than depending on future status reads.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e on insertion (#29597) * fix: suppress browser-wait log in JSON mode; sweep expired state on insert * test: make JSON-mode log suppression test exercise the actual if (!jsonMode) guard Configure the logger mock to write to process.stdout in the specific test so the test would fail if the guard were removed from connect.ts. Also reset mockLogInfo to no-op in beforeEach for test isolation. --------- Co-authored-by: credence-the-bot <credence@vellum.ai>
…rings (#29603) * fix(oauth): replace 'Daemon' with 'assistant' in user-facing error strings * test: update assertion to match new 'assistant' string from devin nit fix The previous commit replaced 'Daemon' with 'assistant' in user-facing error strings (per Devin review), but missed updating the test that asserts on the old 'Daemon returned unexpected response' wording. --------- Co-authored-by: credence-the-bot <credence@vellum.ai> Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
|
@codex review this PR again — all previously-flagged issues (P1 IPC operationId/params, P2 daemon JSON parse, error string consistency, expired OAuth state sweep) have been addressed in HEAD |
|
@devin review this PR again — all previously-flagged issues (P1 IPC operationId/params, P2 daemon JSON parse, error string consistency, expired OAuth state sweep, 'Daemon' → 'assistant' user-facing strings) have been addressed in HEAD |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f4e672d1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| export const ROUTES: RouteDefinition[] = [ | ||
| { | ||
| operationId: "internal_oauth_connect_start", |
There was a problem hiding this comment.
Registering internal_oauth_connect_start/internal_oauth_connect_status in the shared ROUTES array exposes them to the gateway IPC proxy, but gateway/src/auth/ipc-route-policy.ts has no entries for these operationIds. In tryIpcProxy() (gateway/src/http/routes/ipc-runtime-proxy.ts), missing policy means getIpcRoutePolicy() returns undefined and the request is allowed once JWT auth passes, so an actor token can call /v1/internal/oauth/connect/* by setting X-Vellum-Proxy-Server: ipc, bypassing the daemon-side svc_gateway restriction intended in route-policy.ts.
Useful? React with 👍 / 👎.
Brings in 18 commits since this branch was cut, including: - #29571 (security): IPC policy entries for daemon MCP OAuth routes - Memory v2 work, proactive-artifact, trace-events index, salesforce OAuth, etc. 3-way merge clean (only auto-merged assistant/openapi.yaml).
| summary: "Start daemon-owned OAuth connect flow", | ||
| description: | ||
| "Starts an OAuth connect flow in the daemon and returns the authorization URL for the CLI to open in the browser.", | ||
| tags: ["internal"], | ||
| requestBody: z.object({ | ||
| service: z.string(), | ||
| clientId: z.string(), | ||
| clientSecret: z.string().optional(), | ||
| callbackTransport: z.enum(["loopback", "gateway"]), | ||
| requestedScopes: z.array(z.string()).optional(), | ||
| }), | ||
| handler: handleOAuthConnectStart, | ||
| }, | ||
| { | ||
| operationId: "internal_oauth_connect_status", | ||
| endpoint: "internal/oauth/connect/status/:state", | ||
| method: "GET", | ||
| summary: "Poll daemon OAuth connect flow status", | ||
| description: | ||
| "Returns the current status of an in-flight daemon-owned OAuth connect flow (pending/complete/error).", |
There was a problem hiding this comment.
🚩 Route summary/description use 'daemon' terminology in internal-only routes
The route summary and description fields (lines 126-128, 143-145) use 'daemon-owned' and 'in the daemon'. Per the AGENTS.md user-facing terminology rule, 'daemon' should be 'assistant' in user-facing text. However, these are internal-tagged routes accessible only by svc_gateway principals, and their descriptions appear only in the auto-generated OpenAPI spec. The existing MCP auth routes (openapi.yaml:6724) already use identical 'daemon-owned' phrasing in the same context. This follows the established convention for internal API documentation that no end-user sees, though it's a gray area — the OpenAPI spec could be considered developer documentation.
Was this helpful? React with 👍 or 👎 to provide feedback.
…#29612) Closes Codex P1 on #29596 by adding IPC policy entries for the new daemon OAuth connect routes (`internal_oauth_connect_start`, `internal_oauth_connect_status`). Mirrors the pattern from #29571 for the MCP OAuth routes. 4-line additive change, 6/6 tests pass. Both Codex and Devin re-reviewed clean.
|
@chatgpt-codex-connector review The P1 you flagged on the previous HEAD ( Summary of what changed since your last review:
Current HEAD: @devin-ai-integration please re-review as well — feature branch has been updated. |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@devin-ai-integration review this PR again — the IPC-policy P1 you flagged on |
|
Status update for the morning review (HEAD Resolved on this HEAD:
Gray-area Devin nit on this HEAD (
@chatgpt-codex-connector review Re-tagging Codex for a fresh pass on |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…h-connect-daemon-flow # Conflicts: # assistant/src/runtime/auth/route-policy.ts
|
@chatgpt-codex-connector review Merged main into this branch (HEAD Substantive change vs. previously-clean HEAD
|
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…onnect`
The CLI's BYO OAuth connect path used to fall through to an in-process
`orchestrateOAuthConnect` invocation when the daemon IPC was
unreachable. That fallback was buggy and dead code in practice:
- It re-introduced the heap-split bug for `--callback-transport=gateway`
(platform redirect lands in a different process from the loopback
waiter — see #29596).
- The OAuth tokens it would acquire need the daemon running before
they're usable, so "daemon unreachable" is already a fatal
precondition for the user's flow.
Now we surface a clear "Is the assistant running?" error and exit 1
when the daemon is unreachable, matching the MCP CLI consolidation in
#29484. The daemon-orchestrated IPC path remains the canonical
implementation; nothing changes when the daemon is up.
…onnect` CLI (#29756) * fix(oauth): drop in-process loopback fallback from `assistant oauth connect` The CLI's BYO OAuth connect path used to fall through to an in-process `orchestrateOAuthConnect` invocation when the daemon IPC was unreachable. That fallback was buggy and dead code in practice: - It re-introduced the heap-split bug for `--callback-transport=gateway` (platform redirect lands in a different process from the loopback waiter — see #29596). - The OAuth tokens it would acquire need the daemon running before they're usable, so "daemon unreachable" is already a fatal precondition for the user's flow. Now we surface a clear "Is the assistant running?" error and exit 1 when the daemon is unreachable, matching the MCP CLI consolidation in #29484. The daemon-orchestrated IPC path remains the canonical implementation; nothing changes when the daemon is up. * test(oauth-cli): add daemon-unreachable regression coverage for BYO connect Three new tests in `oauth-cli.test.ts` lock in the post-cleanup invariant that the BYO `oauth connect` flow MUST exit 1 when the daemon is unreachable, never silently fall back to in-process orchestration: 1. ECONNREFUSED → exit 1, error mentions 'Is the assistant running?' 2. 'Unknown method' → exit 1; `orchestrateOAuthConnect` is NOT called 3. Daemon HTTP error (statusCode set) → surfaces error verbatim Tests (2) and (3) explicitly count invocations of the in-process orchestrator mock and assert zero calls — this is the same regression guard pattern PR #29484 used for the MCP CLI consolidation. * fix(oauth-cli): address review feedback Address Codex P1s + Devin findings on PR #29756: 1. **Use 'assistant' instead of 'daemon' in user-facing CLI errors.** Per assistant/AGENTS.md, 'daemon' is an internal implementation detail and CLI output must use 'assistant' for user-facing language. The new error string in connect.ts violated this. 2. **Drop historical-narrative comment block.** AGENTS.md prohibits comments that reference removed code or prior implementations ('We previously...'). Replaced the block with a description of current behavior only — why an unreachable assistant is a fatal precondition, with no reference to the removed loopback fallback or PR history. 3. **Delete 6 tests that pinned the removed in-process fallback.** These tests mocked `mockOrchestrateOAuthConnect` and asserted the orchestrator was called or its result surfaced. With the loopback fallback removed, every CLI path now goes through IPC, so those tests assert removed behavior. The IPC-first path tests (added in the PR's first commit) cover every JSON shape and error case the deleted tests asserted on: - 'BYO mode --no-browser prints auth URL' → 'IPC start + --no-browser without json → prints URL' - 'BYO default calls orchestrator with isInteractive: true' → deleted; orchestrator no longer in CLI heap - 'JSON output for deferred case' → 'IPC start + --no-browser + json → returns deferred JSON' - 'JSON output for completed case' → 'IPC start succeeds + polling returns complete' - 'IPC ok:false → falls back to in-process orchestrator' → opposite is now true; covered by 'IPC ok:false with statusCode → does NOT fall back' - 'orchestrator error propagates correctly' → 'IPC poll returns ok:false with statusCode → breaks early' 18 tests pass (down from 18 surviving + 6 broken = 24 total). * fix(test): update oauth-cli.test.ts assertion to new error wording Sibling test file at assistant/src/__tests__/oauth-cli.test.ts:1288 still asserted on the previous 'Could not reach the assistant daemon' string. Updated to match the new wording in connect.ts (per assistant/AGENTS.md: 'daemon' is internal implementation detail). --------- Co-authored-by: credence-the-bot[bot] <207016121+credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
…30126) Bookmarks routes (#30119/#30122) added new endpoints under /v1/bookmarks but assistant/openapi.yaml was not regenerated, causing the OpenAPI Spec Check job on main to fail. Also adds a SAFE_LINE_PATTERN to the secrets pre-commit hook for YAML schema property declarations (e.g. `clientSecret:` followed by a nested `type: string` on the next line), which exists in the spec since #29596 but was previously unrecognized by the hook. Co-authored-by: Vellum Assistant <assistant@vellum.ai>
1 participant
Summary
Fixes the heap-split bug in
assistant oauth connect <provider> --callback-transport=gatewaywhereregisterPendingCallbackruns in the CLI process heap butconsumeCallbackruns in the daemon heap — so the callback is never found and the flow times out.Root cause: The CLI was calling
orchestrateOAuthConnectlocally (in its own heap). The daemon'sconsumeCallbackreceives the gateway callback in its heap, but can't find the pending entry registered by the CLI.Fix: Route the flow through the daemon via IPC:
POST internal/oauth/connect/start+GET internal/oauth/connect/status/:stateorchestrate the OAuth flow entirely inside the daemon/start, opens the browser, then polls/status/:stateuntil completionChanges
assistant/src/oauth/oauth-connect-state.ts— new in-memory state map (keyed by state token UUID) with TTL cleanupassistant/src/runtime/routes/oauth-connect-routes.ts— two new daemon routesassistant/src/runtime/routes/index.ts— register new routesassistant/src/runtime/auth/route-policy.ts— addsvc_gateway-only policy for new internal endpointsassistant/src/cli/commands/oauth/connect.ts— IPC-first path with poll helper; falls back to in-process when IPC unavailableassistant/src/oauth/connect-orchestrator.ts— threadgrantedScopesthroughonDeferredCompletecallbackTest plan
assistant oauth connect <provider> --callback-transport=gatewaycompletes without heap-split timeout🤖 Generated with Claude Code