feat(cli): migrate oauth providers + apps + shared to thin IPC wrappers#30246
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <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: 07e42c7653
ℹ️ 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".
| policyKey: "oauth/providers.register", | ||
| summary: "Register OAuth provider", | ||
| description: "Register a new OAuth provider configuration.", | ||
| tags: ["oauth"], | ||
| requirePolicyEnforcement: true, | ||
| responseStatus: "201", |
There was a problem hiding this comment.
These new provider mutation routes rely on policy keys (oauth/providers.register, oauth/providers.update, oauth/providers.delete) that are not registered in assistant/src/runtime/auth/route-policy.ts (which currently only registers oauth/providers for this area). In assistant/src/runtime/http-router.ts, missing policy registrations make enforcePolicy return allow, so these writes bypass the intended scope gate instead of requiring settings.write.
Useful? React with 👍 / 👎.
| operationId: "oauth_apps_upsert", | ||
| endpoint: "oauth/apps/upsert", | ||
| method: "POST", | ||
| policyKey: "oauth/apps.upsert", | ||
| summary: "Upsert OAuth app", |
There was a problem hiding this comment.
The newly added app routes introduce new policy keys/endpoints (oauth/apps.upsert and derived oauth/apps/lookup) but there are no matching entries in assistant/src/runtime/auth/route-policy.ts. Because the router treats missing policies as unprotected, these routes skip normal scope enforcement and can be called without the expected OAuth-settings read/write permissions.
Useful? React with 👍 / 👎.
| let rows: SerializedProvider[] = (r.result?.providers ?? []).map( | ||
| (p: Record<string, unknown>) => ({ | ||
| providerKey: p.provider_key as string, | ||
| displayName: p.display_name as string | null, | ||
| description: p.description as string | null, | ||
| supportsManagedMode: p.supports_managed_mode as boolean, | ||
| managedServiceIsPaid: p.managed_service_is_paid as boolean, | ||
| requiresClientSecret: p.requires_client_secret as boolean, | ||
| logoUrl: p.logo_url as string | null, | ||
| dashboardUrl: p.dashboard_url as string | null, | ||
| clientIdPlaceholder: p.client_id_placeholder as string | null, | ||
| featureFlag: p.feature_flag as string | null, | ||
| }), |
There was a problem hiding this comment.
oauth providers list
This migration changes assistant oauth providers list --json from full provider objects to a reduced summary shape by mapping only a small subset of fields from oauth_providers_get. The previous implementation returned full serialized providers (including fields like auth/token URLs, scopes, and timestamps), and the help text here still advertises those fields, so existing scripts consuming the old JSON contract will silently break.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let rows: SerializedProvider[] = (r.result?.providers ?? []).map( | ||
| (p: Record<string, unknown>) => ({ | ||
| providerKey: p.provider_key as string, | ||
| displayName: p.display_name as string | null, | ||
| description: p.description as string | null, | ||
| supportsManagedMode: p.supports_managed_mode as boolean, | ||
| managedServiceIsPaid: p.managed_service_is_paid as boolean, | ||
| requiresClientSecret: p.requires_client_secret as boolean, | ||
| logoUrl: p.logo_url as string | null, | ||
| dashboardUrl: p.dashboard_url as string | null, | ||
| clientIdPlaceholder: p.client_id_placeholder as string | null, | ||
| featureFlag: p.feature_flag as string | null, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 defaultScopes omitted from providers list CLI mapping, scopes never display
The providers list CLI command maps the IPC response from snake_case to camelCase at providers.ts:284-297, but the mapping does not include defaultScopes. The server-side serializeProviderSummary (provider-serializer.ts:134-149) also does not include a default_scopes field. As a result, formatProviderSummary (providers.ts:63-65) always sees p.defaultScopes as undefined, and the Scopes: ... line is never rendered in text output. In JSON mode, the defaultScopes field is also absent. The old code used serializeProvider (full serialization) which included defaultScopes, so this is a behavioral regression for both text and JSON consumers of assistant oauth providers list.
Prompt for agents
The providers list CLI mapping at providers.ts:284-297 is missing the defaultScopes field. The server route handleListProviders in oauth-providers.ts uses serializeProviderSummary which returns a snake_case summary that does NOT include default_scopes. Two changes are needed:
1. In assistant/src/runtime/routes/oauth-providers.ts, function handleListProviders: either switch from serializeProviderSummary to serializeProviderFull (which includes defaultScopes), or add a default_scopes field to the SerializedProviderSummary type and serializeProviderSummary function in assistant/src/oauth/provider-serializer.ts.
2. In assistant/src/cli/commands/oauth/providers.ts, the snake_case-to-camelCase mapper at line 284 must also include defaultScopes mapped from whatever snake_case key the server provides (e.g. default_scopes).
Without both changes, formatProviderSummary at line 63 will never render the Scopes line because p.defaultScopes is always undefined.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| return { provider: serializeProviderSummary(row) }; | ||
| return { provider: serializeProviderFull(row) }; |
There was a problem hiding this comment.
🚩 providers get route changed from summary to full serialization — API contract change
The handleGetProvider route at assistant/src/runtime/routes/oauth-providers.ts:93 was changed from returning serializeProviderSummary(row) (snake_case summary with ~10 fields) to serializeProviderFull(row) (camelCase full object with ~30+ fields). This is an intentional improvement for the CLI's providers get command (which uses formatProviderDetail expecting camelCase), but it also changes the HTTP API response shape for GET /oauth/providers/:providerKey. Existing HTTP consumers that parsed the snake_case summary will now receive camelCase full data. The CLI is the primary consumer, so this is likely acceptable, but worth confirming no other HTTP callers depend on the old shape.
Was this helpful? React with 👍 or 👎 to provide feedback.
) The CLI IPC refactor PRs (#30238–#30247) added new shared routes without matching policy entries in route-policy.ts. The route-policy coverage guard test fails on main with 29 missing endpoints: - audit, auth/info - conversations/cli/{list,create,export,clear} - inference/send - internal/mcp/{list,add,remove} - oauth/apps.upsert, oauth/apps/lookup - oauth/providers.{register,update,delete} - platform/{status,connect,disconnect,callback-routes/register,callback-routes} - sequences/{list,get,pause,resume,cancel-enrollment,stats,guardrails} - user-routes/{list,inspect} http-router.ts treats unregistered endpoints as unprotected (enforcePolicy no-ops on missing keys), so any authenticated principal could hit these over HTTP without the intended scope/principal-type gates. The IPC dispatch path bypasses enforcePolicy entirely, so the existing CLI flows are unaffected by these additions. Scope/principal assignments mirror sibling routes: - oauth/apps mutations use ACTOR_ENDPOINTS settings.write (matches oauth/apps.create/.delete already present) - oauth/providers mutations use ACTOR_ENDPOINTS settings.write - oauth/apps/lookup uses ACTOR_ENDPOINTS settings.read - auth/info uses ACTOR_ENDPOINTS settings.read - internal/mcp/{list,add,remove} join INTERNAL_ENDPOINTS (gateway-only, internal.write) alongside the existing internal/mcp/reload entry - CLI-driven routes (audit, conversations/cli/*, inference/send, platform/*, sequences/*, user-routes/*) are local-only with settings.read or settings.write, mirroring tasks/*, cache/*, defer/*, domain/*, email/* — destructive 'conversations/cli/clear' elevates to settings.write to match conversations/clear-all - inference/send uses chat.write (LLM dispatch on behalf of caller), aligned with tts/synthesize-cli + stt/transcribe-file Closes route-policy coverage findings on PRs #30241 (sequences, user-routes), #30242 (conversations/cli/clear), #30244 (inference/send), #30245 (internal/mcp), #30246 (oauth/providers, oauth/apps), #30247 (platform/*). Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
1 participant
Part of plan: cli-ipc-migration.md (PR 9 of 11)