refactor(desktop): upgrade mastracode + simplify small-model naming#3517
Merged
refactor(desktop): upgrade mastracode + simplify small-model naming#3517
Conversation
The provider-diagnostics store was fed by callSmallModel's per-attempt reporting, which was removed when small-model tasks moved to direct AI-SDK + mastracode's AuthStorage. Nothing writes to the issue map anymore, so the clearIssue mutation, getStatuses query, and diagnosticStatus plumbing in ModelsSettings were all no-ops. Settings still surfaces "Session expired / Reconnect" via auth-status alone. ProviderIssue type collapsed from 8 codes to just "expired" to match.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes provider diagnostics tracking and legacy small-model provider infrastructure from the desktop app, introduces a new shared Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop as Desktop Chat Service
participant AuthStorage as Auth Storage
participant Anthropic as Anthropic API
participant Cache as Credential Cache
rect rgb(200, 150, 255, 0.5)
Note over Desktop,Cache: OAuth Token Refresh Flow (Expired Credentials)
end
Desktop->>AuthStorage: getApiKey(ANTHROPIC_AUTH_PROVIDER_ID)
activate AuthStorage
AuthStorage->>AuthStorage: Check stored OAuth expiry
alt Token Expired (expiresAt <= Date.now())
AuthStorage->>Anthropic: POST /token/refresh
Anthropic-->>AuthStorage: New access_token, expires_in
AuthStorage->>AuthStorage: Update credential store
AuthStorage->>AuthStorage: reload()
else Token Valid
AuthStorage->>AuthStorage: Return cached access_token
end
AuthStorage-->>Desktop: Updated ClaudeCredentials or null
deactivate AuthStorage
Desktop->>Desktop: Use refreshed credential for AI operations
alt Refresh Failed
Desktop->>Desktop: Log warning, proceed with expired/null state
end
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
69-75:⚠️ Potential issue | 🟠 MajorReinstate package-age security policy in Agent Rules.
The Agent Rules section appears to have dropped the 7-day minimum release-age policy guidance. That creates a supply-chain posture gap for contributors/agents operating from this document. Please restore a clear rule stating installs/updates blocked by age policy must not be bypassed and should be surfaced instead.
Based on learnings: "Global npm, bun, pnpm, and uv configs enforce a 7-day minimum release age; if package install/update/add fails due to version age or blocked scripts, choose an older version or surface the blocked dependency clearly—do not retry, disable the policy, or suggest bypass flags".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 69 - 75, Reinstate a 7-day minimum release-age policy as a new Rule 7 under the "Agent Rules" section in AGENTS.md: state that global npm, bun, pnpm and uv configs enforce a 7-day minimum release age, that installs/updates blocked by this policy must not be bypassed (no retry with disable flags or turning off the policy), and that agents/contributors must either pick an older allowed version or explicitly surface the blocked dependency/version and failure reason; ensure the rule references enforcement channels (e.g., global config) and mandates surfacing the block rather than suggesting bypasses.
🧹 Nitpick comments (5)
packages/chat/src/server/shared/small-model/get-small-model.ts (1)
49-49: Nit:unknown | nullis redundant.
nullis already assignable tounknown, so the return type simplifies tounknown. If the intent is to signal nullability to callers, consider a narrower return type likeLanguageModel | null(imported fromai/@ai-sdk/provider) — that would also let callers drop theascasts when handing the model to Mastra Agent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/shared/small-model/get-small-model.ts` at line 49, The return type of getSmallModel is declared as `unknown | null`, which is redundant; change the signature of the `getSmallModel` function to a single, clearer type — either `unknown` if you want to keep it generic, or preferably a narrower nullable type like `LanguageModel | null` (import `LanguageModel` from your ai/@ai-sdk/provider) so callers don't need `as` casts when passing the model to Mastra Agent; update the function signature `export function getSmallModel(): ...` and add the appropriate import if choosing `LanguageModel | null`.apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.test.ts (1)
3-5: Minor: redundantunknown | nulltype.
unknownalready subsumesnull, so the return annotation can be simplified:-const getSmallModelMock = mock( - (() => null) as (...args: unknown[]) => unknown | null, -); +const getSmallModelMock = mock((() => null) as (...args: unknown[]) => unknown);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.test.ts` around lines 3 - 5, The cast on the IIFE used for getSmallModelMock is overly broad — change the type assertion from (() => null) as (...args: unknown[]) => unknown | null to use unknown only (e.g., (...args: unknown[]) => unknown) so the return type isn't redundantly unioned with null; update the symbol getSmallModelMock (and its mock(...) call) accordingly to remove the unnecessary `| null` from the asserted return type.apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts (1)
119-126: Optional: wrapper is now a thin pass-through.
resolveProviderStatushas collapsed to essentiallyauthStatus ? deriveModelProviderStatus(...) : undefined. Fine to keep as a named seam, but callers inModelsSettings.tsxcould also callderiveModelProviderStatusdirectly now if you'd prefer less indirection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts` around lines 119 - 126, The resolveProviderStatus wrapper is now a thin pass-through around deriveModelProviderStatus; either remove resolveProviderStatus and update its callers (e.g., in ModelsSettings.tsx) to call deriveModelProviderStatus({ providerId, authStatus }) directly, or keep the function but document its intent; if you remove it, replace references to resolveProviderStatus(...) with the direct call and delete the resolveProviderStatus declaration to avoid indirection and dead code.apps/desktop/plans/20260415-v2-host-service-ai-branch-naming.md (1)
11-18: Nit: specify a language on the fenced block.markdownlint flags MD040 here. Use
text(ormermaidif you want to render it as a diagram) so the block passes linting.✏️ Suggested diff
-``` +```text v2 useSubmitWorkspace └─> client.workspaceCreation.generateBranchName.mutate({ projectId, prompt }) └─> generateBranchNameFromPrompt(...) [host-service] └─> getSmallModel() [shared helper] └─> resolveModel(modelId) from mastracode (full auth: API-key + keychain + OAuth middleware)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@apps/desktop/plans/20260415-v2-host-service-ai-branch-naming.mdaround lines
11 - 18, The fenced code block showing the call sequence (the block starting
with "v2 useSubmitWorkspace" and the triple backticks) lacks a language tag and
triggers markdownlint MD040; update its opening fence fromtotext (orapps/desktop/src/shared/ai/provider-status.ts (1)
8-16: Consider removing the unused"add_api_key"remediation variant.The
ProviderRemediationtype defines"add_api_key"alongside"reconnect", but"add_api_key"is never assigned or checked anywhere in the codebase. Only"reconnect"is emitted bygetIssueFromAuthStatusand consumed in ModelsSettings. Narrow the type toProviderRemediation = "reconnect"or removeremediationentirely if it's now deterministic from the issue code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/ai/provider-status.ts` around lines 8 - 16, The ProviderRemediation union includes an unused "add_api_key" variant; update the types to reflect actual usage by changing ProviderRemediation to only "reconnect" (or remove the optional remediation field if you prefer deriving remediation from the code). Specifically, edit the ProviderRemediation type and adjust ProviderIssue if needed so it matches what getIssueFromAuthStatus emits and what ModelsSettings consumes (ensure symbols ProviderRemediation, ProviderIssue, ProviderIssueCode, getIssueFromAuthStatus, and ModelsSettings remain consistent).
🤖 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/lib/trpc/routers/workspaces/utils/ai-name.ts`:
- Around line 40-54: The guard in the workspace title generator currently only
checks generated !== null && generated !== undefined, allowing empty strings to
be treated as valid; update the post-generation check in the
generateTitleFromMessage handling inside the function in ai-name.ts (the block
that calls generateTitleFromMessage) to treat falsy/empty results as failures
(e.g., if (!generated) return null or trim and check length) so empty-string
generations fall through to deriveWorkspaceTitleFromPrompt and behave like
ai-branch-name.ts; ensure the returned shape remains { name: string,
usedPromptFallback: boolean } only when a non-empty name exists and otherwise
return null to trigger fallback in getWorkspaceAutoRenameDecision.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/ai-branch-name.ts`:
- Around line 16-27: sanitizeGeneratedBranchName currently trims
leading/trailing dots/dashes before slicing, so the final .slice(0,
MAX_BRANCH_LENGTH) can reintroduce a trailing '.' or '-' which git rejects;
after you perform the slice, re-run the trailing/leading-trim (the
.replace(/^[-.]+|[-.]+$/g, "") step) and any final forbidden-suffix removal
(e.g., .replace(/\.lock$/g, "")) and ensure you also collapse repeated
dots/dashes again if needed, keeping the function name
sanitizeGeneratedBranchName as the target for the change.
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 498-520: The inline git.raw branch-walk in the generateBranchName
flow should be replaced with the module helper listBranchNames to preserve
correct refname classification; instead of calling ctx.git(...)/git.raw and
post-processing, call listBranchNames(ctx, localProject.repoPath) and assign its
result to existingBranches so branch collision detection uses the safe,
canonical refname parsing implemented by listBranchNames; remove the
try/catch/generic raw parsing block and rely on listBranchNames' error handling
(add a warn inside listBranchNames only if you need the original log).
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 69-75: Reinstate a 7-day minimum release-age policy as a new Rule
7 under the "Agent Rules" section in AGENTS.md: state that global npm, bun, pnpm
and uv configs enforce a 7-day minimum release age, that installs/updates
blocked by this policy must not be bypassed (no retry with disable flags or
turning off the policy), and that agents/contributors must either pick an older
allowed version or explicitly surface the blocked dependency/version and failure
reason; ensure the rule references enforcement channels (e.g., global config)
and mandates surfacing the block rather than suggesting bypasses.
---
Nitpick comments:
In `@apps/desktop/plans/20260415-v2-host-service-ai-branch-naming.md`:
- Around line 11-18: The fenced code block showing the call sequence (the block
starting with "v2 useSubmitWorkspace" and the triple backticks) lacks a language
tag and triggers markdownlint MD040; update its opening fence from ``` to
```text (or ```mermaid if you prefer a diagram) so the block is explicitly typed
and the linter passes, leaving the inner content unchanged.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.test.ts`:
- Around line 3-5: The cast on the IIFE used for getSmallModelMock is overly
broad — change the type assertion from (() => null) as (...args: unknown[]) =>
unknown | null to use unknown only (e.g., (...args: unknown[]) => unknown) so
the return type isn't redundantly unioned with null; update the symbol
getSmallModelMock (and its mock(...) call) accordingly to remove the unnecessary
`| null` from the asserted return type.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts`:
- Around line 119-126: The resolveProviderStatus wrapper is now a thin
pass-through around deriveModelProviderStatus; either remove
resolveProviderStatus and update its callers (e.g., in ModelsSettings.tsx) to
call deriveModelProviderStatus({ providerId, authStatus }) directly, or keep the
function but document its intent; if you remove it, replace references to
resolveProviderStatus(...) with the direct call and delete the
resolveProviderStatus declaration to avoid indirection and dead code.
In `@apps/desktop/src/shared/ai/provider-status.ts`:
- Around line 8-16: The ProviderRemediation union includes an unused
"add_api_key" variant; update the types to reflect actual usage by changing
ProviderRemediation to only "reconnect" (or remove the optional remediation
field if you prefer deriving remediation from the code). Specifically, edit the
ProviderRemediation type and adjust ProviderIssue if needed so it matches what
getIssueFromAuthStatus emits and what ModelsSettings consumes (ensure symbols
ProviderRemediation, ProviderIssue, ProviderIssueCode, getIssueFromAuthStatus,
and ModelsSettings remain consistent).
In `@packages/chat/src/server/shared/small-model/get-small-model.ts`:
- Line 49: The return type of getSmallModel is declared as `unknown | null`,
which is redundant; change the signature of the `getSmallModel` function to a
single, clearer type — either `unknown` if you want to keep it generic, or
preferably a narrower nullable type like `LanguageModel | null` (import
`LanguageModel` from your ai/@ai-sdk/provider) so callers don't need `as` casts
when passing the model to Mastra Agent; update the function signature `export
function getSmallModel(): ...` and add the appropriate import if choosing
`LanguageModel | null`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b8b5f01-7796-49f6-8604-8dda6c31eff3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
AGENTS.mdapps/desktop/package.jsonapps/desktop/plans/20260415-v2-host-service-ai-branch-naming.mdapps/desktop/runtime-dependencies.tsapps/desktop/src/lib/ai/call-small-model.test.tsapps/desktop/src/lib/ai/call-small-model.tsapps/desktop/src/lib/ai/provider-diagnostics.tsapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/model-providers/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/ai-branch-name.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.tsapps/desktop/src/renderer/components/Chat/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.tsapps/desktop/src/renderer/components/Chat/ChatInterface/components/ModelPicker/hooks/useOpenAIOAuth/useOpenAIOAuth.tsapps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.tsapps/desktop/src/shared/ai/provider-status.test.tsapps/desktop/src/shared/ai/provider-status.tsbunfig.tomlpackages/chat/package.jsonpackages/chat/src/server/desktop/index.tspackages/chat/src/server/desktop/small-model/index.tspackages/chat/src/server/desktop/small-model/small-model.test.tspackages/chat/src/server/desktop/small-model/small-model.tspackages/chat/src/server/desktop/title-generation/index.tspackages/chat/src/server/desktop/title-generation/title-generation.test.tspackages/chat/src/server/desktop/title-generation/title-generation.tspackages/chat/src/server/shared/index.tspackages/chat/src/server/shared/small-model/get-small-model.tspackages/chat/src/server/shared/small-model/index.tspackages/host-service/package.jsonpackages/host-service/src/trpc/router/workspace-creation/utils/ai-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
💤 Files with no reviewable changes (12)
- bunfig.toml
- packages/chat/src/server/desktop/title-generation/title-generation.test.ts
- apps/desktop/src/renderer/components/Chat/ChatInterface/components/ModelPicker/hooks/useAnthropicOAuth/useAnthropicOAuth.ts
- packages/chat/src/server/desktop/small-model/index.ts
- apps/desktop/src/lib/ai/call-small-model.test.ts
- apps/desktop/src/lib/trpc/routers/model-providers/index.ts
- packages/chat/src/server/desktop/small-model/small-model.test.ts
- packages/chat/src/server/desktop/title-generation/title-generation.ts
- apps/desktop/src/lib/ai/provider-diagnostics.ts
- packages/chat/src/server/desktop/small-model/small-model.ts
- apps/desktop/src/lib/trpc/routers/index.ts
- apps/desktop/src/lib/ai/call-small-model.ts
Greptile SummaryThis PR upgrades Key changes:
Confidence Score: 4/5Safe to merge — all typecheck and targeted tests pass, documented regression is acceptable, no critical logic bugs found. The refactor is well-executed: dead code is cleanly removed, new helper is simple and easy to reason about, and the OAuth regression is explicitly documented and scoped to small-model tasks only. All P2 findings are minor cleanup items (stale type member, missing test file, dead branch in getStatusBadge, exported-but-unused function). Score is 4 rather than 5 because the new get-small-model.ts has no test coverage, which is a gap worth closing before callers multiply. packages/chat/src/server/shared/small-model/get-small-model.ts (missing co-located test), apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.ts (stale type member)
|
| Filename | Overview |
|---|---|
| packages/chat/src/server/shared/small-model/get-small-model.ts | New shared helper that resolves an AI-SDK LanguageModel for small-model tasks. Clean, synchronous, API-key-only (documented OAuth regression). No co-located test file despite project conventions. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.ts | Simplified workspace naming; uses getSmallModel() instead of callSmallModel(). Stale "missing-credentials" union member left in WorkspaceAutoRenameResult type that is never returned by the implementation. |
| apps/desktop/src/shared/ai/provider-status.ts | Simplified: ProviderIssueCode narrowed to "expired" only, ProviderDiagnostic removed, deriveModelProviderStatus now only takes authStatus. Logic is clean and tests cover the updated paths. |
| apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx | Removes all provider-diagnostics queries and clearIssue calls; settings UI now driven by auth status alone. Clean removal with no functional regressions for normal users. |
| packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts | Adds dormant generateBranchName mutation (protectedProcedure) for v2 wiring. Branch listing and sanitization logic is solid; returns null gracefully when no credentials or git errors occur. |
| packages/host-service/src/trpc/router/workspace-creation/utils/ai-branch-name.ts | New host-service branch naming utility; uses local sanitizeGeneratedBranchName (duplicating some desktop logic per plan doc). Correct error handling and null fallback. |
| bunfig.toml | Removes minimumReleaseAge = 604800 (7-day guard) to allow mastracode 0.14.0 install. Security policy also removed from AGENTS.md. Intentional but weakens supply-chain posture. |
| apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts | resolveProviderStatus simplified to derive from authStatus only. getStatusBadge retains a dead else-branch (status.issue without code "expired" can never occur with narrowed ProviderIssueCode). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Branch naming / Workspace title\n(v1 desktop or host-service)"] --> B["getSmallModel()"]
B --> C{Env ANTHROPIC_API_KEY?}
C -- yes --> D["createAnthropic(key)\nclaude-haiku-4-5-20251001"]
C -- no --> E{Stored Anthropic key\nvia getStoredApiKey?}
E -- yes --> D
E -- no --> F{Env OPENAI_API_KEY?}
F -- yes --> G["createOpenAI(key).chat\ngpt-4o-mini"]
F -- no --> H{Stored OpenAI key?}
H -- yes --> G
H -- no --> I["return null\n(fallback to random/prompt-derived name)"]
D --> J["generateTitleFromMessage\nvia Mastra Agent"]
G --> J
J --> K["sanitize + dedup branch name"]
K --> L["Return branch/workspace name"]
I --> M["OAuth-only users: random branch name\n(documented regression)"]
Comments Outside Diff (2)
-
apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.ts, line 19 (link)Stale dead type member
"missing-credentials"is listed in theWorkspaceAutoRenameResult.reasonunion but is never returned byattemptWorkspaceAutoRenameFromPromptin the new implementation. The oldcallSmallModel-based path returned this reason when no provider had credentials; the replacement usesgenerateWorkspaceNameFromPromptwhich falls straight to"generation-failed"whengeneratedNameisnull.None of the callers (
workspace-init.ts,create.ts) switch onreason, so there is no runtime impact — but the stale member will confuse future readers who search for where it is produced.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.ts Line: 19 Comment: **Stale dead type member** `"missing-credentials"` is listed in the `WorkspaceAutoRenameResult.reason` union but is never returned by `attemptWorkspaceAutoRenameFromPrompt` in the new implementation. The old `callSmallModel`-based path returned this reason when no provider had credentials; the replacement uses `generateWorkspaceNameFromPrompt` which falls straight to `"generation-failed"` when `generatedName` is `null`. None of the callers (`workspace-init.ts`, `create.ts`) switch on `reason`, so there is no runtime impact — but the stale member will confuse future readers who search for where it is produced. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts, line 107-112 (link)ProviderIssueCodewas narrowedProviderIssueCodeis now"expired"only, so the second guard at line 110 (if (status.issue)) can never be reached — any non-nullstatus.issuealready hascode === "expired"and is returned on line 107–109.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts Line: 107-112 Comment: **Dead else-branch after `ProviderIssueCode` was narrowed** `ProviderIssueCode` is now `"expired"` only, so the second guard at line 110 (`if (status.issue)`) can never be reached — any non-null `status.issue` already has `code === "expired"` and is returned on line 107–109. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-name.ts
Line: 19
Comment:
**Stale dead type member**
`"missing-credentials"` is listed in the `WorkspaceAutoRenameResult.reason` union but is never returned by `attemptWorkspaceAutoRenameFromPrompt` in the new implementation. The old `callSmallModel`-based path returned this reason when no provider had credentials; the replacement uses `generateWorkspaceNameFromPrompt` which falls straight to `"generation-failed"` when `generatedName` is `null`.
None of the callers (`workspace-init.ts`, `create.ts`) switch on `reason`, so there is no runtime impact — but the stale member will confuse future readers who search for where it is produced.
```suggestion
| "empty-prompt"
| "generation-failed"
| "missing-workspace"
| "empty-generated-name"
| "workspace-deleting"
| "workspace-named"
| "workspace-name-changed";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/chat/src/server/shared/small-model/get-small-model.ts
Line: 73-87
Comment:
**Exported but unconsumed production API**
`hasSmallModelCredentials` is exported from `packages/chat/src/server/shared/index.ts` but is not imported by any production code — only by the test mock in `ai-name.test.ts` (which mocks the whole module). If this function is not yet needed, exporting it from the public barrel adds surface area that may mislead future readers.
Consider removing it from `shared/index.ts` until a real consumer exists, or add a comment indicating it is reserved for the upcoming Step 2 / host-service work.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/chat/src/server/shared/small-model/get-small-model.ts
Line: 1-10
Comment:
**Missing co-located test file**
Project conventions (AGENTS.md) require tests to live next to the file they cover (`get-small-model.test.ts` in the same directory). The deleted `small-model.test.ts` had substantial coverage of credential resolution, provider selection, and OAuth paths; the replacement `get-small-model.ts` has no test file.
The key behaviours worth exercising: (a) Anthropic key from env wins over stored, (b) falls back to OpenAI when only `OPENAI_API_KEY` is set, (c) returns `null` when `safeAuthStorage` throws, (d) `hasSmallModelCredentials` correctness.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts
Line: 107-112
Comment:
**Dead else-branch after `ProviderIssueCode` was narrowed**
`ProviderIssueCode` is now `"expired"` only, so the second guard at line 110 (`if (status.issue)`) can never be reached — any non-null `status.issue` already has `code === "expired"` and is returned on line 107–109.
```suggestion
if (status.issue?.code === "expired") {
return { label: "Expired", variant: "destructive" };
}
if (status.connectionState === "connected") {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(desktop): remove dead provider-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-branch-name.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/ai-branch-name.ts:62">
P2: This introduces a functional regression for OAuth-only users: branch-name generation exits early when `getSmallModel()` is unavailable, instead of using the previous OAuth-capable generation path.</violation>
</file>
<file name="packages/host-service/src/trpc/router/workspace-creation/utils/ai-branch-name.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/utils/ai-branch-name.ts:26">
P2: Truncating after validation can produce Git-invalid branch names (e.g., trailing `.` or `.lock`). Re-run the suffix cleanup after slicing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Anthropic credentials were read via authStorage.get() everywhere, so mastracode's built-in refresh flow never ran. Once the 1-hour access token expired, status flipped to "Reconnect" and users had to do a full PKCE re-auth, even though a valid refresh token was already stored. Resolvers now call authStorage.getApiKey() for oauth creds on expiry, which triggers refreshToken() and persists the refreshed credential. getAnthropicAuthStatus does the same before declaring issue: "expired". Mirrors the pattern already used for OpenAI small-model auth.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/chat/src/server/desktop/auth/anthropic/anthropic.ts (1)
231-231: Nit: redundant?? nullfallback.
getCredentialsFromAuthStorage()is typedPromise<ClaudeCredentials | null>, sostorageCredentialis neverundefined. The?? nullcoalesce is a no-op and can be dropped.💡 Suggested change
- firstExpired ??= storageCredential ?? null; + firstExpired ??= storageCredential;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/desktop/auth/anthropic/anthropic.ts` at line 231, Redundant null coalescing: in the assignment that sets firstExpired (referencing firstExpired and storageCredential returned by getCredentialsFromAuthStorage()), remove the unnecessary "?? null" because storageCredential is typed as ClaudeCredentials | null (never undefined); simply assign storageCredential to firstExpired (e.g., set firstExpired = storageCredential) or use the existing null-aware pattern without the redundant coalesce.packages/chat/src/server/desktop/chat-service/chat-service.ts (1)
95-101: Consider logging refresh failures for auth diagnostics.The silent catch swallows every failure path of
authStorage.getApiKey(network errors, revoked refresh token, malformed stored credential). Given this PR's stated goal is to fix the "Reconnect despite valid refresh token" regression, surfacing refresh failures through the existinglogAuthResolutionchannel would make future OAuth regressions far easier to diagnose without changing behavior.💡 Suggested change
) { try { await authStorage.getApiKey(ANTHROPIC_AUTH_PROVIDER_ID); authStorage.reload(); storedCredential = authStorage.get(ANTHROPIC_AUTH_PROVIDER_ID); - } catch { - // Refresh failed; fall through to expired-state handling below. + } catch (error) { + // Refresh failed; fall through to expired-state handling below. + this.logAuthResolution("anthropic", { + event: "oauth-refresh-failed", + error: error instanceof Error ? error.message : String(error), + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/desktop/chat-service/chat-service.ts` around lines 95 - 101, Wrap the empty catch so it logs the refresh failure via the existing logAuthResolution path: when calling authStorage.getApiKey(ANTHROPIC_AUTH_PROVIDER_ID) in the try block, catch the error as e and call logAuthResolution(ANTHROPIC_AUTH_PROVIDER_ID, { status: "refresh_failed", error: String(e) }) (or similar existing logAuthResolution signature) before falling through to expired-state handling; keep behavior unchanged otherwise and reference authStorage.reload() and storedCredential assignment as the subsequent steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/chat/src/server/desktop/auth/anthropic/anthropic.ts`:
- Line 231: Redundant null coalescing: in the assignment that sets firstExpired
(referencing firstExpired and storageCredential returned by
getCredentialsFromAuthStorage()), remove the unnecessary "?? null" because
storageCredential is typed as ClaudeCredentials | null (never undefined); simply
assign storageCredential to firstExpired (e.g., set firstExpired =
storageCredential) or use the existing null-aware pattern without the redundant
coalesce.
In `@packages/chat/src/server/desktop/chat-service/chat-service.ts`:
- Around line 95-101: Wrap the empty catch so it logs the refresh failure via
the existing logAuthResolution path: when calling
authStorage.getApiKey(ANTHROPIC_AUTH_PROVIDER_ID) in the try block, catch the
error as e and call logAuthResolution(ANTHROPIC_AUTH_PROVIDER_ID, { status:
"refresh_failed", error: String(e) }) (or similar existing logAuthResolution
signature) before falling through to expired-state handling; keep behavior
unchanged otherwise and reference authStorage.reload() and storedCredential
assignment as the subsequent steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4652fb9f-e054-4c58-b8e3-4953334293aa
📒 Files selected for processing (5)
packages/chat/src/server/desktop/auth/anthropic/anthropic.tspackages/chat/src/server/desktop/chat-service/chat-service.test.tspackages/chat/src/server/desktop/chat-service/chat-service.tspackages/host-service/src/providers/model-providers/LocalModelProvider/LocalModelProvider.tspackages/host-service/src/providers/model-providers/LocalModelProvider/utils/resolveAnthropicCredential.ts
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/chat/src/server/desktop/chat-service/chat-service.ts">
<violation number="1" location="packages/chat/src/server/desktop/chat-service/chat-service.ts:99">
P2: Do not silently swallow OAuth refresh failures; log context in the catch block so expired-state fallbacks remain diagnosable.
(Based on your team's feedback about handling async failures explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/providers/model-providers/LocalModelProvider/utils/resolveAnthropicCredential.ts">
<violation number="1" location="packages/host-service/src/providers/model-providers/LocalModelProvider/utils/resolveAnthropicCredential.ts:122">
P1: Do not return `kind: "oauth"` when an expired token cannot be refreshed to a valid access token; return `null` so callers can fall back correctly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- host-service ai-branch-name: run trailing-trim after slice so a 100-char truncation can't re-introduce a bare "." or "-" that git rejects as an invalid ref (coderabbit / cubic #2, #7). - host-service workspace-creation.generateBranchName: reuse the existing listBranchNames helper instead of the inline git walk, which classified off the short refname and could conflate a local "origin/foo" with refs/remotes/origin/foo (coderabbit #3). - packages/chat shared/small-model: drop the unused hasSmallModelCredentials export; only a test mock consumed it (greptile #4). - resolveAnthropicCredential: on refresh failure, return null instead of kind:"oauth" with a stale expiresAt so callers fall back cleanly (cubic #8). - chat-service.getAnthropicAuthStatus: log context when refresh throws instead of silently swallowing (cubic #9).
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx:199">
P2: The new `!action` early return hides the provider action for connected API-key/env states, removing the prior Connect fallback and making OAuth connection unavailable from this card in those states.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Active providers now always show a Logout button. Clears OAuth or API key depending on authMethod — no more "Active" badge with no way to disconnect.
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts:146">
P2: `connected` now maps to Logout even for `authMethod: "env"`, but the logout handler only clears OAuth/API-key auth. This exposes a Logout action that cannot disconnect env-backed providers.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx:242">
P1: Handle errors in the new async disconnect click handler to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silently unhandled failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| isStartingOAuth: isStartingAnthropicOAuth, | ||
| canDisconnect: anthropicOAuthDialog.canDisconnect, | ||
| onDisconnect: anthropicOAuthDialog.onDisconnect, | ||
| onDisconnect: async () => { |
Contributor
There was a problem hiding this comment.
P1: Handle errors in the new async disconnect click handler to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silently unhandled failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx, line 242:
<comment>Handle errors in the new async disconnect click handler to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls/errors explicitly and avoiding silently unhandled failures.) </comment>
<file context>
@@ -239,7 +239,15 @@ export function ModelsSettings({ visibleItems }: ModelsSettingsProps) {
startOAuth: startAnthropicOAuth,
isStartingOAuth: isStartingAnthropicOAuth,
- onDisconnect: anthropicOAuthDialog.onDisconnect,
+ onDisconnect: async () => {
+ if (anthropicStatus?.authMethod === "oauth") {
+ anthropicOAuthDialog.onDisconnect();
</file context>
| if (status.issue?.remediation === "reconnect") { | ||
| return { kind: "reconnect" }; | ||
| } | ||
| if (status.connectionState === "connected") { |
Contributor
There was a problem hiding this comment.
P2: connected now maps to Logout even for authMethod: "env", but the logout handler only clears OAuth/API-key auth. This exposes a Logout action that cannot disconnect env-backed providers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/utils.ts, line 146:
<comment>`connected` now maps to Logout even for `authMethod: "env"`, but the logout handler only clears OAuth/API-key auth. This exposes a Logout action that cannot disconnect env-backed providers.</comment>
<file context>
@@ -143,8 +143,8 @@ export function getProviderAction(
return { kind: "reconnect" };
}
- if (status.authMethod === "oauth") {
+ if (status.connectionState === "connected") {
return { kind: "logout" };
}
</file context>
Match Anthropic dialog's layout: remove the raw OAuth URL display and "Tip" block, auto-open the browser on OAuth start. Change "Back" to "Cancel" for consistency.
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Extract shared OAuthDialog component with provider config object. AnthropicOAuthDialog and OpenAIOAuthDialog become thin wrappers that pass provider-specific labels and options.
Each provider section now renders AccountCard + ConfigRow inside one rounded card with a divider, instead of two separate cards. Removes the standalone "API Keys" collapsible section.
OAuth row is now a single inline row (label + status + action) instead of a stacked AccountCard. Both providers share the same 2-row card layout: OAuth row + API key row with divider.
Connect is now primary (filled). Save only shows when there's input. Clear only shows when a key is saved. Removes visual noise from empty-state provider cards.
…nnected providers
This was referenced Apr 18, 2026
Merged
6 tasks
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
2 participants
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
mastracode0.9.2 → 0.14.0 (@mastra/core1.16 → 1.25 transitively).callSmallModel/SmallModelProviderorchestration with a ~60 LOCgetSmallModel()helper that uses@ai-sdk/*directly +createAuthStorage().getStoredApiKeyfor credential resolution.workspaceCreation.generateBranchNamehost-service procedure for future v2 wiring (not yet consumed).Net: ~640 lines removed, ~1246 added (most additions are in the
bun.lockregen).Packaging / infra changes
bunfig.toml— removedminimumReleaseAge = 604800so 0.14.0 installs.apps/desktop/package.json—devscript getsNODE_OPTIONS=--max-old-space-size=8192(the upgraded@mastra/coretree pushed electron-vite bundling over the default heap).apps/desktop/runtime-dependencies.ts— externalizemastracodein the main rollup config (transitively pulls@mastra/fastembed→onnxruntime-nodewith an un-bundleable dynamic require; also cut a 20 MB chunk down to 1.2 MB).@mastra/core@1.25.0as a direct dep ofapps/desktopandpackages/chatso it's hoisted into top-levelnode_modules(Bun's isolated linker only symlinks direct deps, andmastracodedynamically imports@mastra/core/agent).Behavior changes
getSmallModelhelper. Same user-visible flow.authStorage.getApiKey()instead of falling back to Reconnect on expiry (from fix(auth): auto-refresh expired Anthropic OAuth tokens #3510). Applies to chat + host-service runtime-env provisioning.createMastraCode.Test plan
bun run typecheck— 25/25bun run lint:fix— cleanai-name,chat-service,auth/*,provider-status,trpc/service) — all passexpires), send a chat message — should silently refresh instead of showing ReconnectSummary by CodeRabbit
New Features
Improvements