feat(desktop): add Anthropic OAuth re-auth flow in model picker#1739
Merged
Kitenite merged 4 commits intosuperset-sh:mainfrom Feb 24, 2026
Merged
feat(desktop): add Anthropic OAuth re-auth flow in model picker#1739Kitenite merged 4 commits intosuperset-sh:mainfrom
Kitenite merged 4 commits intosuperset-sh:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds end-to-end Anthropic OAuth: PKCE session creation and token exchange, ChatService APIs and TRPC auth endpoints, agent credential storage and expiry, a useAnthropicOAuth hook and AnthropicOAuthDialog UI, and provider-based grouping in ModelPicker with per-provider OAuth wiring. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as ModelPicker UI
participant Hook as useAnthropicOAuth
participant API as TRPC Router
participant Service as ChatService
participant Anthropic as Anthropic OAuth
participant Agent as Agent Token Storage
User->>UI: Click "Connect Anthropic"
UI->>Hook: startAnthropicOAuth()
Hook->>API: startAnthropicOAuth()
API->>Service: startAnthropicOAuth()
Service->>Service: create PKCE session (verifier, state, authUrl)
Service-->>API: { url, instructions }
API-->>Hook: authUrl
Hook-->>UI: open AnthropicOAuthDialog(authUrl)
User->>Anthropic: Visit authUrl (browser)
Anthropic-->>User: Authorization code
User->>UI: Paste/enter code
UI->>Hook: onSubmit(code)
Hook->>API: completeAnthropicOAuth({ code })
API->>Service: completeAnthropicOAuth({ code })
Service->>Anthropic: POST token endpoint (code, verifier)
Anthropic-->>Service: tokens (access, refresh, expires_in)
Service->>Agent: setAnthropicOAuthCredentials(tokens)
Service->>Service: clear anthropicAuthSession
Service-->>API: { success, expiresAt }
API-->>Hook: success
Hook-->>UI: close dialog, refresh status
UI->>User: Anthropic models enabled/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/chat/src/host/auth/anthropic/oauth.ts (1)
3-6: Base64-encoding the client ID adds obfuscation, not security.The
CLIENT_IDis a public OAuth client identifier — it's not a secret. The base64 wrapping only makes the code harder to read/audit. Consider using the plaintext UUID directly.Proposed simplification
-const CLIENT_ID = Buffer.from( - "OWQxYzI1MGEtZTYxYi00NGQ5LTg4ZWQtNTk0NGQxOTYyZjVl", - "base64", -).toString("utf8"); +const CLIENT_ID = "9d1c250a-e61b-44d9-88ed-5944d1962f5e";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 3 - 6, The CLIENT_ID constant is unnecessarily base64-encoded; replace the Buffer.from(...,"base64").toString("utf8") expression with the plaintext client ID string (the UUID) so CLIENT_ID is assigned directly as a string literal; update the constant declaration for CLIENT_ID in oauth.ts and remove the base64 decoding logic to simplify and improve readability.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsx (1)
57-70: Consider extracting OAuth state and handlers into a custom hook.The ModelPicker now manages five OAuth state variables and four tRPC mutation/query bindings alongside the model selection logic. Extracting a
useAnthropicOAuthhook would improve readability and make the OAuth flow independently testable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsx` around lines 57 - 70, Extract the OAuth state and handlers into a new custom hook (e.g., useAnthropicOAuth) that encapsulates oauthDialogOpen, oauthUrl, oauthCode, oauthError, hasPendingOAuthSession and the tRPC bindings startAnthropicOAuthMutation, completeAnthropicOAuthMutation, cancelAnthropicOAuthMutation and refetchAnthropicStatus; implement handler functions (startOAuth, completeOAuth, cancelOAuth, setCode, open/close dialog) inside the hook that call the respective mutations/queries and update state, then replace the inline state and mutation usage in ModelPicker with calls to useAnthropicOAuth and consume the returned state and handler functions to keep model selection logic separate and testable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/AnthropicOAuthDialog.tsx`:
- Around line 110-112: The paragraph in the AnthropicOAuthDialog component uses
Markdown backticks which render literally in JSX; update the JSX in
AnthropicOAuthDialog (look for the <p className="text-muted-foreground text-xs">
node) to render inline code using a proper <code> element instead of backtick
characters so the text displays as inline code (`code` or `code#state` should be
rendered as <code>code</code> or <code>code#state</code>); ensure surrounding
spacing and styling remain consistent with the existing className.
In `@packages/chat/src/host/auth/anthropic/oauth.ts`:
- Around line 59-77: The PKCE verifier is being leaked by setting state =
verifier in createAnthropicOAuthSession; generate a separate
cryptographically-random state value (do not reuse verifier) and place that in
the URL params as state while keeping verifier secret, add state to the
AnthropicOAuthSession return type so the session stores it, and update
parseAuthorizationCodeInput to validate the incoming state matches the
session.state before exchanging the code (reject if mismatched). Ensure you use
a secure RNG for state generation and do not change the code_challenge usage.
In `@packages/chat/src/host/chat-service/chat-service.ts`:
- Around line 119-127: The code calls exchangeAnthropicAuthorizationCode and
only persists the access token via setAnthropicAuthToken, discarding the
returned refreshToken; persist the refreshToken so the service can perform
silent refreshes later (e.g., store it on this.anthropicAuthSession or call a
corresponding setter such as setAnthropicRefreshToken), and ensure expiresAt is
tied to the stored tokens; if you intentionally defer storing refresh tokens,
add a clear TODO comment in the exchangeAnthropicAuthorizationCode handling
block explaining why and how refresh will be handled.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsx`:
- Around line 57-70: Extract the OAuth state and handlers into a new custom hook
(e.g., useAnthropicOAuth) that encapsulates oauthDialogOpen, oauthUrl,
oauthCode, oauthError, hasPendingOAuthSession and the tRPC bindings
startAnthropicOAuthMutation, completeAnthropicOAuthMutation,
cancelAnthropicOAuthMutation and refetchAnthropicStatus; implement handler
functions (startOAuth, completeOAuth, cancelOAuth, setCode, open/close dialog)
inside the hook that call the respective mutations/queries and update state,
then replace the inline state and mutation usage in ModelPicker with calls to
useAnthropicOAuth and consume the returned state and handler functions to keep
model selection logic separate and testable.
In `@packages/chat/src/host/auth/anthropic/oauth.ts`:
- Around line 3-6: The CLIENT_ID constant is unnecessarily base64-encoded;
replace the Buffer.from(...,"base64").toString("utf8") expression with the
plaintext client ID string (the UUID) so CLIENT_ID is assigned directly as a
string literal; update the constant declaration for CLIENT_ID in oauth.ts and
remove the base64 decoding logic to simplify and improve readability.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/AnthropicOAuthDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/index.tspackages/chat/src/host/auth/anthropic/index.tspackages/chat/src/host/auth/anthropic/oauth.tspackages/chat/src/host/chat-service/chat-service.tspackages/chat/src/host/router/router.ts
Comment on lines
110
to
112
| <p className="text-muted-foreground text-xs"> | ||
| Accepts either `code` or `code#state`. | ||
| </p> |
Contributor
There was a problem hiding this comment.
Backticks render as literal characters in HTML — use <code> elements instead.
The markdown-style backticks won't render as inline code in JSX. They'll appear as literal ` characters in the UI.
Proposed fix
- <p className="text-muted-foreground text-xs">
- Accepts either `code` or `code#state`.
- </p>
+ <p className="text-muted-foreground text-xs">
+ Accepts either <code>code</code> or <code>code#state</code>.
+ </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/AnthropicOAuthDialog.tsx`
around lines 110 - 112, The paragraph in the AnthropicOAuthDialog component uses
Markdown backticks which render literally in JSX; update the JSX in
AnthropicOAuthDialog (look for the <p className="text-muted-foreground text-xs">
node) to render inline code using a proper <code> element instead of backtick
characters so the text displays as inline code (`code` or `code#state` should be
rendered as <code>code</code> or <code>code#state</code>); ensure surrounding
spacing and styling remain consistent with the existing className.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/ModelProviderGroup.tsx (2)
42-43: Redundantkeyprop onModelSelectorGroup.The parent (
ModelPicker.tsxline 77) already setskey={provider}on<ModelProviderGroup>. The innerkey={provider}on<ModelSelectorGroup>at line 43 has no effect since it's the only root element returned. You can safely remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/ModelProviderGroup.tsx` around lines 42 - 43, Remove the redundant key prop passed to the ModelSelectorGroup inside the ModelProviderGroup component: the parent ModelPicker already sets key={provider} on ModelProviderGroup, so delete key={provider} from the ModelSelectorGroup JSX returned by ModelProviderGroup to avoid a no-op prop and keep the root element unchanged.
55-84: Per-modelproviderToLogocall is redundant within a group.Since
groupModelsByProvidergroups by exactmodel.provider, every model in this group shares the same provider string. SoproviderToLogo(model.provider)on line 56 always equalsgroupLogo(line 34). You can simplify by reusinggroupLogoandisAnthropicProvider:♻️ Suggested simplification
{models.map((model) => { - const logo = providerToLogo(model.provider); - const modelDisabled = - logo === ANTHROPIC_LOGO_PROVIDER && !isAnthropicAuthenticated; + const modelDisabled = + isAnthropicProvider && !isAnthropicAuthenticated; return ( <ModelSelectorItem key={model.id} value={model.id} disabled={modelDisabled} onSelect={() => { onSelectModel(model); onCloseModelSelector(); }} > - {logo === ANTHROPIC_LOGO_PROVIDER ? ( + {isAnthropicProvider ? ( <img alt="Claude" className="size-3" src={claudeIcon} /> ) : ( - <ModelSelectorLogo provider={logo} /> + <ModelSelectorLogo provider={groupLogo} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/ModelProviderGroup.tsx` around lines 55 - 84, The code calls providerToLogo(model.provider) for each model in models.map even though models are grouped by provider; replace the per-model providerToLogo call with the already-computed groupLogo and compute modelDisabled using groupLogo === ANTHROPIC_LOGO_PROVIDER (and isAnthropicAuthenticated) so the logo/disabled logic reuses groupLogo; keep the rest of the JSX (ModelSelectorItem, onSelectModel, onCloseModelSelector, ModelSelectorLogo/claudeIcon, ModelSelectorName) unchanged but reference groupLogo for rendering and disabled checks instead of providerToLogo(model.provider).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts`:
- Around line 62-83: openExternalUrl currently swallows errors and
unconditionally falls back to window.open, so if window.open is blocked the
caller (openOAuthUrl) never sees the failure; change openExternalUrl to detect
when the fallback window.open returns null (or otherwise fails) and throw an
error so callers can handle it. Specifically, inside openExternalUrl (which
calls electronTrpcClient.external.openUrl.mutate) keep the try/catch for the IPC
call, but after calling window.open(url, "_blank") check the return value and if
it's null/undefined or throws, throw a descriptive Error; this ensures
openOAuthUrl's catch can call setOauthError(getErrorMessage(...)) and surface
the failure to the user.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/ModelProviderGroup.tsx`:
- Around line 42-43: Remove the redundant key prop passed to the
ModelSelectorGroup inside the ModelProviderGroup component: the parent
ModelPicker already sets key={provider} on ModelProviderGroup, so delete
key={provider} from the ModelSelectorGroup JSX returned by ModelProviderGroup to
avoid a no-op prop and keep the root element unchanged.
- Around line 55-84: The code calls providerToLogo(model.provider) for each
model in models.map even though models are grouped by provider; replace the
per-model providerToLogo call with the already-computed groupLogo and compute
modelDisabled using groupLogo === ANTHROPIC_LOGO_PROVIDER (and
isAnthropicAuthenticated) so the logo/disabled logic reuses groupLogo; keep the
rest of the JSX (ModelSelectorItem, onSelectModel, onCloseModelSelector,
ModelSelectorLogo/claudeIcon, ModelSelectorName) unchanged but reference
groupLogo for rendering and disabled checks instead of
providerToLogo(model.provider).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/ModelPicker.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/ModelProviderGroup.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/components/AnthropicProviderHeading/AnthropicProviderHeading.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/components/AnthropicProviderHeading/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/ModelProviderGroup/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/utils/groupModelsByProvider/groupModelsByProvider.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/utils/groupModelsByProvider/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/utils/providerToLogo/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/utils/providerToLogo/providerToLogo.ts
...w/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/chat/src/host/chat-service/chat-service.ts (1)
132-136: Unnecessary manual spread — passcredentialsdirectly.
AnthropicOAuthCredentialsfromoauth.ts(required fields) is structurally assignable to the internalAnthropicOAuthCredentialsinsuperagent.ts(optional fields). The spread is redundant.♻️ Proposed simplification
- setAnthropicOAuthCredentials({ - accessToken: credentials.accessToken, - refreshToken: credentials.refreshToken, - expiresAt: credentials.expiresAt, - }); + setAnthropicOAuthCredentials(credentials);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/host/chat-service/chat-service.ts` around lines 132 - 136, The code currently calls setAnthropicOAuthCredentials by constructing a new object with accessToken/refreshToken/expiresAt from credentials; instead, pass the existing credentials object directly (i.e. call setAnthropicOAuthCredentials(credentials)) because the external AnthropicOAuthCredentials type from oauth.ts is structurally compatible with the internal type in superagent.ts, making the manual spread redundant — update the call sites that use setAnthropicOAuthCredentials to pass credentials directly (refer to the setAnthropicOAuthCredentials function and the credentials variable used in this diff).packages/agent/src/superagent.ts (1)
22-26:AnthropicOAuthCredentialsis duplicated with divergent optionality.The same type name is defined here (internal,
refreshToken?/expiresAt?optional) and inpackages/chat/src/host/auth/anthropic/oauth.ts(exported, both required). The two packages are structurally compatible today but will silently diverge if either definition changes. Consider exporting the type fromoauth.tsand importing it here, or moving it to a shared types module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/src/superagent.ts` around lines 22 - 26, The type AnthropicOAuthCredentials is duplicated with different optionality; replace the local definition in superagent.ts with a single source of truth by importing the exported type from packages/chat/src/host/auth/anthropic/oauth.ts (or move it to a shared types module and import from there), update any references in superagent.ts to use the imported AnthropicOAuthCredentials, and remove the local type to prevent future divergence.packages/chat/src/host/auth/anthropic/oauth.ts (3)
33-37: PKCE verifier is at the RFC 7636 minimum length.
randomBytes(32)encodes to exactly 43 base64url characters — the floor of the 43–128 range required by RFC 7636. UsingrandomBytes(33)yields 44 characters and moves off the boundary with negligible cost.♻️ Proposed fix
function generatePKCE(): { verifier: string; challenge: string } { - const verifier = base64Url(randomBytes(32)); + const verifier = base64Url(randomBytes(33)); const challenge = base64Url(createHash("sha256").update(verifier).digest()); return { verifier, challenge }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 33 - 37, The PKCE verifier generated in generatePKCE() uses randomBytes(32) which yields a 43-character base64url string — exactly the RFC 7636 minimum; increase entropy to avoid the boundary by using randomBytes(33) so the verifier encodes to 44 characters. Update the verifier generation inside generatePKCE() to use randomBytes(33) (leave challenge computation with createHash("sha256") as-is) so the produced verifier meets the recommended length safely.
120-133: No timeout on the token exchangefetch— a hanging Anthropic server will block indefinitely.The
fetchtoTOKEN_URLhas no timeout. Add anAbortSignal.timeoutto bound the call.♻️ Proposed fix
const response = await fetch(TOKEN_URL, { method: "POST", headers: { "Content-Type": "application/json", }, + signal: AbortSignal.timeout(15_000), body: JSON.stringify({ grant_type: "authorization_code", client_id: CLIENT_ID, code, state, redirect_uri: REDIRECT_URI, code_verifier: input.verifier, }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 120 - 133, The token exchange fetch to TOKEN_URL can hang indefinitely; modify the fetch call (the POST block that sends grant_type, client_id, code, state, redirect_uri, code_verifier) to include a signal using AbortSignal.timeout(...) (e.g., a sensible default like 10_000 ms or a configurable timeout) so the request is aborted on timeout; ensure the signal is passed in the fetch options and handle the abort/timeout error where this exchange is awaited so it surfaces a clear error instead of hanging.
3-6:CLIENT_IDbase64 encoding is unnecessary obfuscation.This is a public OAuth client ID (PKCE flows have no client secret), so there's nothing to hide. The base64 wrapping adds indirection without benefit and may mislead future readers into treating it as sensitive material.
♻️ Proposed simplification
-const CLIENT_ID = Buffer.from( - "OWQxYzI1MGEtZTYxYi00NGQ5LTg4ZWQtNTk0NGQxOTYyZjVl", - "base64", -).toString("utf8"); +const CLIENT_ID = "9d1c250a-e61b-44d9-88ed-5944d1962f5e";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 3 - 6, Replace the unnecessary base64 decode for CLIENT_ID in oauth.ts with the plain client ID string: remove Buffer.from(...,"base64").toString("utf8") and assign the decoded client ID directly to the CLIENT_ID constant (keeping the same value), leaving the identifier CLIENT_ID intact and removing the obfuscation so future readers know it’s a public OAuth client ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chat/src/host/chat-service/chat-service.ts`:
- Around line 111-139: The completeAnthropicOAuth method can race when called
concurrently because exchangeAnthropicAuthorizationCode is awaited before
clearing this.anthropicAuthSession; to fix, capture the needed session fields
(verifier and state) into local variables, set this.anthropicAuthSession = null
immediately after the TTL check (before any await) to prevent a second caller
from using the same session, then call exchangeAnthropicAuthorizationCode with
the local verifier/state and proceed to call setAnthropicOAuthCredentials and
return the result; keep the existing TTL check using
ChatService.ANTHROPIC_AUTH_SESSION_TTL_MS and ensure no other code path relies
on anthopicAuthSession after it’s nulled.
---
Nitpick comments:
In `@packages/agent/src/superagent.ts`:
- Around line 22-26: The type AnthropicOAuthCredentials is duplicated with
different optionality; replace the local definition in superagent.ts with a
single source of truth by importing the exported type from
packages/chat/src/host/auth/anthropic/oauth.ts (or move it to a shared types
module and import from there), update any references in superagent.ts to use the
imported AnthropicOAuthCredentials, and remove the local type to prevent future
divergence.
In `@packages/chat/src/host/auth/anthropic/oauth.ts`:
- Around line 33-37: The PKCE verifier generated in generatePKCE() uses
randomBytes(32) which yields a 43-character base64url string — exactly the RFC
7636 minimum; increase entropy to avoid the boundary by using randomBytes(33) so
the verifier encodes to 44 characters. Update the verifier generation inside
generatePKCE() to use randomBytes(33) (leave challenge computation with
createHash("sha256") as-is) so the produced verifier meets the recommended
length safely.
- Around line 120-133: The token exchange fetch to TOKEN_URL can hang
indefinitely; modify the fetch call (the POST block that sends grant_type,
client_id, code, state, redirect_uri, code_verifier) to include a signal using
AbortSignal.timeout(...) (e.g., a sensible default like 10_000 ms or a
configurable timeout) so the request is aborted on timeout; ensure the signal is
passed in the fetch options and handle the abort/timeout error where this
exchange is awaited so it surfaces a clear error instead of hanging.
- Around line 3-6: Replace the unnecessary base64 decode for CLIENT_ID in
oauth.ts with the plain client ID string: remove
Buffer.from(...,"base64").toString("utf8") and assign the decoded client ID
directly to the CLIENT_ID constant (keeping the same value), leaving the
identifier CLIENT_ID intact and removing the obfuscation so future readers know
it’s a public OAuth client ID.
In `@packages/chat/src/host/chat-service/chat-service.ts`:
- Around line 132-136: The code currently calls setAnthropicOAuthCredentials by
constructing a new object with accessToken/refreshToken/expiresAt from
credentials; instead, pass the existing credentials object directly (i.e. call
setAnthropicOAuthCredentials(credentials)) because the external
AnthropicOAuthCredentials type from oauth.ts is structurally compatible with the
internal type in superagent.ts, making the manual spread redundant — update the
call sites that use setAnthropicOAuthCredentials to pass credentials directly
(refer to the setAnthropicOAuthCredentials function and the credentials variable
used in this diff).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/AnthropicOAuthDialog.tsxpackages/agent/src/index.tspackages/agent/src/superagent.tspackages/chat/src/host/auth/anthropic/oauth.tspackages/chat/src/host/chat-service/chat-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/components/AnthropicOAuthDialog/AnthropicOAuthDialog.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts (1)
151-176: ExtractmutateAsyncto avoid unnecessary callback re-creations from unstable mutation object.TanStack Query's
useMutationreturns a non-referentially-stable object that changes on every state transition. However,mutateAsyncis designed to be stable across re-renders. By including the entirecancelAnthropicOAuthMutationobject in the dependency array,onOAuthDialogOpenChangerecreates whenever the mutation state changes, even though onlymutateAsyncis used inside. Depend on the extractedmutateAsyncinstead to prevent these unnecessary re-creations.♻️ Proposed refactor
+ const cancelAnthropicOAuthMutateAsync = cancelAnthropicOAuthMutation.mutateAsync; + const onOAuthDialogOpenChange = useCallback( (nextOpen: boolean) => { setOauthDialogOpen(nextOpen); if (nextOpen) return; onModelSelectorOpenChange(true); setOauthCode(""); setOauthError(null); setOauthUrl(null); if (hasPendingOAuthSession) { - void cancelAnthropicOAuthMutation.mutateAsync().catch((error) => { + void cancelAnthropicOAuthMutateAsync().catch((error) => { console.error( "[model-picker] Failed to cancel Anthropic OAuth:", error, ); }); setHasPendingOAuthSession(false); } }, [ - cancelAnthropicOAuthMutation, + cancelAnthropicOAuthMutateAsync, hasPendingOAuthSession, onModelSelectorOpenChange, ], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts` around lines 151 - 176, The onOAuthDialogOpenChange callback depends on the whole cancelAnthropicOAuthMutation object which is unstable; extract mutateAsync from cancelAnthropicOAuthMutation (e.g., const { mutateAsync: cancelAnthropicOAuth } = cancelAnthropicOAuthMutation) and use that stable function inside onOAuthDialogOpenChange, then replace cancelAnthropicOAuthMutation in the useCallback dependency array with the extracted cancelAnthropicOAuth; keep the existing references like hasPendingOAuthSession, setHasPendingOAuthSession, onModelSelectorOpenChange, setOauthCode, setOauthError and setOauthUrl unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts`:
- Around line 132-138: The model selector is opened via
onModelSelectorOpenChange(true) before the refetchAnthropicStatus() completes,
causing components to read a stale isAnthropicAuthenticated value; change the
order so you await refetchAnthropicStatus() first (await
refetchAnthropicStatus()), then call onModelSelectorOpenChange(true), keeping
the other state resets (setHasPendingOAuthSession(false),
setOauthDialogOpen(false), setOauthUrl(null), setOauthCode("")) as-is before or
around the await so the selector only opens after the authentication status is
refreshed.
In `@packages/chat/src/host/auth/anthropic/oauth.ts`:
- Around line 169-173: The expiresAt calculation can be in the past when
data.expires_in < 300; adjust the logic in
packages/chat/src/host/auth/anthropic/oauth.ts so expiresAt is clamped to a
minimum future timestamp. Compute the candidateExpires = Date.now() +
data.expires_in * 1000 - 5 * 60 * 1000 and then set expiresAt =
Math.max(candidateExpires, Date.now() + 1000) (or otherwise ensure
candidateExpires >= Date.now()), referencing the expiresAt return value and
data.expires_in to prevent immediate expiry on malformed/short-lived tokens.
- Around line 129-136: The token exchange is incorrectly including the `state`
parameter in the POST body; per RFC 6749/7636 and Anthropic's token
expectations, remove `state` from the JSON payload sent in the token exchange so
the body contains only grant_type, client_id, code, redirect_uri, and
code_verifier (locate the token exchange block in
packages/chat/src/host/auth/anthropic/oauth.ts where the body is constructed for
the authorization_code exchange).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts`:
- Around line 151-176: The onOAuthDialogOpenChange callback depends on the whole
cancelAnthropicOAuthMutation object which is unstable; extract mutateAsync from
cancelAnthropicOAuthMutation (e.g., const { mutateAsync: cancelAnthropicOAuth }
= cancelAnthropicOAuthMutation) and use that stable function inside
onOAuthDialogOpenChange, then replace cancelAnthropicOAuthMutation in the
useCallback dependency array with the extracted cancelAnthropicOAuth; keep the
existing references like hasPendingOAuthSession, setHasPendingOAuthSession,
onModelSelectorOpenChange, setOauthCode, setOauthError and setOauthUrl
unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.tspackages/chat/src/host/auth/anthropic/oauth.tspackages/chat/src/host/chat-service/chat-service.ts
Comment on lines
+132
to
+138
| await completeAnthropicOAuthMutation.mutateAsync({ code }); | ||
| setHasPendingOAuthSession(false); | ||
| setOauthDialogOpen(false); | ||
| setOauthUrl(null); | ||
| setOauthCode(""); | ||
| onModelSelectorOpenChange(true); | ||
| await refetchAnthropicStatus(); |
Contributor
There was a problem hiding this comment.
Model selector opens before status refetch resolves.
onModelSelectorOpenChange(true) is called on line 137 before await refetchAnthropicStatus(). If the model picker reads isAnthropicAuthenticated immediately on open, it will see the stale false value until the refetch settles, potentially showing a brief "not authenticated" state right after a successful OAuth.
Consider awaiting the refetch before reopening the selector:
♻️ Proposed fix
- onModelSelectorOpenChange(true);
await refetchAnthropicStatus();
+ onModelSelectorOpenChange(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts`
around lines 132 - 138, The model selector is opened via
onModelSelectorOpenChange(true) before the refetchAnthropicStatus() completes,
causing components to read a stale isAnthropicAuthenticated value; change the
order so you await refetchAnthropicStatus() first (await
refetchAnthropicStatus()), then call onModelSelectorOpenChange(true), keeping
the other state resets (setHasPendingOAuthSession(false),
setOauthDialogOpen(false), setOauthUrl(null), setOauthCode("")) as-is before or
around the await so the selector only opens after the authentication status is
refreshed.
Comment on lines
+129
to
+136
| body: JSON.stringify({ | ||
| grant_type: "authorization_code", | ||
| client_id: CLIENT_ID, | ||
| code, | ||
| state, | ||
| redirect_uri: REDIRECT_URI, | ||
| code_verifier: input.verifier, | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Anthropic OAuth token endpoint request parameters documentation
💡 Result:
Anthropic does not appear to publish first-party public docs that list the request parameters for its OAuth token endpoint; their Help Center instead points you to the general Console docs site. [1]
What is publicly observable (from Claude/Anthropic OAuth implementations in the wild) is that the token exchange uses a standard OAuth 2.0 + PKCE shape against:
- Token endpoint:
POST https://console.anthropic.com/v1/oauth/token[2][3]
Authorization code + PKCE token exchange (form-encoded)
Send Content-Type: application/x-www-form-urlencoded with: [2]
grant_type=authorization_codecode(the authorization code)client_idredirect_uricode_verifier(PKCE verifier)
Refresh token exchange
Implementations also use the same token endpoint to refresh, using: [3]
grant_type=refresh_tokenrefresh_tokenclient_id(commonly included)
Where to look (official): Anthropic’s Help Center directs API documentation requests to the Console docs site. [1]
Sources:
[1] Anthropic Help Center: “Where can I find your API documentation?” https://support.anthropic.com/en/articles/8114490-where-can-i-find-your-api-documentation
[2] GitHub issue showing Anthropic token exchange parameters (PKCE) anthropics/claude-code#11814
[3] Example helper code using https://console.anthropic.com/v1/oauth/token for exchange/refresh https://gist.github.com/actuallyepic/6dd18b829e48a5333620ca2f2b22a27c
Remove the state parameter from the token endpoint request body.
Per RFC 6749 (OAuth 2.0) and RFC 7636 (PKCE), state belongs only in the authorization request, not the token exchange. Anthropic's token endpoint does not include state in its observed request parameters (grant_type, code, client_id, redirect_uri, code_verifier). Sending it violates the OAuth 2.0 specification and may cause failures if the endpoint strictly validates parameters.
Current code
body: JSON.stringify({
grant_type: "authorization_code",
client_id: CLIENT_ID,
code,
state,
redirect_uri: REDIRECT_URI,
code_verifier: input.verifier,
}),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 129 - 136, The
token exchange is incorrectly including the `state` parameter in the POST body;
per RFC 6749/7636 and Anthropic's token expectations, remove `state` from the
JSON payload sent in the token exchange so the body contains only grant_type,
client_id, code, redirect_uri, and code_verifier (locate the token exchange
block in packages/chat/src/host/auth/anthropic/oauth.ts where the body is
constructed for the authorization_code exchange).
Comment on lines
+169
to
+173
| return { | ||
| accessToken: data.access_token, | ||
| refreshToken: data.refresh_token, | ||
| expiresAt: Date.now() + data.expires_in * 1000 - 5 * 60 * 1000, | ||
| }; |
Contributor
There was a problem hiding this comment.
expiresAt can be in the past if expires_in < 300.
Date.now() + data.expires_in * 1000 - 5 * 60 * 1000 yields a past timestamp when expires_in is less than 300 seconds. While Anthropic returns standard values (e.g., 3600s), a malformed or unusually short-lived response would cause the token to be treated as immediately expired on the client side.
🛡️ Proposed fix
- expiresAt: Date.now() + data.expires_in * 1000 - 5 * 60 * 1000,
+ expiresAt: Date.now() + Math.max(0, data.expires_in - 300) * 1000,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| expiresAt: Date.now() + data.expires_in * 1000 - 5 * 60 * 1000, | |
| }; | |
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| expiresAt: Date.now() + Math.max(0, data.expires_in - 300) * 1000, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chat/src/host/auth/anthropic/oauth.ts` around lines 169 - 173, The
expiresAt calculation can be in the past when data.expires_in < 300; adjust the
logic in packages/chat/src/host/auth/anthropic/oauth.ts so expiresAt is clamped
to a minimum future timestamp. Compute the candidateExpires = Date.now() +
data.expires_in * 1000 - 5 * 60 * 1000 and then set expiresAt =
Math.max(candidateExpires, Date.now() + 1000) (or otherwise ensure
candidateExpires >= Date.now()), referencing the expiresAt return value and
data.expires_in to prevent immediate expiry on malformed/short-lived tokens.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/chatauthtRPC procedures for Anthropic status/start/complete/cancel and wire chat service methodsAnthropicOAuthDialogusing@superset/uiinput-group componentsValidation
bun run --cwd packages/chat typecheckbun run --cwd apps/desktop typecheckSummary by CodeRabbit