feat: add case.dev and Core to /login dropdown#7
Conversation
Register case.dev (device flow) and Core (OAuth device flow) as OAuth providers so they appear in the interactive /login selector dropdown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
DB Cooper Database ReviewNo database-facing changes detected in the provided diff; this PR only adds OAuth provider wiring in the coding agent and does not touch schema, migrations, or queries. No DB risks identified. No database risks found. Migrations and DB-impacting changes look safe. DB review by DB Cooper | Re-run with |
Judicial ReviewAdds case.dev and Core device-flow providers and registers them for /login, but there are a couple of hardcoded/overbroad defaults and some error-path gaps that could mask failures or over-grant access. Findings: 0 critical, 2 warnings, 1 info Security
Recommendation: Make scopes configurable (env/config) and default to the minimum required scope for CLI login; document expected scopes. Error Handling
Recommendation: Handle other non-2xx statuses explicitly with a descriptive error (include status code) or a bounded retry with backoff and a final failure message. Architectureℹ️ INFO in
Recommendation: Centralize provider registration in one place (prefer ModelRegistry lifecycle) and ensure registration is idempotent to avoid duplicate entries after refresh. Legal-Grade review by Thurgood | Re-review with |
Oppenheimer Cleanup ReviewPrimary complexity is duplicated device-flow logic and repeated provider registration, which can be collapsed into shared helpers without changing behavior. No dead code proven; simplification is feasible but should be done with care to keep the login UX consistent. Estimated LOC reduction: ~30 lines 📦 Collapse (2)
Cleanup review by Oppenheimer | Re-run with |
![thurgood-bot[bot]](https://avatars.githubusercontent.com/in/2711760?s=60&v=4){kind=small}
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| scopes: { services: [{ service: "all", scopes: ["read", "write"] }] }, |
There was a problem hiding this comment.
WARNING: Device-flow scope is hardcoded to full read/write for all services, which violates least-privilege and makes access breadth non-configurable.
Suggestion: Make scopes configurable (env/config) and default to the minimum required scope for CLI login; document expected scopes.
| signal: callbacks.signal, | ||
| }); | ||
|
|
||
| if (pollRes.status === 200) { |
There was a problem hiding this comment.
WARNING: Polling ignores non-200/202/429/403/410 responses; server errors or malformed responses will silently loop until timeout, obscuring the real failure state.
Suggestion: Handle other non-2xx statuses explicitly with a descriptive error (include status code) or a bounded retry with backoff and a final failure message.
| @@ -815,6 +817,10 @@ export async function main(args: string[]) { | |||
|
|
|||
There was a problem hiding this comment.
INFO: Provider registration is performed in both main startup and ModelRegistry refresh, creating two registration paths that can drift or duplicate providers depending on registry behavior.
Suggestion: Centralize provider registration in one place (prefer ModelRegistry lifecycle) and ensure registration is idempotent to avoid duplicate entries after refresh.
|
|
||
| const FAR_FUTURE = Date.now() + 365 * 24 * 60 * 60 * 1000; | ||
|
|
||
| interface DeviceFlowStartResponse { |
There was a problem hiding this comment.
WARNING: Device-flow request/response types and polling logic duplicate the CLI login flow, increasing drift risk and maintenance cost. Repo-wide grep shows DeviceFlowStartResponse/DeviceFlowPollResponse/CoreDeviceCodeResponse/CoreTokenResponse appear only here and in packages/coding-agent/src/cli/login.ts, and the auth/cli endpoints are referenced only in those two files.
Suggestion: Collapse by extracting shared device-flow helpers (types + polling) into a small module and have both the OAuth providers and CLI login call it, keeping exact error/timeout behavior.
|
|
||
| const modelRegistry = new ModelRegistry(authStorage, getModelsPath()); | ||
|
|
||
| // Register case.dev and Core as auth providers for /login |
There was a problem hiding this comment.
WARNING: Provider registration is duplicated in main startup and ModelRegistry.refresh, which is safe but repetitive. Repo-wide grep shows casedevOAuthProvider/coreOAuthProvider are only imported in main.ts and packages/coding-agent/src/core/model-registry.ts (besides their definition).
Suggestion: Collapse registration into a single helper (e.g., registerDefaultOAuthProviders) invoked from both the ModelRegistry constructor/refresh or from a shared initializer to remove duplication without behavior change.
![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: 8a02390178
ℹ️ 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 (result.apiKey) { | ||
| return { access: result.apiKey, refresh: "", expires: FAR_FUTURE }; | ||
| } |
There was a problem hiding this comment.
When this provider is selected from interactive /login, AuthStorage.login() stores the returned value as { type: "oauth", access: ... }, but the actual request path still only reads auth.json when data.casedev.type === "api_key" in AuthStorage.getApiKey() and isAuthenticated(). After this flow succeeds, hasAuth("casedev") can mark models available, but getApiKey() returns undefined, so the next model call fails with no API key; the same storage mismatch affects the new Core OAuth token unless the auth storage path is taught to return OAuth access tokens or these flows save the token in the existing API-key slot.
Useful? React with 👍 / 👎.
Thurgood-AI
left a comment
Thurgood-AI
left a comment
There was a problem hiding this comment.
LGTM. Both device flows are well-implemented — proper abort signal handling, backoff for slow_down, correct token lifecycle (case.dev=long-lived key, Core=refresh rotation). Dual registration in main.ts + model-registry refresh() correctly handles the resetOAuthProviders() wipe. ⚖️
2 participants
Summary
/loginselectorrefresh()cycles (login/logout)Test plan
linc, type/login, verify case.dev and Core appear in the dropdown🤖 Generated with Claude Code