Upstream sync: Claude adapter + plan consumption + fixes (v0.0.13)#26
Merged
aaditagrawal merged 15 commits intomainfrom Mar 20, 2026
Merged
Upstream sync: Claude adapter + plan consumption + fixes (v0.0.13)#26aaditagrawal merged 15 commits intomainfrom
aaditagrawal merged 15 commits intomainfrom
Conversation
Preparatory rename for upstream sync. Upstream renamed the provider kind from "claudeCode" to "claudeAgent" and type names accordingly. This rename reduces future sync friction. - Provider kind: "claudeCode" → "claudeAgent" - ClaudeCodeProviderStartOptions → ClaudeProviderStartOptions - ClaudeCodeModelOptions → ClaudeModelOptions - ClaudeCodeAdapter → ClaudeAdapter (files + class) - ClaudeCodeTraitsPicker → ClaudeTraitsPicker
Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: JustYannicc <52761674+JustYannicc@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughRenames Claude provider from Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Composer UI
participant Store as Draft Store
participant Server as Orchestration Server
participant Adapter as ClaudeAdapter
participant SDK as Claude SDK
User->>UI: select model + set modelOptions (claudeAgent)
UI->>Store: setModelOptions(threadId, { claudeAgent: {...} })
UI->>Server: POST thread.turn.start (maybe sourceProposedPlan)
Server->>Server: validate sourceProposedPlan & compute runtime inputs
Server->>Adapter: ensureSessionForThread / startSession/sendTurn (modelOptions, resumeCursor)
Adapter->>SDK: query({ effort, fastMode, thinking })
SDK-->>Adapter: stream messages/events
Adapter->>Server: emit ProviderRuntimeEvent (turn.started/completed, approvals, user-input, progress)
Server->>Server: on turn.started -> mark source proposed plan implemented (implementedAt, implementationThreadId)
Server-->>UI: updated turn + plan state
UI->>User: render updated plan/turn status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/server/src/provider/Layers/ProviderService.ts (2)
242-250:⚠️ Potential issue | 🟠 MajorPersist
modelOptionson every session rebind.These branches call
upsertSessionBinding(...)withoutextra.modelOptions. SinceProviderSessionitself does not carrymodelOptions, the rewrittenruntimePayloaddrops them after the first recover/stop cycle, and the next resume falls back to default provider settings instead of the persisted Claude/Codex tuning.Also applies to: 290-296, 594-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 242 - 250, The upsertSessionBinding calls (e.g., the call around readPersistedProviderOptions and input.binding.runtimePayload) omit passing persisted modelOptions, causing ProviderSession to lose model tuning on rebind; update all upsertSessionBinding calls (including the instances referenced and similar calls at the other locations) to include extra.modelOptions populated from the persisted source (e.g., readPersistedProviderOptions or a readPersistedModelOptions helper) when present so that the call becomes upsertSessionBinding(..., existingProviderOptions ? { providerOptions: existingProviderOptions, modelOptions: existingProviderOptions.modelOptions } : undefined) (or equivalent extraction of modelOptions) to ensure modelOptions survive recover/stop cycles.
347-369:⚠️ Potential issue | 🟠 Major
startSessionrestores the persisted cursor, but not the persisted model options.When
startSession(...)is called for an existing binding, onlyresumeCursoris recovered. If the caller omitsmodelOptions, the adapter restarts with defaults and this upsert immediately overwrites the stored options withundefined.Suggested patch
const input = { ...parsed, threadId, provider: parsed.provider ?? "codex", }; const persistedBinding = Option.getOrUndefined(yield* directory.getBinding(threadId)); + const persistedModelOptions = + persistedBinding?.provider === input.provider + ? readPersistedModelOptions(persistedBinding.runtimePayload) + : undefined; + const effectiveModelOptions = input.modelOptions ?? persistedModelOptions; const effectiveResumeCursor = input.resumeCursor ?? (persistedBinding?.provider === input.provider ? persistedBinding.resumeCursor : undefined); const adapter = yield* registry.getByProvider(input.provider); const session = yield* adapter.startSession({ ...input, + ...(effectiveModelOptions !== undefined + ? { modelOptions: effectiveModelOptions } + : {}), ...(effectiveResumeCursor !== undefined ? { resumeCursor: effectiveResumeCursor } : {}), }); if (session.provider !== adapter.provider) { return yield* toValidationError( @@ yield* upsertSessionBinding(session, threadId, { - modelOptions: input.modelOptions, + modelOptions: effectiveModelOptions, providerOptions: input.providerOptions, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 347 - 369, The code restores only resumeCursor from persistedBinding but not modelOptions/providerOptions, causing undefined to overwrite stored options; update ProviderService.startSession to derive effectiveModelOptions and effectiveProviderOptions (e.g., const effectiveModelOptions = input.modelOptions ?? persistedBinding?.modelOptions and similarly for providerOptions), pass those effective values into adapter.startSession and into the upsertSessionBinding call (use effectiveModelOptions/effectiveProviderOptions instead of input.modelOptions/input.providerOptions) so existing persisted options are preserved when the caller omits them.apps/web/src/lib/threadProvider.ts (1)
5-14:⚠️ Potential issue | 🟠 MajorKeep a legacy alias for
claudeCodein provider parsing.Dropping
claudeCodefrom recognized provider names can break inference for pre-rename persisted session data. Add a compatibility mapping intoProviderKind.🛠️ Suggested compatibility bridge
export function toProviderKind(providerName: string | null | undefined): ProviderKind | null { if (!providerName) return null; + if (providerName === "claudeCode") return "claudeAgent"; return PROVIDER_KIND_SET.has(providerName as ProviderKind) ? (providerName as ProviderKind) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/threadProvider.ts` around lines 5 - 14, Add a compatibility mapping so legacy provider name "claudeCode" is treated as "claudeAgent" during parsing: update the provider parsing function (toProviderKind) to normalize incoming strings by mapping "claudeCode" → "claudeAgent" before validating against PROVIDER_KINDS, ensuring old persisted session data still resolves to the new ProviderKind; keep the rest of the validation logic unchanged so unrecognized names still error/return undefined as before.apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts (1)
511-524:⚠️ Potential issue | 🟠 MajorDon't bake an unsupported Claude model into this expectation.
This test now accepts
claudeAgentstarting withgpt-5-codex, but the provider's model list only includes Claude models. That masks a real provider/model mismatch instead of catching it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts` around lines 511 - 524, The test expectation is asserting the model "gpt-5-codex" for a claudeAgent which is invalid; update the assertion so the mocked startSession call expects a Claude model (e.g., "claude-2" or whichever model is defined in the provider's model list) instead of "gpt-5-codex" — locate the assertion checking harness.startSession.mock.calls[0]?.[1] and replace the hard-coded "gpt-5-codex" with the correct Claude model or a lookup from the provider config so the test reflects a supported provider/model pair.
🧹 Nitpick comments (12)
apps/server/src/provider/Layers/ProviderService.test.ts (2)
587-608: Remove the duplicatedclaudeAgentrouting test.This block is identical to the test on Lines 524-545, including the same title. Keeping both only slows the suite and makes failures harder to attribute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderService.test.ts` around lines 587 - 608, The test duplicated the "routes explicit claudeAgent provider session starts to the claude adapter" case; remove the redundant test block that calls ProviderService.startSession with provider "claudeAgent" (the one asserting routing.claude.startSession.mock.calls.length and validating startInput.cwd and startPayload.provider) so only the original test remains; ensure you delete the duplicate it.effect test entirely (including its Effect.gen closure and assertions) and keep the other identical test unchanged.
873-887: Replace the fixed sleeps with an explicit readiness signal.These waits still rely on timing instead of knowing when the
PubSubsubscription is live. On a loaded CI runner,50msis still enough to intermittently miss the first emitted event.Also applies to: 912-912, 976-976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderService.test.ts` around lines 873 - 887, Replace the timing-based sleeps with an explicit readiness handshake: change the test to await a promise that resolves when the PubSub subscription is actually registered before calling fanout.codex.emit. Concretely, modify the subscription setup used by ProviderService tests to return (or set) a "ready" promise/resolve callback when the handler is attached, then in the test await that readiness promise instead of yield* sleep(50) before invoking fanout.codex.emit (and do the same for the other two occurrences).packages/contracts/src/model.ts (1)
238-238: Keep the new git-model default out ofpackages/contracts.
DEFAULT_GIT_TEXT_GENERATION_MODELadds runtime configuration to a package that is supposed to stay schema/type-only. This default should live in the consuming runtime package instead of being exported from contracts.As per coding guidelines, "Keep
packages/contractspackage schema-only — include no runtime logic, only effect/Schema schemas and TypeScript contracts for provider events, WebSocket protocol, and model/session types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/model.ts` at line 238, Remove the runtime default from the contracts package: delete the exported constant DEFAULT_GIT_TEXT_GENERATION_MODEL from packages/contracts/src/model.ts so contracts remain schema/type-only, and instead define that default in the consuming runtime package (where configuration/implementation lives). Update any imports that currently reference DEFAULT_GIT_TEXT_GENERATION_MODEL to point to the new runtime location (or to use a literal/default there) so packages/contracts only contains types/schemas (e.g., leave model types/interfaces in model.ts but move the actual string default into the runtime codebase).apps/server/src/wsServer.ts (1)
986-988:claudeAgentusage endpoint returns a stub - is this intentional?The handler returns only
{ provider }without actual usage data. Other providers likecopilot,codex, andcursorhave full implementations. If the Claude Agent SDK provides usage/quota information, consider implementing it here for parity. If not available, the stub is acceptable but a brief comment would clarify intent.📝 Optional: Add clarifying comment
if (provider === "claudeAgent") { + // Claude Agent SDK does not expose usage metrics; return minimal stub return { provider } satisfies ProviderUsageResult; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/wsServer.ts` around lines 986 - 988, The branch handling provider === "claudeAgent" returns only a stub ({ provider } satisfies ProviderUsageResult) without usage/quota data; either implement actual usage retrieval using the Claude Agent SDK (call its usage/quota methods and populate the ProviderUsageResult fields like usage, quota, remaining, etc., then return that object) inside the same conditional, or add a clear inline comment above the clause explaining that the Claude Agent SDK does not expose usage/quota and this stub is intentional; update the return to match the shape returned for other providers (e.g., copilot, codex, cursor) so callers receive a consistent ProviderUsageResult.apps/web/src/appSettings.test.ts (1)
209-215: Remove duplicated provider-custom-model test block.This assertion is already covered in Line [173]-Line [179]. Keeping both adds redundant maintenance without extra signal.
♻️ Suggested cleanup
-describe("provider-specific custom models", () => { - it("includes provider-specific custom slugs in non-codex model lists", () => { - const claudeOptions = getAppModelOptions("claudeAgent", ["claude/custom-opus"]); - - expect(claudeOptions.some((option) => option.slug === "claude/custom-opus")).toBe(true); - }); -});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.test.ts` around lines 209 - 215, Remove the duplicate test block that asserts provider-specific custom slugs are included: delete the describe/it block calling getAppModelOptions("claudeAgent", ["claude/custom-opus"]) and its expect assertion because the same assertion is already covered earlier; ensure no other unique assertions in that block are lost and run tests after removal to confirm coverage (look for getAppModelOptions and the "provider-specific custom models" test title to locate the duplicate).apps/web/src/index.css (1)
421-482: Add reduced-motion fallback for animated ultrathink styles.These infinite animations should be disabled for users with
prefers-reduced-motion.♿ Suggested accessibility fallback
`@keyframes` ultrathink-chroma-shift { @@ } +@media (prefers-reduced-motion: reduce) { + .ultrathink-frame, + .ultrathink-chroma, + .ultrathink-pill, + .ultrathink-word { + animation: none !important; + } +} + .ultrathink-frame {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/index.css` around lines 421 - 482, Add a prefers-reduced-motion fallback that disables the infinite animations and motion-based effects for the ultrathink styles: inside a `@media` (prefers-reduced-motion: reduce) block override the animations for ultrathink-rainbow and ultrathink-chroma (and any animated properties on .ultrathink-frame, .ultrathink-chroma, .ultrathink-pill, .ultrathink-word) to none (or effectively zero duration) and freeze animated properties like background-position and filter to a sensible static value so the gradient/chroma remain visible but not animated.apps/web/src/components/chat/ProviderModelPicker.tsx (2)
180-186: Unused type guard can be removed.The
isAvailableProviderOptiontype guard is defined but never called in this file. Consider removing it to reduce dead code, or use it to filterAVAILABLE_PROVIDER_OPTIONSif type narrowing is needed elsewhere.🧹 Suggested removal
-function isAvailableProviderOption(option: (typeof PROVIDER_OPTIONS)[number]): option is { - value: ProviderKind; - label: string; - available: true; -} { - return option.available; -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ProviderModelPicker.tsx` around lines 180 - 186, The function isAvailableProviderOption is an unused type guard; remove this dead code or use it when constructing AVAILABLE_PROVIDER_OPTIONS to narrow types. Either delete the isAvailableProviderOption function definition entirely (and remove any imports referencing it) or apply it to filter PROVIDER_OPTIONS into AVAILABLE_PROVIDER_OPTIONS so the resulting array is correctly typed; update any usages of AVAILABLE_PROVIDER_OPTIONS (and references to PROVIDER_OPTIONS) to reflect the narrowed type if you choose to keep the guard.
269-280: UnusedhandleModelChangefunction can be removed.This function is defined but never called. The component uses inline
onModelSelectcallbacks within the map iterations instead. Consider removing to eliminate dead code.🧹 Suggested removal
- const handleModelChange = (provider: ProviderKind, value: string) => { - if (props.disabled) return; - if (!value) return; - const resolvedModel = resolveModelForProviderPicker( - provider, - value, - props.modelOptionsByProvider[provider], - ); - if (!resolvedModel) return; - props.onProviderModelChange(provider, resolvedModel); - setIsMenuOpen(false); - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ProviderModelPicker.tsx` around lines 269 - 280, Remove the dead helper function handleModelChange since it is defined but never used; locate the function named handleModelChange in ProviderModelPicker and delete it along with any unused imports/closures it references (e.g., resolveModelForProviderPicker, props.onProviderModelChange, setIsMenuOpen) or, if you prefer to keep behavior, replace its single-use callers with calls to the existing inline onModelSelect handlers instead; ensure no references to handleModelChange remain and run type checks to remove any leftover unused symbol warnings.apps/server/src/provider/Layers/ProviderHealth.ts (1)
282-301: Consider extracting a shared command runner to reduce duplication.
runClaudeCommandis nearly identical torunCodexCommand(lines 261-280) andrunGeminiCommand(lines 431-450). While the duplication is acceptable for clarity, a parameterized helper could reduce maintenance burden.♻️ Optional: Extract shared command runner
const makeCommandRunner = (binary: string) => (args: ReadonlyArray<string>) => Effect.gen(function* () { const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; const command = ChildProcess.make(binary, [...args], { shell: process.platform === "win32", }); const child = yield* spawner.spawn(command); const [stdout, stderr, exitCode] = yield* Effect.all( [ collectStreamAsString(child.stdout), collectStreamAsString(child.stderr), child.exitCode.pipe(Effect.map(Number)), ], { concurrency: "unbounded" }, ); return { stdout, stderr, code: exitCode } satisfies CommandResult; }).pipe(Effect.scoped); const runCodexCommand = makeCommandRunner("codex"); const runClaudeCommand = makeCommandRunner("claude"); const runGeminiCommand = makeCommandRunner("gemini");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 282 - 301, Extract the duplicated child-process logic into a parameterized helper (e.g., makeCommandRunner) and replace runCodexCommand, runClaudeCommand, and runGeminiCommand to call that helper with their respective binary names; the helper should encapsulate the Effect.gen block that obtains ChildProcessSpawner.ChildProcessSpawner, constructs ChildProcess.make(binary, [...args], { shell: process.platform === "win32" }), spawns the child, collects stdout/stderr via collectStreamAsString, maps child.exitCode to Number, and returns the CommandResult, then .pipe(Effect.scoped) so the three run* functions simply reference makeCommandRunner("codex"/"claude"/"gemini")..plans/17-claude-agent.md (1)
68-70: Consider aligningproviderOptions.claudeCodenaming withclaudeAgent.The plan references
providerOptions.claudeCodeas the provider-specific options key. Given the PR's rename fromclaudeCodetoclaudeAgentthroughout the codebase, consider whether the options key should also beproviderOptions.claudeAgentfor consistency.However, if
claudeCoderefers specifically to the Claude Code SDK/binary (distinct from the agent abstraction), keeping it may be intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.plans/17-claude-agent.md around lines 68 - 70, The plan uses providerOptions.claudeCode but the codebase renamed claudeCode to claudeAgent; update the plan and types to use providerOptions.claudeAgent for consistency by replacing all occurrences of providerOptions.claudeCode with providerOptions.claudeAgent and updating the corresponding ProviderOptions/typed providerOptions interface and any usages in functions or classes that read that key (e.g., code that constructs the Claude agent, references to claudeAgent/ClaudeAgent), unless the original providerOptions.claudeCode intentionally refers to the separate Claude Code SDK—in that exceptional case add a clarifying comment and keep the original name.apps/web/src/session-logic.test.ts (1)
449-543: Add the cross-thread source-plan case here.
findSidebarProposedPlan()now resolveslatestTurn.sourceProposedPlan.threadIdagainst the fullthreadscollection, but these fixtures only cover the same-thread lookup. A regression in the actual cross-thread plan-consumption path would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/session-logic.test.ts` around lines 449 - 543, Tests for findSidebarProposedPlan only cover when latestTurn.sourceProposedPlan points to a plan in the same thread; add a case where latestTurn.sourceProposedPlan.threadId refers to a different thread in the threads array to ensure cross-thread lookup works. Update the test suite (the describe "findSidebarProposedPlan" block) to include a scenario where latestTurn.sourceProposedPlan.threadId is ThreadId.makeUnsafe("thread-2") while the current threadId is "thread-1", populate proposedPlans on thread-2 with the referenced plan, and assert the returned plan matches that cross-thread plan so the implementation that resolves sourceProposedPlan.threadId against the full threads collection is exercised.apps/server/src/provider/Layers/ClaudeAdapter.test.ts (1)
1075-1101: Scope this stream consumer to the test fiber.
Effect.runFork(...)here creates an unscoped reader, so if anything fails before Line 1100 it can keep drainingadapter.streamEventsinto later tests. The fixedyieldNowchain also makes the assertion timing-sensitive. PreferEffect.forkChildplusFiber.interruptso cleanup stays tied to the test scope.♻️ Suggested cleanup pattern
- const runtimeEventsFiber = Effect.runFork( - Stream.runForEach(adapter.streamEvents, (event) => - Effect.sync(() => { - runtimeEvents.push(event); - }), - ), - ); + const runtimeEventsFiber = yield* Stream.runForEach(adapter.streamEvents, (event) => + Effect.sync(() => { + runtimeEvents.push(event); + }), + ).pipe(Effect.forkChild); ... - runtimeEventsFiber.interruptUnsafe(); + yield* Fiber.interrupt(runtimeEventsFiber);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts` around lines 1075 - 1101, Replace the unscoped Effect.runFork consumer with a child fiber scoped to the test: use Effect.forkChild (or the equivalent scoped fork) to start Stream.runForEach(adapter.streamEvents, ...) and save the returned child Fiber (currently runtimeEventsFiber) so it is bound to the test fiber; when cleaning up interrupt that child fiber with its interrupt method (not interruptUnsafe) so the consumer cannot outlive the test and race with later tests; keep the Stream.runForEach and runtimeEvents push logic unchanged but swap the fork call and cleanup to use forkChild + Fiber.interrupt on runtimeEventsFiber.
🤖 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/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Line 166: threadModelOptions is populated but never cleared on thread
teardown; update the thread teardown handlers that already remove
threadProviderOptions (those reacting to "thread.deleted" and
"thread.session-stop-requested") to also delete the entry from
threadModelOptions. Locate the Map named threadModelOptions and add
corresponding threadModelOptions.delete(threadId) calls inside the same event
handlers (and mirror the same cleanup logic referenced around the other
occurrence at the block noted by the reviewer) so option blobs are removed when
a thread is torn down.
- Around line 540-550: The catch currently maps all causes (including
interrupts) to a provider failure activity; instead, detect and re-raise
interruption causes so shutdown/drain interrupts are preserved and only
non-interrupt failures produce the appendProviderFailureActivity call. Modify
the Effect.catchCause block around the pipeline that calls
appendProviderFailureActivity (the block referencing event.payload.threadId,
kind "provider.turn.start.failed", and appendProviderFailureActivity) to inspect
the Cause and rethrow/propagate if it's an interrupt (preserve the cause) and
only call appendProviderFailureActivity for non-interrupt causes.
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 110-116: The helpers toTurnId and toProviderItemId check trimmed
input but pass the original untrimmed value into TurnId.makeUnsafe and
ProviderItemId.makeUnsafe, which preserves whitespace in IDs; update both
functions to trim once into a local variable (e.g., const v = value?.trim()) and
use that trimmed v for the truthy check and when calling TurnId.makeUnsafe(v)
and ProviderItemId.makeUnsafe(v) so IDs never include leading/trailing
whitespace.
In `@apps/web/src/appSettings.ts`:
- Around line 49-58: Persisted values under
gitTextGenerationModelByProvider.claudeCode will be dropped because
PROVIDER_KINDS no longer includes "claudeCode"; add a migration step before
schema normalization that detects gitTextGenerationModelByProvider.claudeCode,
moves/merges its value to the new key (claudeAgent) and removes the legacy key
so users don't lose settings. Locate the normalization flow around
PROVIDER_KINDS and the code that reads/writes gitTextGenerationModelByProvider
and inject a one-time migration that checks for the legacy property, transfers
its value to gitTextGenerationModelByProvider.claudeAgent (respecting existing
claudeAgent value if present), and then deletes
gitTextGenerationModelByProvider.claudeCode.
In `@apps/web/src/components/chat/ProviderModelPicker.browser.tsx`:
- Around line 80-95: The test assumes ProviderModelPicker hides other provider
submenus when lockedProvider is set, but the component currently renders
submenus and only disables non-locked providers; update the test instead of the
component: in the "shows models directly when the provider is locked mid-thread"
spec (the mountPicker invocation and subsequent UI assertions), replace the
assertion that "Codex" is not present with assertions that "Codex" appears in
the popup but is disabled (check for a disabled attribute, aria-disabled, or a
CSS disabled class on the Codex menu item/button). Ensure the test still
verifies the locked provider's models (e.g., "Claude Sonnet 4.6") are visible
and interactable while other providers exist but are disabled.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 335-407: The normalizeProviderModelOptions function reconstructs
and returns an object containing only codex and claudeAgent, which drops other
provider keys (copilot, cursor, opencode, geminiCli, amp, kilo, etc.); update
normalizeProviderModelOptions to merge and preserve any other valid provider
entries from the incoming candidate (or the original value) into the returned
ProviderModelOptions instead of rebuilding only codex/claudeAgent — locate
normalizeProviderModelOptions and the similar reconstruction sites (the other
occurrences noted around the file) and change the return to include
...otherProviders (or otherwise copy through unknown provider keys that match
ProviderModelOptions shape) so existing provider fields are retained during
setModelOptions()/rehydration.
- Around line 34-37: The debounced storage (composerDebouncedStorage created via
createDebouncedStorage) delays setItem() and can cause
syncPersistedAttachments() to read a stale snapshot; fix by flushing the
debounced store before any synchronous read of persisted attachments: call and
await composerDebouncedStorage.flush() (or the equivalent immediate-sync method
provided by createDebouncedStorage) at the start of syncPersistedAttachments()
and before the probe that checks persisted attachments (the code around
syncPersistedAttachments and the read at lines ~787-796), making the caller
async if needed so the flush completes before reading storage.
In `@apps/web/src/index.css`:
- Line 450: The CSS declaration background-image: var(--ultrathink-spectrum); in
index.css violates the declaration-empty-line-before rule; fix it by adding a
single empty line immediately above that declaration (or otherwise ensure it is
the first declaration in the rule) so the block conforms to Stylelint's
declaration-empty-line-before expectations in apps/web/src/index.css.
In `@apps/web/src/session-logic.ts`:
- Around line 541-570: The current collapse logic in
shouldCollapseToolLifecycleEntries (used by collapseDerivedWorkLogEntries and
mergeDerivedWorkLogEntries) only compares collapseKey, causing distinct adjacent
tool calls with identical args to be merged; modify
shouldCollapseToolLifecycleEntries so it also requires a stable instance
discriminator to match (preferably activity.providerItemId if present, otherwise
activity.turnId) before returning true, i.e., only collapse when collapseKey is
equal AND the chosen instance id is equal between previous and next; update any
callers/tests referencing this behavior accordingly.
In `@apps/web/src/store.ts`:
- Around line 9-13: The import list includes an unused symbol
inferProviderForModel; remove inferProviderForModel from the import statement so
only resolveModelSlug and resolveModelSlugForProvider remain imported (update
the import that currently references inferProviderForModel to exclude it).
In `@packages/contracts/src/providerRuntime.ts`:
- Around line 431-433: The multiSelect field currently declares multiSelect:
Schema.optional(Schema.Boolean) but uses Schema.withConstructorDefault(() =>
Option.some(false)), causing a type mismatch between Boolean|undefined and
Option<boolean>; fix by either changing the schema to an Option type with
Schema.optionalWith(Schema.Boolean, { as: "Option" }) and keep
Schema.withConstructorDefault(() => Option.some(false)), or keep
Schema.optional(Schema.Boolean) and replace the default with a plain boolean via
Schema.withConstructorDefault(() => false); update the multiSelect declaration
and its .pipe(...) usage accordingly (referencing multiSelect, Schema.optional,
Schema.optionalWith, Schema.withConstructorDefault, and Option.some).
In `@packages/shared/src/model.test.ts`:
- Around line 74-79: Remove the duplicated test block named "uses
provider-specific aliases" that calls normalizeModelSlug (the assertions
expecting "claude-sonnet-4-6", "claude-opus-4-6", and "claude-haiku-4-5")—keep
the original single test and delete the redundant copy so there is only one set
of these assertions in model.test.ts.
In `@packages/shared/src/model.ts`:
- Around line 466-480: inferProviderForModel currently only checks "claudeAgent"
and "codex", causing many provider-specific slugs to fall back; update
inferProviderForModel to consult MODEL_SLUG_SET_BY_PROVIDER for all providers
instead of only those two. Use normalizeModelSlug(model, <providerKind>) and
test membership in MODEL_SLUG_SET_BY_PROVIDER[<providerKind>] for each
ProviderKind (or iterate the keys of MODEL_SLUG_SET_BY_PROVIDER) and return the
matching provider, falling back to the existing final heuristic (and fallback
param) only if no provider matches; reference inferProviderForModel,
normalizeModelSlug, MODEL_SLUG_SET_BY_PROVIDER and ProviderKind to locate and
implement the change.
- Around line 610-624: The guard that prevents double-prepending the Ultrathink
prefix is case-sensitive and only checks for "Ultrathink:"; update
applyClaudePromptEffortPrefix to check for the prefix case-insensitively (e.g.,
use trimmed.toLowerCase().startsWith("ultrathink:") or a case-insensitive regex
test against trimmed) so inputs like "ultrathink:" or "ULTRATHINK:" are
recognized and the function won't add a second "Ultrathink:" prefix. Ensure you
replace the existing startsWith("Ultrathink:") check and keep the rest of the
logic (trim, effort === "ultrathink", return original trimmed string) intact.
- Around line 508-514: The overloads for getDefaultReasoningEffort promise a
non-null ProviderReasoningEffort for the generic provider signature but the
implementation can return null (via DEFAULT_REASONING_EFFORT_BY_PROVIDER) for
several ProviderKind values; update the third overload declaration (the one
taking provider?: ProviderKind) to return ProviderReasoningEffort | null so the
type matches runtime behavior, and keep the implementation of
getDefaultReasoningEffort(provider: ProviderKind = "codex") returning
DEFAULT_REASONING_EFFORT_BY_PROVIDER[provider] unchanged; reference
getDefaultReasoningEffort and DEFAULT_REASONING_EFFORT_BY_PROVIDER when making
the signature change.
In `@scripts/claude-fast-mode-probe.ts`:
- Around line 107-112: The non-success branch unconditionally calls
message.errors.join(...) which can throw because message.errors is optional;
update the result handling inside the message.type === "result" block (where
summary.resultSubtype, summary.resultFastModeState, and summary.resultText are
set) to guard the error path by using a safe fallback like
message.errors?.join("\n") ?? String(message.error ?? "") (or a similar fallback
such as JSON.stringify(message) or null) so summary.resultText is assigned
without throwing when errors is absent.
In `@scripts/claude-haiku-thinking-probe.ts`:
- Around line 228-241: When handling SDKMessage in the block that checks
message.type === "assistant" and message.type === "result", add defensive
null/undefined checks before accessing nested optional properties: ensure
message.message exists and message.message.content is an array before iterating
and incrementing summary.assistantThinkingBlockCount, and ensure message.result
and message.errors are present (fallback to empty string/array) before assigning
summary.resultText; also guard reading message.subtype into
summary.resultSubtype. Update the checks around the assistant loop and the
result branch (the code that references message.message.content,
message.subtype, message.result, and message.errors) so no property access
occurs on undefined.
---
Outside diff comments:
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts`:
- Around line 511-524: The test expectation is asserting the model "gpt-5-codex"
for a claudeAgent which is invalid; update the assertion so the mocked
startSession call expects a Claude model (e.g., "claude-2" or whichever model is
defined in the provider's model list) instead of "gpt-5-codex" — locate the
assertion checking harness.startSession.mock.calls[0]?.[1] and replace the
hard-coded "gpt-5-codex" with the correct Claude model or a lookup from the
provider config so the test reflects a supported provider/model pair.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 242-250: The upsertSessionBinding calls (e.g., the call around
readPersistedProviderOptions and input.binding.runtimePayload) omit passing
persisted modelOptions, causing ProviderSession to lose model tuning on rebind;
update all upsertSessionBinding calls (including the instances referenced and
similar calls at the other locations) to include extra.modelOptions populated
from the persisted source (e.g., readPersistedProviderOptions or a
readPersistedModelOptions helper) when present so that the call becomes
upsertSessionBinding(..., existingProviderOptions ? { providerOptions:
existingProviderOptions, modelOptions: existingProviderOptions.modelOptions } :
undefined) (or equivalent extraction of modelOptions) to ensure modelOptions
survive recover/stop cycles.
- Around line 347-369: The code restores only resumeCursor from persistedBinding
but not modelOptions/providerOptions, causing undefined to overwrite stored
options; update ProviderService.startSession to derive effectiveModelOptions and
effectiveProviderOptions (e.g., const effectiveModelOptions = input.modelOptions
?? persistedBinding?.modelOptions and similarly for providerOptions), pass those
effective values into adapter.startSession and into the upsertSessionBinding
call (use effectiveModelOptions/effectiveProviderOptions instead of
input.modelOptions/input.providerOptions) so existing persisted options are
preserved when the caller omits them.
In `@apps/web/src/lib/threadProvider.ts`:
- Around line 5-14: Add a compatibility mapping so legacy provider name
"claudeCode" is treated as "claudeAgent" during parsing: update the provider
parsing function (toProviderKind) to normalize incoming strings by mapping
"claudeCode" → "claudeAgent" before validating against PROVIDER_KINDS, ensuring
old persisted session data still resolves to the new ProviderKind; keep the rest
of the validation logic unchanged so unrecognized names still error/return
undefined as before.
---
Nitpick comments:
In @.plans/17-claude-agent.md:
- Around line 68-70: The plan uses providerOptions.claudeCode but the codebase
renamed claudeCode to claudeAgent; update the plan and types to use
providerOptions.claudeAgent for consistency by replacing all occurrences of
providerOptions.claudeCode with providerOptions.claudeAgent and updating the
corresponding ProviderOptions/typed providerOptions interface and any usages in
functions or classes that read that key (e.g., code that constructs the Claude
agent, references to claudeAgent/ClaudeAgent), unless the original
providerOptions.claudeCode intentionally refers to the separate Claude Code
SDK—in that exceptional case add a clarifying comment and keep the original
name.
In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts`:
- Around line 1075-1101: Replace the unscoped Effect.runFork consumer with a
child fiber scoped to the test: use Effect.forkChild (or the equivalent scoped
fork) to start Stream.runForEach(adapter.streamEvents, ...) and save the
returned child Fiber (currently runtimeEventsFiber) so it is bound to the test
fiber; when cleaning up interrupt that child fiber with its interrupt method
(not interruptUnsafe) so the consumer cannot outlive the test and race with
later tests; keep the Stream.runForEach and runtimeEvents push logic unchanged
but swap the fork call and cleanup to use forkChild + Fiber.interrupt on
runtimeEventsFiber.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 282-301: Extract the duplicated child-process logic into a
parameterized helper (e.g., makeCommandRunner) and replace runCodexCommand,
runClaudeCommand, and runGeminiCommand to call that helper with their respective
binary names; the helper should encapsulate the Effect.gen block that obtains
ChildProcessSpawner.ChildProcessSpawner, constructs ChildProcess.make(binary,
[...args], { shell: process.platform === "win32" }), spawns the child, collects
stdout/stderr via collectStreamAsString, maps child.exitCode to Number, and
returns the CommandResult, then .pipe(Effect.scoped) so the three run* functions
simply reference makeCommandRunner("codex"/"claude"/"gemini").
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Around line 587-608: The test duplicated the "routes explicit claudeAgent
provider session starts to the claude adapter" case; remove the redundant test
block that calls ProviderService.startSession with provider "claudeAgent" (the
one asserting routing.claude.startSession.mock.calls.length and validating
startInput.cwd and startPayload.provider) so only the original test remains;
ensure you delete the duplicate it.effect test entirely (including its
Effect.gen closure and assertions) and keep the other identical test unchanged.
- Around line 873-887: Replace the timing-based sleeps with an explicit
readiness handshake: change the test to await a promise that resolves when the
PubSub subscription is actually registered before calling fanout.codex.emit.
Concretely, modify the subscription setup used by ProviderService tests to
return (or set) a "ready" promise/resolve callback when the handler is attached,
then in the test await that readiness promise instead of yield* sleep(50) before
invoking fanout.codex.emit (and do the same for the other two occurrences).
In `@apps/server/src/wsServer.ts`:
- Around line 986-988: The branch handling provider === "claudeAgent" returns
only a stub ({ provider } satisfies ProviderUsageResult) without usage/quota
data; either implement actual usage retrieval using the Claude Agent SDK (call
its usage/quota methods and populate the ProviderUsageResult fields like usage,
quota, remaining, etc., then return that object) inside the same conditional, or
add a clear inline comment above the clause explaining that the Claude Agent SDK
does not expose usage/quota and this stub is intentional; update the return to
match the shape returned for other providers (e.g., copilot, codex, cursor) so
callers receive a consistent ProviderUsageResult.
In `@apps/web/src/appSettings.test.ts`:
- Around line 209-215: Remove the duplicate test block that asserts
provider-specific custom slugs are included: delete the describe/it block
calling getAppModelOptions("claudeAgent", ["claude/custom-opus"]) and its expect
assertion because the same assertion is already covered earlier; ensure no other
unique assertions in that block are lost and run tests after removal to confirm
coverage (look for getAppModelOptions and the "provider-specific custom models"
test title to locate the duplicate).
In `@apps/web/src/components/chat/ProviderModelPicker.tsx`:
- Around line 180-186: The function isAvailableProviderOption is an unused type
guard; remove this dead code or use it when constructing
AVAILABLE_PROVIDER_OPTIONS to narrow types. Either delete the
isAvailableProviderOption function definition entirely (and remove any imports
referencing it) or apply it to filter PROVIDER_OPTIONS into
AVAILABLE_PROVIDER_OPTIONS so the resulting array is correctly typed; update any
usages of AVAILABLE_PROVIDER_OPTIONS (and references to PROVIDER_OPTIONS) to
reflect the narrowed type if you choose to keep the guard.
- Around line 269-280: Remove the dead helper function handleModelChange since
it is defined but never used; locate the function named handleModelChange in
ProviderModelPicker and delete it along with any unused imports/closures it
references (e.g., resolveModelForProviderPicker, props.onProviderModelChange,
setIsMenuOpen) or, if you prefer to keep behavior, replace its single-use
callers with calls to the existing inline onModelSelect handlers instead; ensure
no references to handleModelChange remain and run type checks to remove any
leftover unused symbol warnings.
In `@apps/web/src/index.css`:
- Around line 421-482: Add a prefers-reduced-motion fallback that disables the
infinite animations and motion-based effects for the ultrathink styles: inside a
`@media` (prefers-reduced-motion: reduce) block override the animations for
ultrathink-rainbow and ultrathink-chroma (and any animated properties on
.ultrathink-frame, .ultrathink-chroma, .ultrathink-pill, .ultrathink-word) to
none (or effectively zero duration) and freeze animated properties like
background-position and filter to a sensible static value so the gradient/chroma
remain visible but not animated.
In `@apps/web/src/session-logic.test.ts`:
- Around line 449-543: Tests for findSidebarProposedPlan only cover when
latestTurn.sourceProposedPlan points to a plan in the same thread; add a case
where latestTurn.sourceProposedPlan.threadId refers to a different thread in the
threads array to ensure cross-thread lookup works. Update the test suite (the
describe "findSidebarProposedPlan" block) to include a scenario where
latestTurn.sourceProposedPlan.threadId is ThreadId.makeUnsafe("thread-2") while
the current threadId is "thread-1", populate proposedPlans on thread-2 with the
referenced plan, and assert the returned plan matches that cross-thread plan so
the implementation that resolves sourceProposedPlan.threadId against the full
threads collection is exercised.
In `@packages/contracts/src/model.ts`:
- Line 238: Remove the runtime default from the contracts package: delete the
exported constant DEFAULT_GIT_TEXT_GENERATION_MODEL from
packages/contracts/src/model.ts so contracts remain schema/type-only, and
instead define that default in the consuming runtime package (where
configuration/implementation lives). Update any imports that currently reference
DEFAULT_GIT_TEXT_GENERATION_MODEL to point to the new runtime location (or to
use a literal/default there) so packages/contracts only contains types/schemas
(e.g., leave model types/interfaces in model.ts but move the actual string
default into the runtime codebase).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2401992-6971-4f73-8389-0ad80c7c1c48
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (102)
.docs/provider-architecture.md.github/workflows/release.yml.plans/17-claude-agent.md.plans/17-claude-code.mdAGENTS.mdapps/desktop/package.jsonapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/TestProviderAdapter.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/package.jsonapps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/orchestration/decider.tsapps/server/src/persistence/Layers/ProjectionThreadProposedPlans.tsapps/server/src/persistence/Layers/ProjectionTurns.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/014_ProjectionThreadProposedPlanImplementation.tsapps/server/src/persistence/Migrations/015_ProjectionTurnsSourceProposedPlan.tsapps/server/src/persistence/Services/ProjectionThreadProposedPlans.tsapps/server/src/persistence/Services/ProjectionTurns.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.test.tsapps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/ProviderAdapterConformance.test.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/ProviderSessionDirectory.test.tsapps/server/src/provider/Layers/ProviderSessionDirectory.tsapps/server/src/provider/Services/ClaudeAdapter.tsapps/server/src/provider/Services/ClaudeCodeAdapter.tsapps/server/src/provider/Services/ProviderAdapter.tsapps/server/src/provider/Services/ProviderHealth.tsapps/server/src/provider/claude-agent-sdk.d.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.tsapps/web/package.jsonapps/web/public/mockServiceWorker.jsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/BranchToolbar.logic.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/ProviderLogo.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ClaudeCodeTraitsPicker.tsxapps/web/src/components/chat/ClaudeTraitsPicker.browser.tsxapps/web/src/components/chat/ClaudeTraitsPicker.tsxapps/web/src/components/chat/CodexTraitsPicker.browser.tsxapps/web/src/components/chat/CodexTraitsPicker.tsxapps/web/src/components/chat/CompactComposerControlsMenu.browser.tsxapps/web/src/components/chat/CompactComposerControlsMenu.tsxapps/web/src/components/chat/ProviderHealthBanner.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/index.cssapps/web/src/lib/storage.tsapps/web/src/lib/threadDraftDefaults.test.tsapps/web/src/lib/threadProvider.test.tsapps/web/src/lib/threadProvider.tsapps/web/src/lib/utils.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.settings.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/store.test.tsapps/web/src/store.tsapps/web/src/types.tsapps/web/vitest.browser.config.tspackages/contracts/package.jsonpackages/contracts/src/model.tspackages/contracts/src/orchestration.test.tspackages/contracts/src/orchestration.tspackages/contracts/src/provider.test.tspackages/contracts/src/provider.tspackages/contracts/src/providerRuntime.test.tspackages/contracts/src/providerRuntime.tspackages/shared/src/model.test.tspackages/shared/src/model.tsscripts/claude-fast-mode-probe.tsscripts/claude-haiku-thinking-probe.tsscripts/package.json
💤 Files with no reviewable changes (3)
- apps/web/src/components/chat/ClaudeCodeTraitsPicker.tsx
- apps/server/src/provider/Layers/ClaudeCodeAdapter.test.ts
- apps/server/src/provider/Services/ClaudeCodeAdapter.ts
| ), | ||
| ); | ||
|
|
||
| const threadModelOptions = new Map<ThreadId, ProviderModelOptions>(); |
There was a problem hiding this comment.
Clear threadModelOptions when a thread is torn down.
This cache is populated here, but unlike threadProviderOptions it is never deleted on thread.deleted or thread.session-stop-requested. Long-lived servers will retain option blobs for dead threads indefinitely.
Also applies to: 397-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` at line 166,
threadModelOptions is populated but never cleared on thread teardown; update the
thread teardown handlers that already remove threadProviderOptions (those
reacting to "thread.deleted" and "thread.session-stop-requested") to also delete
the entry from threadModelOptions. Locate the Map named threadModelOptions and
add corresponding threadModelOptions.delete(threadId) calls inside the same
event handlers (and mirror the same cleanup logic referenced around the other
occurrence at the block noted by the reviewer) so option blobs are removed when
a thread is torn down.
Comment on lines
+110
to
+116
| function toTurnId(value: string | undefined): TurnId | undefined { | ||
| return value?.trim() ? TurnId.makeUnsafe(value) : undefined; | ||
| } | ||
|
|
||
| function toProviderItemId(value: string | undefined): ProviderItemId | undefined { | ||
| return value?.trim() ? ProviderItemId.makeUnsafe(value) : undefined; | ||
| } |
There was a problem hiding this comment.
Inconsistent use of trimmed value in ID helpers.
Both functions validate against the trimmed value (value?.trim()) but then create the ID with the original untrimmed value. If value contains leading/trailing whitespace, the check passes but the whitespace is preserved in the resulting ID.
🔧 Proposed fix to use trimmed value consistently
function toTurnId(value: string | undefined): TurnId | undefined {
- return value?.trim() ? TurnId.makeUnsafe(value) : undefined;
+ const trimmed = value?.trim();
+ return trimmed ? TurnId.makeUnsafe(trimmed) : undefined;
}
function toProviderItemId(value: string | undefined): ProviderItemId | undefined {
- return value?.trim() ? ProviderItemId.makeUnsafe(value) : undefined;
+ const trimmed = value?.trim();
+ return trimmed ? ProviderItemId.makeUnsafe(trimmed) : undefined;
}📝 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
| function toTurnId(value: string | undefined): TurnId | undefined { | |
| return value?.trim() ? TurnId.makeUnsafe(value) : undefined; | |
| } | |
| function toProviderItemId(value: string | undefined): ProviderItemId | undefined { | |
| return value?.trim() ? ProviderItemId.makeUnsafe(value) : undefined; | |
| } | |
| function toTurnId(value: string | undefined): TurnId | undefined { | |
| const trimmed = value?.trim(); | |
| return trimmed ? TurnId.makeUnsafe(trimmed) : undefined; | |
| } | |
| function toProviderItemId(value: string | undefined): ProviderItemId | undefined { | |
| const trimmed = value?.trim(); | |
| return trimmed ? ProviderItemId.makeUnsafe(trimmed) : undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/CodexAdapter.ts` around lines 110 - 116, The
helpers toTurnId and toProviderItemId check trimmed input but pass the original
untrimmed value into TurnId.makeUnsafe and ProviderItemId.makeUnsafe, which
preserves whitespace in IDs; update both functions to trim once into a local
variable (e.g., const v = value?.trim()) and use that trimmed v for the truthy
check and when calling TurnId.makeUnsafe(v) and ProviderItemId.makeUnsafe(v) so
IDs never include leading/trailing whitespace.
Comment on lines
+466
to
+480
| export function inferProviderForModel( | ||
| model: string | null | undefined, | ||
| fallback: ProviderKind = "codex", | ||
| ): ProviderKind { | ||
| const normalizedClaude = normalizeModelSlug(model, "claudeAgent"); | ||
| if (normalizedClaude && MODEL_SLUG_SET_BY_PROVIDER.claudeAgent.has(normalizedClaude)) { | ||
| return "claudeAgent"; | ||
| } | ||
|
|
||
| const normalizedCodex = normalizeModelSlug(model, "codex"); | ||
| if (normalizedCodex && MODEL_SLUG_SET_BY_PROVIDER.codex.has(normalizedCodex)) { | ||
| return "codex"; | ||
| } | ||
|
|
||
| return typeof model === "string" && model.trim().startsWith("claude-") ? "claudeAgent" : fallback; |
There was a problem hiding this comment.
inferProviderForModel() still ignores six providers.
Lines 466-480 only test the Claude Agent and Codex catalogs. Unique slugs like composer-1.5, smart, and gemini-2.5-pro therefore fall back instead of resolving to cursor, amp, and geminiCli, which makes this helper unsafe for the rest of the provider registry.
💡 Proposed fix
export function inferProviderForModel(
model: string | null | undefined,
fallback: ProviderKind = "codex",
): ProviderKind {
- const normalizedClaude = normalizeModelSlug(model, "claudeAgent");
- if (normalizedClaude && MODEL_SLUG_SET_BY_PROVIDER.claudeAgent.has(normalizedClaude)) {
- return "claudeAgent";
- }
-
- const normalizedCodex = normalizeModelSlug(model, "codex");
- if (normalizedCodex && MODEL_SLUG_SET_BY_PROVIDER.codex.has(normalizedCodex)) {
- return "codex";
- }
-
- return typeof model === "string" && model.trim().startsWith("claude-") ? "claudeAgent" : fallback;
+ const matches = (Object.keys(MODEL_SLUG_SET_BY_PROVIDER) as ProviderKind[]).filter((provider) => {
+ const normalized = normalizeModelSlug(model, provider);
+ return normalized ? MODEL_SLUG_SET_BY_PROVIDER[provider].has(normalized) : false;
+ });
+
+ if (matches.includes(fallback)) {
+ return fallback;
+ }
+ if (matches.length === 1) {
+ return matches[0];
+ }
+
+ return typeof model === "string" && model.trim().startsWith("claude-")
+ ? "claudeAgent"
+ : fallback;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/model.ts` around lines 466 - 480, inferProviderForModel
currently only checks "claudeAgent" and "codex", causing many provider-specific
slugs to fall back; update inferProviderForModel to consult
MODEL_SLUG_SET_BY_PROVIDER for all providers instead of only those two. Use
normalizeModelSlug(model, <providerKind>) and test membership in
MODEL_SLUG_SET_BY_PROVIDER[<providerKind>] for each ProviderKind (or iterate the
keys of MODEL_SLUG_SET_BY_PROVIDER) and return the matching provider, falling
back to the existing final heuristic (and fallback param) only if no provider
matches; reference inferProviderForModel, normalizeModelSlug,
MODEL_SLUG_SET_BY_PROVIDER and ProviderKind to locate and implement the change.
Comment on lines
+610
to
+624
| export function applyClaudePromptEffortPrefix( | ||
| text: string, | ||
| effort: ClaudeCodeEffort | null | undefined, | ||
| ): string { | ||
| const trimmed = text.trim(); | ||
| if (!trimmed) { | ||
| return trimmed; | ||
| } | ||
| if (effort !== "ultrathink") { | ||
| return trimmed; | ||
| } | ||
| if (trimmed.startsWith("Ultrathink:")) { | ||
| return trimmed; | ||
| } | ||
| return `Ultrathink:\n${trimmed}`; |
There was a problem hiding this comment.
Make the ultrathink prefix guard case-insensitive.
Lines 621-624 only recognize Ultrathink: with exact casing. If a user edits the prompt to ultrathink: or ULTRATHINK:, this helper prepends a second copy.
💡 Proposed fix
- if (trimmed.startsWith("Ultrathink:")) {
+ if (/^ultrathink:/i.test(trimmed)) {
return trimmed;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/model.ts` around lines 610 - 624, The guard that prevents
double-prepending the Ultrathink prefix is case-sensitive and only checks for
"Ultrathink:"; update applyClaudePromptEffortPrefix to check for the prefix
case-insensitively (e.g., use trimmed.toLowerCase().startsWith("ultrathink:") or
a case-insensitive regex test against trimmed) so inputs like "ultrathink:" or
"ULTRATHINK:" are recognized and the function won't add a second "Ultrathink:"
prefix. Ensure you replace the existing startsWith("Ultrathink:") check and keep
the rest of the logic (trim, effort === "ultrathink", return original trimmed
string) intact.
Comment on lines
+107
to
+112
| if (message.type !== "result") { | ||
| return; | ||
| } | ||
| summary.resultSubtype = message.subtype; | ||
| summary.resultFastModeState = message.fast_mode_state ?? null; | ||
| summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'claude-agent-sdk\.d\.ts$' | head -n1)"
sed -n '60,120p' "$file"Repository: aaditagrawal/t3code
Length of output: 2016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the actual file to confirm lines 107-112
echo "=== Lines 107-112 from scripts/claude-fast-mode-probe.ts ==="
cat -n scripts/claude-fast-mode-probe.ts | sed -n '105,115p'
echo ""
echo "=== Check for other message.errors usages in the file ==="
rg 'message\.errors' scripts/claude-fast-mode-probe.ts -A 2 -B 2Repository: aaditagrawal/t3code
Length of output: 784
Guard the result error path when errors is absent.
Line 112 unconditionally calls message.errors.join("\n") for non-success results, but the SDK type defines errors as optional. This will throw if the SDK emits only the error field instead.
Suggested fix
summary.resultSubtype = message.subtype;
summary.resultFastModeState = message.fast_mode_state ?? null;
- summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n");
+ summary.resultText =
+ message.subtype === "success"
+ ? (message.result ?? null)
+ : Array.isArray(message.errors)
+ ? message.errors.map(String).join("\n")
+ : (typeof message.error === "string" ? message.error : null);📝 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
| if (message.type !== "result") { | |
| return; | |
| } | |
| summary.resultSubtype = message.subtype; | |
| summary.resultFastModeState = message.fast_mode_state ?? null; | |
| summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n"); | |
| if (message.type !== "result") { | |
| return; | |
| } | |
| summary.resultSubtype = message.subtype; | |
| summary.resultFastModeState = message.fast_mode_state ?? null; | |
| summary.resultText = | |
| message.subtype === "success" | |
| ? (message.result ?? null) | |
| : Array.isArray(message.errors) | |
| ? message.errors.map(String).join("\n") | |
| : (typeof message.error === "string" ? message.error : null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/claude-fast-mode-probe.ts` around lines 107 - 112, The non-success
branch unconditionally calls message.errors.join(...) which can throw because
message.errors is optional; update the result handling inside the message.type
=== "result" block (where summary.resultSubtype, summary.resultFastModeState,
and summary.resultText are set) to guard the error path by using a safe fallback
like message.errors?.join("\n") ?? String(message.error ?? "") (or a similar
fallback such as JSON.stringify(message) or null) so summary.resultText is
assigned without throwing when errors is absent.
Comment on lines
+228
to
+241
| if (message.type === "assistant") { | ||
| for (const block of message.message.content) { | ||
| if (typeof block?.type === "string" && block.type.includes("thinking")) { | ||
| summary.assistantThinkingBlockCount += 1; | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (message.type === "result") { | ||
| summary.resultSubtype = message.subtype; | ||
| summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null checks for optional message properties.
The SDKMessage type (from claude-agent-sdk.d.ts) has optional message and content fields. Accessing message.message.content on Line 229 will throw if message.message is undefined. Similarly, message.result and message.errors on Line 239 are optional.
🛡️ Proposed fix with defensive access
if (message.type === "assistant") {
- for (const block of message.message.content) {
+ const content = message.message?.content;
+ if (!content) return;
+ for (const block of content) {
if (typeof block?.type === "string" && block.type.includes("thinking")) {
summary.assistantThinkingBlockCount += 1;
}
}
return;
}
if (message.type === "result") {
summary.resultSubtype = message.subtype;
- summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n");
+ summary.resultText = message.subtype === "success"
+ ? (message.result ?? null)
+ : (message.errors?.join("\n") ?? null);
}📝 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
| if (message.type === "assistant") { | |
| for (const block of message.message.content) { | |
| if (typeof block?.type === "string" && block.type.includes("thinking")) { | |
| summary.assistantThinkingBlockCount += 1; | |
| } | |
| } | |
| return; | |
| } | |
| if (message.type === "result") { | |
| summary.resultSubtype = message.subtype; | |
| summary.resultText = message.subtype === "success" ? message.result : message.errors.join("\n"); | |
| } | |
| } | |
| if (message.type === "assistant") { | |
| const content = message.message?.content; | |
| if (!content) return; | |
| for (const block of content) { | |
| if (typeof block?.type === "string" && block.type.includes("thinking")) { | |
| summary.assistantThinkingBlockCount += 1; | |
| } | |
| } | |
| return; | |
| } | |
| if (message.type === "result") { | |
| summary.resultSubtype = message.subtype; | |
| summary.resultText = message.subtype === "success" | |
| ? (message.result ?? null) | |
| : (message.errors?.join("\n") ?? null); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/claude-haiku-thinking-probe.ts` around lines 228 - 241, When handling
SDKMessage in the block that checks message.type === "assistant" and
message.type === "result", add defensive null/undefined checks before accessing
nested optional properties: ensure message.message exists and
message.message.content is an array before iterating and incrementing
summary.assistantThinkingBlockCount, and ensure message.result and
message.errors are present (fallback to empty string/array) before assigning
summary.resultText; also guard reading message.subtype into
summary.resultSubtype. Update the checks around the assistant loop and the
result branch (the code that references message.message.content,
message.subtype, message.result, and message.errors) so no property access
occurs on undefined.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/appSettings.ts (1)
49-58:⚠️ Potential issue | 🟠 MajorMigrate legacy
claudeCodegit overrides before normalization.Once
claudeCodeis removed fromPROVIDER_KINDS, persistedgitTextGenerationModelByProvider.claudeCodevalues are silently dropped the next time settings are parsed. Add a bridge inmigratePersistedAppSettingsthat copies the legacy key toclaudeAgentbefore normalization, without overwriting an existingclaudeAgentvalue.🛠️ Suggested migration patch
function migratePersistedAppSettings(value: unknown): unknown { if (!value || typeof value !== "object" || Array.isArray(value)) { return value; } const settings = { ...(value as Record<string, unknown>) }; if (settings.providerLogoAppearance === undefined && settings.grayscaleProviderLogos === true) { settings.providerLogoAppearance = "grayscale"; } + + const gitOverrides = settings.gitTextGenerationModelByProvider; + if (gitOverrides && typeof gitOverrides === "object" && !Array.isArray(gitOverrides)) { + const record = { ...(gitOverrides as Record<string, unknown>) }; + if (typeof record.claudeAgent !== "string" && typeof record.claudeCode === "string") { + record.claudeAgent = record.claudeCode; + } + delete record.claudeCode; + settings.gitTextGenerationModelByProvider = record; + } return settings; }Also applies to: 432-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 49 - 58, Add a migration bridge in migratePersistedAppSettings to copy a legacy gitTextGenerationModelByProvider.claudeCode value to gitTextGenerationModelByProvider.claudeAgent before any normalization runs (so the removal of "claudeCode" from PROVIDER_KINDS won't drop persisted values); perform the copy only if claudeCode exists and claudeAgent is undefined/null to avoid overwriting an existing claudeAgent, then remove the claudeCode key so downstream normalization and PROVIDER_KINDS no longer need to handle it.
🧹 Nitpick comments (2)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
282-301: Extract a generic CLI runner before this file drifts further.This is now the third copy of the same spawn/collect/exit-code plumbing. A shared
runCliCommand(binary, args)helper would make future provider additions and upstream syncs less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 282 - 301, Extract a reusable helper runCliCommand(binary: string, args: ReadonlyArray<string>) that encapsulates the spawn/collect/exit-code plumbing currently duplicated: use ChildProcessSpawner.ChildProcessSpawner to spawn a ChildProcess.make(binary, [...args], { shell: process.platform === "win32" }), collect stdout/stderr via collectStreamAsString, map child.exitCode to Number and return { stdout, stderr, code } satisfying CommandResult; then replace the existing runClaudeCommand implementation to call runCliCommand("claude", args) so only runCliCommand holds the shared logic.apps/server/src/provider/Layers/ClaudeAdapter.test.ts (1)
2532-2589: ExerciseAskUserQuestionwith a live turn.This case invokes
canUseToolimmediately afterstartSession, so it bypasses the active-turn bookkeeping that runs in production. Starting a turn first would make the full-access assertion much more representative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts` around lines 2532 - 2589, Test calls canUseTool immediately after adapter.startSession which skips the per-turn bookkeeping; start an active turn before invoking canUseTool so the full-access assertion mirrors production. After creating the session via adapter.startSession, invoke the adapter's turn-start method (e.g., call the adapter method that begins a turn for the session — locate the method that advances/starts a turn in this test harness) or otherwise trigger a live turn via adapter.streamEvents (emit/await the turn-start event) before calling createInput?.options.canUseTool; then continue using streamEvents, respondToUserInput and permissionPromise as before so canUseTool runs within an actual active turn.
🤖 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/server/src/provider/Layers/ClaudeAdapter.test.ts`:
- Around line 1076-1101: The test currently forks a collector with
Effect.runFork over adapter.streamEvents, uses multiple yieldNow calls and
interruptUnsafe(), which causes races and unmanaged fibers; change it to scope
the collector and wait deterministically for the "session.exited" event: run the
stream with a scoped runner (e.g., Stream.runForEachScoped or Stream.runInto a
Queue) over adapter.streamEvents and push events into runtimeEvents inside that
scoped effect, start the session via adapter.startSession and sendTurn as
before, then await until runtimeEvents contains an event with type
"session.exited" (or pull from the queue until that event) before calling
harness.query.fail and making assertions; remove the yieldNow spins and
interruptUnsafe() so the collector is cleanly stopped by the scope when the test
completes.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 721-731: The conditional in ProviderHealth.ts that inspects
lowerOutput for auth errors currently checks for "run `claude login`" and "run
claude login" but not the CLI form you instruct users to run ("claude auth
login"), so add checks for "run `claude auth login`", "run claude auth login",
and a plain "claude auth login" to the same if block (the block that returns
status: "error" and authStatus: "unauthenticated") so emitted messages with the
"auth login" form are recognized as unauthenticated.
- Around line 62-66: The helper isCommandMissingCause currently checks for
"enoent" and "notfound" but omits the spaced form "command not found" which
other code paths (e.g., the missing-binary handler) expect; update
isCommandMissingCause to also check the lowercased error.message for the phrase
"command not found" (case-insensitive) in addition to the existing checks so the
function correctly identifies missing-command errors like "command not found".
---
Duplicate comments:
In `@apps/web/src/appSettings.ts`:
- Around line 49-58: Add a migration bridge in migratePersistedAppSettings to
copy a legacy gitTextGenerationModelByProvider.claudeCode value to
gitTextGenerationModelByProvider.claudeAgent before any normalization runs (so
the removal of "claudeCode" from PROVIDER_KINDS won't drop persisted values);
perform the copy only if claudeCode exists and claudeAgent is undefined/null to
avoid overwriting an existing claudeAgent, then remove the claudeCode key so
downstream normalization and PROVIDER_KINDS no longer need to handle it.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts`:
- Around line 2532-2589: Test calls canUseTool immediately after
adapter.startSession which skips the per-turn bookkeeping; start an active turn
before invoking canUseTool so the full-access assertion mirrors production.
After creating the session via adapter.startSession, invoke the adapter's
turn-start method (e.g., call the adapter method that begins a turn for the
session — locate the method that advances/starts a turn in this test harness) or
otherwise trigger a live turn via adapter.streamEvents (emit/await the
turn-start event) before calling createInput?.options.canUseTool; then continue
using streamEvents, respondToUserInput and permissionPromise as before so
canUseTool runs within an actual active turn.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 282-301: Extract a reusable helper runCliCommand(binary: string,
args: ReadonlyArray<string>) that encapsulates the spawn/collect/exit-code
plumbing currently duplicated: use ChildProcessSpawner.ChildProcessSpawner to
spawn a ChildProcess.make(binary, [...args], { shell: process.platform ===
"win32" }), collect stdout/stderr via collectStreamAsString, map child.exitCode
to Number and return { stdout, stderr, code } satisfying CommandResult; then
replace the existing runClaudeCommand implementation to call
runCliCommand("claude", args) so only runCliCommand holds the shared logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 855c859c-70a4-4eeb-8990-b9ed1151e47c
📒 Files selected for processing (5)
apps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/web/src/appSettings.tspackages/contracts/src/provider.test.tspackages/contracts/src/provider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts/src/provider.test.ts
Comment on lines
+1076
to
+1101
| const runtimeEventsFiber = Effect.runFork( | ||
| Stream.runForEach(adapter.streamEvents, (event) => | ||
| Effect.sync(() => { | ||
| runtimeEvents.push(event); | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| yield* adapter.startSession({ | ||
| threadId: THREAD_ID, | ||
| provider: "claudeAgent", | ||
| runtimeMode: "full-access", | ||
| }); | ||
|
|
||
| const turn = yield* adapter.sendTurn({ | ||
| threadId: THREAD_ID, | ||
| input: "hello", | ||
| attachments: [], | ||
| }); | ||
|
|
||
| harness.query.fail(new Error("All fibers interrupted without error")); | ||
|
|
||
| yield* Effect.yieldNow; | ||
| yield* Effect.yieldNow; | ||
| yield* Effect.yieldNow; | ||
| runtimeEventsFiber.interruptUnsafe(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/server/src/provider/Layers/ClaudeAdapter.test.ts | sed -n '1050,1120p'Repository: aaditagrawal/t3code
Length of output: 2941
🏁 Script executed:
# Check the full test method to understand the context
wc -l apps/server/src/provider/Layers/ClaudeAdapter.test.tsRepository: aaditagrawal/t3code
Length of output: 121
🏁 Script executed:
# Find the test method that contains lines 1076-1101
ast-grep --pattern $'it($_, ($$$) => {
$$$
Effect.runFork($$$);
$$$
runtimeEventsFiber.interruptUnsafe();
$$$
})'Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Search for other uses of Effect.runFork in test files to see patterns
rg 'Effect\.runFork' apps/server/src/provider/Layers/ -A 5 -B 2Repository: aaditagrawal/t3code
Length of output: 12216
🏁 Script executed:
# Check if session.exited is used as an event in the codebase
rg 'session\.exited|sessionExited' apps/server/src/provider/Layers/ClaudeAdapter.test.tsRepository: aaditagrawal/t3code
Length of output: 195
Avoid scheduler-dependent stream collection in this abort test.
Effect.runFork plus three yieldNows makes the assertion race the consumer, and interruptUnsafe() can leave the collector unmanaged if the test exits early. Keep the collector scoped to the test and wait for a deterministic stop condition—e.g. the "session.exited" event appearing in runtimeEvents—before asserting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts` around lines 1076 -
1101, The test currently forks a collector with Effect.runFork over
adapter.streamEvents, uses multiple yieldNow calls and interruptUnsafe(), which
causes races and unmanaged fibers; change it to scope the collector and wait
deterministically for the "session.exited" event: run the stream with a scoped
runner (e.g., Stream.runForEachScoped or Stream.runInto a Queue) over
adapter.streamEvents and push events into runtimeEvents inside that scoped
effect, start the session via adapter.startSession and sendTurn as before, then
await until runtimeEvents contains an event with type "session.exited" (or pull
from the queue until that event) before calling harness.query.fail and making
assertions; remove the yieldNow spins and interruptUnsafe() so the collector is
cleanly stopped by the scope when the test completes.
Comment on lines
62
to
66
| function isCommandMissingCause(error: unknown): boolean { | ||
| if (!(error instanceof Error)) return false; | ||
| const lower = error.message.toLowerCase(); | ||
| return ( | ||
| lower.includes("command not found: codex") || | ||
| lower.includes("spawn codex enoent") || | ||
| lower.includes("enoent") || | ||
| lower.includes("notfound") | ||
| ); | ||
| return lower.includes("enoent") || lower.includes("notfound"); | ||
| } |
There was a problem hiding this comment.
Restore the spaced missing-command match in the shared helper.
Line 65 drops the spaced "command not found" form that Line 468 still treats as a missing-binary signal. That regresses Codex/Claude from the clearer “not installed or not on PATH” message to a generic execution failure for the same outcome.
🩹 Proposed fix
function isCommandMissingCause(error: unknown): boolean {
if (!(error instanceof Error)) return false;
const lower = error.message.toLowerCase();
- return lower.includes("enoent") || lower.includes("notfound");
+ return (
+ lower.includes("enoent") ||
+ lower.includes("notfound") ||
+ lower.includes("command not found")
+ );
}📝 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
| function isCommandMissingCause(error: unknown): boolean { | |
| if (!(error instanceof Error)) return false; | |
| const lower = error.message.toLowerCase(); | |
| return ( | |
| lower.includes("command not found: codex") || | |
| lower.includes("spawn codex enoent") || | |
| lower.includes("enoent") || | |
| lower.includes("notfound") | |
| ); | |
| return lower.includes("enoent") || lower.includes("notfound"); | |
| } | |
| function isCommandMissingCause(error: unknown): boolean { | |
| if (!(error instanceof Error)) return false; | |
| const lower = error.message.toLowerCase(); | |
| return ( | |
| lower.includes("enoent") || | |
| lower.includes("notfound") || | |
| lower.includes("command not found") | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 62 - 66, The
helper isCommandMissingCause currently checks for "enoent" and "notfound" but
omits the spaced form "command not found" which other code paths (e.g., the
missing-binary handler) expect; update isCommandMissingCause to also check the
lowercased error.message for the phrase "command not found" (case-insensitive)
in addition to the existing checks so the function correctly identifies
missing-command errors like "command not found".
Comment on lines
+721
to
+731
| if ( | ||
| lowerOutput.includes("not logged in") || | ||
| lowerOutput.includes("login required") || | ||
| lowerOutput.includes("authentication required") || | ||
| lowerOutput.includes("run `claude login`") || | ||
| lowerOutput.includes("run claude login") | ||
| ) { | ||
| return { | ||
| status: "error", | ||
| authStatus: "unauthenticated", | ||
| message: "Claude is not authenticated. Run `claude auth login` and try again.", |
There was a problem hiding this comment.
Match the recovery command you already tell users to run.
Line 731 points users to claude auth login, but Lines 725-726 only recognize claude login. If the CLI emits the auth login form, this falls through to warning/unknown instead of error/unauthenticated.
🩹 Proposed fix
if (
lowerOutput.includes("not logged in") ||
lowerOutput.includes("login required") ||
lowerOutput.includes("authentication required") ||
+ lowerOutput.includes("run `claude auth login`") ||
+ lowerOutput.includes("run claude auth login") ||
lowerOutput.includes("run `claude login`") ||
lowerOutput.includes("run claude login")
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 721 - 731,
The conditional in ProviderHealth.ts that inspects lowerOutput for auth errors
currently checks for "run `claude login`" and "run claude login" but not the CLI
form you instruct users to run ("claude auth login"), so add checks for "run
`claude auth login`", "run claude auth login", and a plain "claude auth login"
to the same if block (the block that returns status: "error" and authStatus:
"unauthenticated") so emitted messages with the "auth login" form are recognized
as unauthenticated.
…coping, interrupt preservation, 8-provider normalization, legacy migration
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/appSettings.ts (1)
394-429:⚠️ Potential issue | 🟠 MajorUnify legacy-key migration across both settings read paths to prevent data loss.
migratePersistedAppSettings()runs only inparsePersistedSettings(), which is used bygetAppSettingsSnapshot(). However,useAppSettings()reads directly throughuseLocalStorage(..., AppSettingsSchema), which bypasses the migration. SinceuseLocalStorage's decode function only applies schema validation without any pre-processing, legacygitTextGenerationModelByProvider.claudeCodevalues will be invisible to React state and silently dropped on the next normalized write. This creates two inconsistent behaviors for the same persisted data.Move the migration logic into
useLocalStorage's decode path or a shared parser function so both entry points apply it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 394 - 429, The migration for legacy keys (e.g., gitTextGenerationModelByProvider.claudeCode) currently runs only in parsePersistedSettings, causing useAppSettings (which uses useLocalStorage with AppSettingsSchema) to skip migration and drop legacy values; update the code so both paths run the same migration: either call migratePersistedAppSettings (or refactor parsePersistedSettings into a shared parse function) inside useLocalStorage’s decode logic, or have useAppSettings call the same parser used by getAppSettingsSnapshot; ensure the migration of gitTextGenerationModelByProvider and providerLogoAppearance happens before AppSettingsSchema.makeUnsafe/validation so normalized writes preserve migrated keys.
♻️ Duplicate comments (4)
packages/shared/src/model.ts (2)
466-481:⚠️ Potential issue | 🟠 Major
inferProviderForModel()still only checksclaudeAgentandcodexproviders.The function currently only tests Claude Agent and Codex catalogs. Provider-specific slugs like
composer-1.5(cursor),smart(amp), andgemini-2.5-pro(geminiCli) will incorrectly fall back to the default instead of resolving to their respective providers.Consider iterating over all
MODEL_SLUG_SET_BY_PROVIDERkeys to support the full provider registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/model.ts` around lines 466 - 481, inferProviderForModel currently only checks "claudeAgent" and "codex"; update it to iterate over all providers in MODEL_SLUG_SET_BY_PROVIDER instead of hardcoding two providers. For each provider key, call normalizeModelSlug(model, providerKey) and if the normalized slug exists in MODEL_SLUG_SET_BY_PROVIDER[providerKey], return that provider (cast/typed as ProviderKind). Preserve the existing fallback behavior (including the special "claude-" prefix check) if no provider matches. Use the existing symbols inferProviderForModel, normalizeModelSlug, MODEL_SLUG_SET_BY_PROVIDER, and ProviderKind to locate and implement this loop.
610-625:⚠️ Potential issue | 🟡 MinorMake the ultrathink prefix guard case-insensitive.
Line 621 checks
trimmed.startsWith("Ultrathink:")with exact casing. If a user writes"ultrathink:"or"ULTRATHINK:", a second prefix will be prepended.💡 Proposed fix
- if (trimmed.startsWith("Ultrathink:")) { + if (/^ultrathink:/i.test(trimmed)) { return trimmed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/model.ts` around lines 610 - 625, The guard in applyClaudePromptEffortPrefix uses trimmed.startsWith("Ultrathink:") which is case-sensitive; change it to a case-insensitive check (e.g., use trimmed.toLowerCase().startsWith("ultrathink:") or a case-insensitive regex like /^ultrathink:/i) so values like "ultrathink:" or "ULTRATHINK:" are recognized and the prefix is not duplicated; keep the rest of the function behavior the same and reference the trimmed variable and applyClaudePromptEffortPrefix function when making the change.apps/server/src/orchestration/Layers/ProviderCommandReactor.ts (1)
167-168:⚠️ Potential issue | 🟡 MinorClear
threadModelOptionswhen a thread is torn down.The
threadModelOptionsmap is populated on line 399 but never cleared. UnlikethreadProviderOptions, it is not deleted inthread.deleted(line 755) orthread.session-stop-requested(line 729) handlers. This causes a memory leak for long-lived servers.💡 Proposed fix for thread.deleted handler
yield* providerService .stopSession({ threadId }) .pipe(Effect.catchCause(() => Effect.void)); threadProviderOptions.delete(threadId); + threadModelOptions.delete(threadId); return;💡 Proposed fix for session-stop-requested handler
threadProviderOptions.delete(thread.id); + threadModelOptions.delete(thread.id); yield* setThreadSession({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` around lines 167 - 168, threadModelOptions (the Map instantiated as threadModelOptions) is never cleaned up, causing a memory leak; update the same teardown points that remove threadProviderOptions to also remove entries from threadModelOptions — specifically, in the handlers for "thread.deleted" and "thread.session-stop-requested" (the functions where threadProviderOptions.delete(threadId) is called) add logic to delete the same threadId from threadModelOptions (or clear any nested state) so entries are removed when a thread is torn down.apps/web/src/composerDraftStore.ts (1)
792-804:⚠️ Potential issue | 🟠 MajorFlush debounced draft storage before probing persisted attachment IDs.
The probe on Line 1558 can run before the debounced write from Line 34-Line 37 is committed, so this read path may observe stale storage and incorrectly classify newly staged attachments as non-persisted.
💡 Proposed fix
function readPersistedAttachmentIdsFromStorage(threadId: ThreadId): string[] { if (threadId.length === 0) { return []; } try { + composerDebouncedStorage.flush(); const persisted = getLocalStorageItem( COMPOSER_DRAFT_STORAGE_KEY, PersistedComposerDraftStoreStorage, );Also applies to: 1558-1560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/composerDraftStore.ts` around lines 792 - 804, The readPersistedAttachmentIdsFromStorage function can observe stale LocalStorage because a debounced draft write may not have been flushed; before reading COMPOSER_DRAFT_STORAGE_KEY and validating PersistedComposerDraftStoreStorage/COMPOSER_DRAFT_STORAGE_VERSION, ensure you synchronously flush any pending debounced draft writes by calling the debounce-flush helper (e.g. flushDebouncedDraftStorage or the function that commits the debounced drafts) and await it if it returns a promise, then proceed with the current logic to return persisted.state.draftsByThreadId[threadId]?.attachments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/appSettings.ts`:
- Around line 394-429: The migration for legacy keys (e.g.,
gitTextGenerationModelByProvider.claudeCode) currently runs only in
parsePersistedSettings, causing useAppSettings (which uses useLocalStorage with
AppSettingsSchema) to skip migration and drop legacy values; update the code so
both paths run the same migration: either call migratePersistedAppSettings (or
refactor parsePersistedSettings into a shared parse function) inside
useLocalStorage’s decode logic, or have useAppSettings call the same parser used
by getAppSettingsSnapshot; ensure the migration of
gitTextGenerationModelByProvider and providerLogoAppearance happens before
AppSettingsSchema.makeUnsafe/validation so normalized writes preserve migrated
keys.
---
Duplicate comments:
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Around line 167-168: threadModelOptions (the Map instantiated as
threadModelOptions) is never cleaned up, causing a memory leak; update the same
teardown points that remove threadProviderOptions to also remove entries from
threadModelOptions — specifically, in the handlers for "thread.deleted" and
"thread.session-stop-requested" (the functions where
threadProviderOptions.delete(threadId) is called) add logic to delete the same
threadId from threadModelOptions (or clear any nested state) so entries are
removed when a thread is torn down.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 792-804: The readPersistedAttachmentIdsFromStorage function can
observe stale LocalStorage because a debounced draft write may not have been
flushed; before reading COMPOSER_DRAFT_STORAGE_KEY and validating
PersistedComposerDraftStoreStorage/COMPOSER_DRAFT_STORAGE_VERSION, ensure you
synchronously flush any pending debounced draft writes by calling the
debounce-flush helper (e.g. flushDebouncedDraftStorage or the function that
commits the debounced drafts) and await it if it returns a promise, then proceed
with the current logic to return
persisted.state.draftsByThreadId[threadId]?.attachments.
In `@packages/shared/src/model.ts`:
- Around line 466-481: inferProviderForModel currently only checks "claudeAgent"
and "codex"; update it to iterate over all providers in
MODEL_SLUG_SET_BY_PROVIDER instead of hardcoding two providers. For each
provider key, call normalizeModelSlug(model, providerKey) and if the normalized
slug exists in MODEL_SLUG_SET_BY_PROVIDER[providerKey], return that provider
(cast/typed as ProviderKind). Preserve the existing fallback behavior (including
the special "claude-" prefix check) if no provider matches. Use the existing
symbols inferProviderForModel, normalizeModelSlug, MODEL_SLUG_SET_BY_PROVIDER,
and ProviderKind to locate and implement this loop.
- Around line 610-625: The guard in applyClaudePromptEffortPrefix uses
trimmed.startsWith("Ultrathink:") which is case-sensitive; change it to a
case-insensitive check (e.g., use
trimmed.toLowerCase().startsWith("ultrathink:") or a case-insensitive regex like
/^ultrathink:/i) so values like "ultrathink:" or "ULTRATHINK:" are recognized
and the prefix is not duplicated; keep the rest of the function behavior the
same and reference the trimmed variable and applyClaudePromptEffortPrefix
function when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4229a73f-2f89-4949-b35e-5cbcad7667d7
📒 Files selected for processing (6)
apps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/web/src/appSettings.tsapps/web/src/composerDraftStore.tspackages/contracts/src/providerRuntime.tspackages/shared/src/model.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/src/providerRuntime.ts
- apps/server/src/provider/Layers/ProviderService.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/appSettings.ts (1)
457-470: Normalize the hook read path too.
getAppSettingsSnapshot()returns normalized settings, butuseAppSettings()still returns only the renamed-key variant. Existing localStorage entries can therefore behave differently depending on which read path is used until a write happens.♻️ Suggested patch
const migratedSettings = useMemo(() => { let next = settings; ... - return next; + return normalizeAppSettings(next); }, [settings]);Also applies to: 484-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 457 - 470, The useAppSettings() hook currently returns the un-migrated settings while getAppSettingsSnapshot() returns normalized settings; update the hook to return the migratedSettings (the result of the legacy key migration logic) so both read paths are consistent. Locate the migration logic that produces migratedSettings (checks settings.gitTextGenerationModelByProvider for "claudeCode" and maps it to "claudeAgent") and ensure useAppSettings() and any other read paths (referenced by the symbol useAppSettings) use that migratedSettings instead of the raw settings object so localStorage entries with old keys are normalized on read.
🤖 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/web/src/appSettings.ts`:
- Around line 404-415: The migration only renames claudeCode inside
gitTextGenerationModelByProvider; also migrate providerAccentColors: when
settings.providerAccentColors is a plain object (not array) and contains
"claudeCode" but not "claudeAgent", move the value to "claudeAgent" and delete
"claudeCode". Apply the same logic in both places where the code handles legacy
keys (the existing gitTextGenerationModelByProvider migration block and the
similar block around lines 457-470), using the same type checks and preserving
other keys when building the new object.
---
Nitpick comments:
In `@apps/web/src/appSettings.ts`:
- Around line 457-470: The useAppSettings() hook currently returns the
un-migrated settings while getAppSettingsSnapshot() returns normalized settings;
update the hook to return the migratedSettings (the result of the legacy key
migration logic) so both read paths are consistent. Locate the migration logic
that produces migratedSettings (checks settings.gitTextGenerationModelByProvider
for "claudeCode" and maps it to "claudeAgent") and ensure useAppSettings() and
any other read paths (referenced by the symbol useAppSettings) use that
migratedSettings instead of the raw settings object so localStorage entries with
old keys are normalized on read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35bcf9be-ff0b-4321-bf11-2b393a726d91
📒 Files selected for processing (1)
apps/web/src/appSettings.ts
2 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
4 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
Integrates 6 upstream commits from
pingdotgg/t3codeinto our multi-provider fork, preserving all 8 providers (codex, copilot, claudeAgent, cursor, opencode, geminiCli, amp, kilo).Upstream commits cherry-picked:
b500a36a— fix: implement button overflow (fix: implement button overflow pingdotgg/t3code#1193)0de5742b— fix: consume plans on implementation (fix: consume plans on implementation pingdotgg/t3code#1133)fb18b2de— fix(web): show text input for adding projects on Linux desktop (fix(web): show text input for adding projects on Linux desktop pingdotgg/t3code#1075)77716b4c— feat: add Claude Code adapter (feat: add Claude Code adapter pingdotgg/t3code#179) — major (2900+ line adapter, SDK integration, new UI components)2a237c20— Use secret release app ID in release workflow (Use secret release app ID in release workflow pingdotgg/t3code#1217)56660b04— chore(release): prepare v0.0.13Strategy
claudeCode→claudeAgentto align naming before cherry-picks, reducing conflict surfacegit merge upstream/mainto isolate conflictsKey changes
ClaudeAdapter.ts, 2900+ lines) with SDK integration, fast mode, ultrathink, adaptive reasoningmodelOptionsconsolidation: Replaced per-field draft state (effort,codexFastMode,claudeAgentEffort) with unifiedProviderModelOptionsobjectClaudeTraitsPicker,CompactComposerControlsMenu, ultrathink visual effectsVerification
bun typecheck— 7/7 packages passbun lint— 0 errors, 10 warnings (unused imports from merge)Test plan
bun typecheckpasses all 7 packagesbun linthas no errorsbun test apps/server/src/provider/— adapter testsbun test apps/server/src/orchestration/— orchestration testsbun test packages/contracts/— contract testsbun test packages/shared/— shared model testsSummary by CodeRabbit
New Features
Bug Fixes
Chores