Merge upstream: adopt ModelSelection union for 8 providers#33
Merged
aaditagrawal merged 17 commits intomainfrom Mar 25, 2026
Merged
Merge upstream: adopt ModelSelection union for 8 providers#33aaditagrawal merged 17 commits intomainfrom
aaditagrawal merged 17 commits intomainfrom
Conversation
…ingdotgg#1178) Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
…ingdotgg#1365) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…roviders Integrate 13 upstream commits including provider-aware model selections, settings UI refactor, sidebar sorting, git hook progress streaming, context window meter, and terminal improvements. Key architectural change: replace separate provider/model/modelOptions fields with a unified ModelSelection discriminated union extended to all 8 providers (codex, claudeAgent, copilot, cursor, opencode, geminiCli, amp, kilo).
github-actions
bot
added
the
size:XXL
📝 WalkthroughWalkthroughReplaces scattered provider/model/modelOptions with a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant WS as WebSocket Server
participant GitMgr as GitManager
participant GitCore as GitCore
Client->>WS: gitRunStackedAction({ actionId, cwd, action })
WS->>GitMgr: runStackedAction(input, { actionId, progressReporter })
activate GitMgr
GitMgr->>GitMgr: emit action_started
GitMgr->>GitCore: commit(cwd, subject, body, { progress })
activate GitCore
GitCore->>GitCore: tail Trace2 file, parse hook events
GitCore->>GitMgr: progress callbacks (hook_started / hook_output / hook_finished)
GitMgr->>WS: progressReporter.publish(hook_started/hook_output/hook_finished)
WS->>Client: push WS_CHANNELS.gitActionProgress events
GitCore-->>GitMgr: commit result / error
GitMgr->>WS: progressReporter.publish(action_finished|action_failed)
deactivate GitCore
deactivate GitMgr
sequenceDiagram
participant Caller as Web Client
participant Adapter as Provider Adapter
participant SDK as Provider SDK
Caller->>Adapter: sendTurn({ modelSelection: { provider, model, options } })
activate Adapter
Adapter->>Adapter: derive capabilities from modelSelection
Adapter->>SDK: setModel(model) and set options (effort/fastMode/thinking)
SDK-->>Adapter: task_progress { usage }
Adapter->>Adapter: normalize token usage -> ThreadTokenUsageSnapshot
Adapter->>Caller: emit thread.token-usage.updated (interim)
SDK-->>Adapter: result { modelUsage }
Adapter->>Adapter: derive final context-window snapshot
Adapter->>Caller: emit thread.token-usage.updated (final snapshot)
deactivate Adapter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
bot
added
the
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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
packages/contracts/src/provider.test.ts (1)
52-70:⚠️ Potential issue | 🔴 CriticalRemove invalid
modelfield from copilot test.The
ProviderSessionStartInputschema does not accept amodelfield—it only acceptsmodelSelection(optional). The test at lines 52-70 passesmodel: "claude-sonnet-4.6", which will fail schema validation. Update to usemodelSelectionwith the same pattern as other provider tests (codex, claudeAgent, cursor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/provider.test.ts` around lines 52 - 70, The test "accepts copilot provider payloads" passes an invalid `model` field to the schema; update the payload to use `modelSelection` instead (matching the other provider tests like codex/claudeAgent/cursor). In the call to decodeProviderSessionStartInput replace the `model: "claude-sonnet-4.6"` entry with `modelSelection: { modelId: "claude-sonnet-4.6" }` (or the same structure used by the other provider tests) so validation against ProviderSessionStartInput succeeds; keep the rest of the assertions (provider, providerOptions.copilot.cliPath/configDir, runtimeMode) unchanged.apps/server/src/provider/Layers/ProviderService.ts (1)
407-429:⚠️ Potential issue | 🟠 MajorPreserve the persisted
modelSelectionwhen a turn does not override it.Line 421 only re-writes
modelSelectionfrom the incoming turn. On the normal path where subsequent turns omit that override, this upsert clears the binding’s saved selection even though the previous payload is already being consulted forproviderOptions. That can break the new provider-aware persistence flow and make recovery fall back to the wrong model/options later.🛠️ Suggested fix
const turn = yield* routed.adapter.sendTurn(input); const sendTurnBinding = yield* directory.getBinding(input.threadId); + const sendTurnRuntimePayload = Option.getOrUndefined(sendTurnBinding)?.runtimePayload; + const persistedModelSelection = readPersistedModelSelection(sendTurnRuntimePayload); const sendTurnProviderOptions = readPersistedProviderOptions( - Option.getOrUndefined(sendTurnBinding)?.runtimePayload, + sendTurnRuntimePayload, ); const safeSendTurnProviderOptions = sendTurnProviderOptions !== undefined ? redactProviderOptions(sendTurnProviderOptions) : undefined; @@ ...(turn.resumeCursor !== undefined ? { resumeCursor: turn.resumeCursor } : {}), runtimePayload: { - ...(input.modelSelection !== undefined ? { modelSelection: input.modelSelection } : {}), + ...(input.modelSelection !== undefined + ? { modelSelection: input.modelSelection } + : persistedModelSelection !== undefined + ? { modelSelection: persistedModelSelection } + : {}), activeTurnId: turn.turnId, lastRuntimeEvent: "provider.sendTurn", lastRuntimeEventAt: new Date().toISOString(),🤖 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 407 - 429, The upsert currently overwrites any persisted modelSelection with only the incoming input.modelSelection, which clears the saved selection when a turn omits it; before calling directory.upsert (use symbols sendTurnBinding and directory.upsert), read the persisted modelSelection from sendTurnBinding.runtimePayload (via Option.getOrUndefined(sendTurnBinding)?.runtimePayload?.modelSelection) and include modelSelection in the new runtimePayload using the incoming value if present otherwise the persisted value (i.e. set modelSelection to input.modelSelection ?? persistedModelSelection), so existing persisted modelSelection is preserved when the turn does not override it.apps/server/src/git/Layers/GitManager.test.ts (1)
448-476:⚠️ Potential issue | 🟡 MinorAlign the helper’s
actionIdin both arguments.When a test passes
options.actionIdbut omitsinput.actionId, this helper still injects"test-action-id"into the request body. That diverges from the new websocket plumbing, which forwards the same id in both places, so these tests can miss regressions in code that readsinput.actionId.Suggested fix
function runStackedAction( manager: GitManagerShape, input: { cwd: string; action: "commit" | "commit_push" | "commit_push_pr"; actionId?: string; @@ }, options?: Parameters<GitManagerShape["runStackedAction"]>[1], ) { + const actionId = input.actionId ?? options?.actionId ?? "test-action-id"; return manager.runStackedAction( { ...input, - actionId: input.actionId ?? "test-action-id", + actionId, }, - options, + options ? { ...options, actionId: options.actionId ?? actionId } : undefined, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 448 - 476, The helper runStackedAction injects a default "test-action-id" into the request body even when options.actionId is provided, causing mismatched IDs between the input and options; change the injection to use input.actionId ?? options?.actionId ?? "test-action-id" so the input.actionId mirrors options.actionId when present (update the object passed to manager.runStackedAction in the runStackedAction helper), keeping references to the GitManagerShape.runStackedAction call and the runStackedAction helper function.apps/web/src/wsTransport.ts (1)
79-110:⚠️ Potential issue | 🟠 MajorReject requests immediately once the transport is disposed.
The new
timeoutMs: nullpath can create an immortal pending request here:request()stores it with no timer,send()no-ops becausethis.disposedis true, and nothing ever resolves or rejects it. That can leave long-running UI actions stuck forever.Suggested fix
async request<T = unknown>( method: string, params?: unknown, options?: RequestOptions, ): Promise<T> { + if (this.disposed || this.state === "disposed") { + throw new Error("Transport disposed"); + } if (typeof method !== "string" || method.length === 0) { throw new Error("Request method is required"); } const id = String(this.nextId++);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/wsTransport.ts` around lines 79 - 110, The request() path can leave promises unresolved when the transport is disposed because send() becomes a no-op and a null timeout creates a permanent pending entry; update request() to check this.disposed before allocating the id/pending entry and immediately reject (or return a rejected Promise) if disposed so no pending entry is created, and also ensure any code that sets pending (this.pending.set) is only reached when send(encoded) will actually run; reference the request method, this.disposed, this.send, this.pending, timeoutMs and REQUEST_TIMEOUT_MS when making the change.apps/server/src/orchestration/Layers/ProviderCommandReactor.ts (1)
313-325:⚠️ Potential issue | 🟠 MajorApply
ModelSelectionrestart checks to every"restart-session"provider.Right now option-only changes only force a restart for Claude. Any other provider whose session config depends on
modelSelection.optionswill keep the old session as long as the slug is unchanged.Suggested fix
const previousModelSelection = threadModelSelections.get(threadId); const shouldRestartForModelSelectionChange = - currentProvider === "claudeAgent" && - requestedModelSelection !== undefined && - !Equal.equals(previousModelSelection, requestedModelSelection); + requestedModelSelection !== undefined && + sessionModelSwitch === "restart-session" && + !Equal.equals(previousModelSelection ?? thread.modelSelection, requestedModelSelection);🤖 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 313 - 325, The code only triggers a restart on modelSelection option changes for ClaudeAgent; change the logic so option-only modelSelection changes trigger restarts for any provider whose sessionModelSwitch === "restart-session". Specifically, replace the provider-specific check using currentProvider === "claudeAgent" in the shouldRestartForModelSelectionChange computation with a check against sessionModelSwitch === "restart-session", keep the requestedModelSelection !== undefined and the Equal.equals(previousModelSelection, requestedModelSelection) comparison and ensure previousModelSelection is read from threadModelSelections.get(threadId) as before so the restart decision applies uniformly to all restart-session providers.apps/web/src/routes/_chat.settings.tsx (1)
909-922:⚠️ Potential issue | 🟠 MajorMake the remove actions keyboard reachable.
The preset delete control is
display: noneuntil hover, and the custom-model delete control stays fully transparent unless hovered. That makes these destructive actions undiscoverable or unreachable without a mouse.Suggested accessibility fix
- className="ml-0.5 hidden text-muted-foreground/50 hover:text-foreground group-hover:inline" + className="ml-0.5 inline-flex text-muted-foreground/50 opacity-0 transition-opacity hover:text-foreground group-hover:opacity-100 focus-visible:opacity-100" - className="shrink-0 opacity-0 transition-opacity group-hover:opacity-100 hover:opacity-100" + className="shrink-0 opacity-0 transition-opacity group-hover:opacity-100 focus-visible:opacity-100 hover:opacity-100"Also applies to: 1204-1211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 909 - 922, The delete buttons are hidden with "hidden" and opacity-only hover styles which makes them unreachable by keyboard; update the button rendering for the accent-preset remove control (the button that calls updateSettings and filters settings.customAccentPresets) and the similar custom-model delete control so they are not display:none but instead remain in the DOM and become visible on hover or keyboard focus — e.g. remove the "hidden" class, replace hover-only visibility with styles that show on :hover and :focus/:focus-visible (or add focus-visible:class equivalents) and ensure the element is keyboard-focusable (retain a semantic <button> and/or set tabIndex if needed) while keeping the aria-label for screen readers; apply the same change to the other instance noted in the comment.apps/server/src/provider/Layers/ClaudeAdapter.ts (1)
2756-2797:⚠️ Potential issue | 🟠 Major
fastMode,thinking, andeffectiveEffortare applied only at session creation; picker changes in subsequent turns are ignored.These traits are baked into
queryOptionsduringstartSession()(lines 2763–2775) but are never reapplied.sendTurn()only callssetModel()andsetPermissionMode()—no setters exist foreffort,fastMode, orthinkingon the query object. Since the sameClaudeSessionContextis reused across turns, any picker changes after the first turn become inert.resolveRoutableSession()returns immediately if a session exists without detecting or restarting based on runtime-state changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 2756 - 2797, The session-level flags (fastMode, thinking, effectiveEffort) are only set when building queryOptions in startSession (see queryOptions, getEffectiveClaudeCodeEffort) and never reapplied on subsequent turns, so changes from the picker are ignored because resolveRoutableSession returns an existing ClaudeSessionContext without reinitializing; update resolveRoutableSession/sendTurn to detect changes to modelSelection/options (effort, fastMode, thinking) and either (a) call new setters on the active ClaudeSessionContext (add setEffort/setFastMode/setThinking or a single applySettings method) or (b) restart the session by recreating queryOptions via startSession flow when those values differ, ensuring setModel/setPermissionMode are still applied as now. Ensure comparisons use the same normalization (trimOrNull/getEffectiveClaudeCodeEffort/hasEffortLevel) as used in startSession to decide when to reapply or restart.packages/shared/src/model.ts (2)
623-635:⚠️ Potential issue | 🟠 MajorDrop unsupported Codex traits when the model changes.
This path never validates
reasoningEffortorfastModeagainstcaps, so options from a previously selected Codex model can leak into one that does not support them. After introducing model-specific capabilities, a stale draft can still dispatch an invalid Codexoptionspayload.🤖 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 623 - 635, normalizeCodexModelOptions currently preserves reasoningEffort and fastMode from previous model without checking the new model's capabilities; update it to consult caps (from getModelCapabilities) before including those fields: only compute and include reasoningEffort when caps exposes/supports an allowed set (e.g., caps.reasoningEfforts or caps.supportsReasoningEffort) and the chosen value is in that allowed set (otherwise fall back to the model-specific default or omit), and only include fastMode when caps indicates fastMode support (e.g., caps.supportsFastMode); adjust the creation of nextOptions to conditionally spread reasoningEffort and fastMode based on those caps checks so unsupported traits are dropped when the model changes.
638-655:⚠️ Potential issue | 🟠 MajorDon't strip Claude effort for models that fall back to provider defaults.
When a Claude model is missing from
MODEL_CAPABILITIES_INDEX, callers ofgetReasoningEffortOptions()still get the provider-level effort list, but this normalizer drops the choseneffortbecausehasEffortLevel(caps, ...)is always false on the empty capability set. Custom or preview Claude models will look configurable and then dispatch without the selected effort.🤖 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 638 - 655, The normalizer in normalizeClaudeModelOptions is dropping a chosen effort when getModelCapabilities returns an empty/unknown capability set; change the effort determination so that resolved effort is preserved if the capabilities are empty (i.e., provider-level fallback) instead of requiring hasEffortLevel to be true. Concretely, in normalizeClaudeModelOptions adjust the condition that computes effort (referencing normalizeClaudeModelOptions, getModelCapabilities, hasEffortLevel, defaultReasoningEffort, isPromptInjected) so it accepts resolvedEffort when caps has no defined effort levels (or an explicit "unknown" / empty capability indicator) OR when hasEffortLevel(caps, resolvedEffort) is true, and still exclude the defaultReasoningEffort and prompt-injected values.
🟡 Minor comments (14)
apps/web/src/components/ui/sidebar.tsx-24-27 (1)
24-27:⚠️ Potential issue | 🟡 MinorRemove redundant
cookieStoredeclaration and update outdated commentThe ambient declaration is unnecessary—TypeScript's lib.dom (included in your
compilerOptions) already provides theCookieStoreinterface. The code comment "not yet in TypeScript's lib.dom" is outdated. The declaration can be safely removed.Additionally, the
cookieStore.set()call lacks error handling for edge cases (non-HTTPS contexts, older browser versions). Consider wrapping it in a try-catch or checking for availability withtypeof cookieStore !== 'undefined'before use, though modern browser support (Chrome 87+, Firefox 140+, Safari 18.4+) makes this a minor concern for most deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/sidebar.tsx` around lines 24 - 27, Remove the redundant ambient declaration for cookieStore (the "declare const cookieStore..." block) and delete the outdated comment in sidebar.tsx; instead, guard calls to cookieStore.set by checking availability (e.g., typeof cookieStore !== 'undefined') and wrap the call to cookieStore.set(...) in a try-catch (or conditional) to handle non-HTTPS/unsupported environments and surface errors. Ensure you update references to CookieStore to rely on the lib.dom types and only call cookieStore.set inside the guarded/try block so failures are handled gracefully.apps/web/src/components/CommandPalette.tsx-438-438 (1)
438-438:⚠️ Potential issue | 🟡 MinorFix formatting issue flagged by CI.
The pipeline indicates an Oxfmt formatting check failure. The keyword access
thread.modelSelection.modelis correct for the new ModelSelection structure, but the line may need reformatting to pass CI.🔧 Suggested fix
- keywords: [thread.modelSelection.model, projectNameById.get(thread.projectId) ?? "", thread.id], + keywords: [ + thread.modelSelection.model, + projectNameById.get(thread.projectId) ?? "", + thread.id, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/CommandPalette.tsx` at line 438, The CI formatter fails on the single-line keywords entry; reformat the keywords array in CommandPalette.tsx so each item is on its own line and spacing matches project style (e.g. put thread.modelSelection.model, projectNameById.get(thread.projectId) ?? "", and thread.id on separate lines inside the array) to satisfy Oxfmt while keeping the same values and order.apps/web/src/projectScripts.ts-66-73 (1)
66-73:⚠️ Potential issue | 🟡 MinorHandle empty/whitespace
worktreePathdefensively inprojectScriptCwd.At Line 72,
??treats""as valid and can return an invalid cwd. Prefer a trimmed truthy check before falling back.💡 Proposed fix
export function projectScriptCwd(input: { project: { cwd: string; }; worktreePath?: string | null; }): string { - return input.worktreePath ?? input.project.cwd; + const worktreePath = input.worktreePath?.trim(); + return worktreePath ? worktreePath : input.project.cwd; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/projectScripts.ts` around lines 66 - 73, projectScriptCwd currently returns input.worktreePath as-is using the nullish coalescing operator, which treats empty string or whitespace-only strings as valid; change the logic in projectScriptCwd to defensively check input.worktreePath by trimming and confirming it's non-empty (e.g., use worktreePath?.trim() and return the trimmed value only if truthy), otherwise fall back to input.project.cwd to avoid returning invalid cwd values.apps/web/src/store.test.ts-125-145 (1)
125-145:⚠️ Potential issue | 🟡 MinorFormatting check is failing for this file (CI blocker).
oxfmt --checkfailed; please run the formatter for this file before merge (the extra spacing around Line 144 is a likely contributor).Likely minimal fix in this segment
function makeReadModelProject( overrides: Partial<OrchestrationReadModel["projects"][number]>, ): OrchestrationReadModel["projects"][number] { return { @@ ...overrides, }; } - - describe("store pure functions", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/store.test.ts` around lines 125 - 145, The file fails oxfmt formatting (extra spacing around the makeReadModelProject block) — run the formatter or fix spacing so the file passes `oxfmt --check`; specifically open apps/web/src/store.test.ts, locate the makeReadModelProject function and its surrounding blank lines, remove the extraneous blank line(s) or re-run your project's formatting command (oxfmt) to normalize spacing and ensure the describe("store pure functions", ...) line follows the function with the expected single blank line/spacing so CI passes.apps/web/src/components/DiffPanel.tsx-634-640 (1)
634-640:⚠️ Potential issue | 🟡 MinorReplace
wrap-break-wordwith the standard Tailwind utilitybreak-words.Line 638 uses the undefined utility
wrap-break-word, which will have no effect. The standard Tailwind CSS utility for breaking long words isbreak-words.Suggested fix
- diffWordWrap - ? "overflow-auto whitespace-pre-wrap wrap-break-word" - : "overflow-auto", + diffWordWrap + ? "overflow-auto whitespace-pre-wrap break-words" + : "overflow-auto",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 634 - 640, In DiffPanel's JSX where the <pre> element builds className (look for the diffWordWrap branch in the className expression inside DiffPanel component), replace the invalid Tailwind utility "wrap-break-word" with the standard "break-words" so the long-word breaking takes effect; update the branch that currently returns "overflow-auto whitespace-pre-wrap wrap-break-word" to use "overflow-auto whitespace-pre-wrap break-words" (preserving other classes) so Tailwind applies the intended behavior.apps/web/src/components/chat/ContextWindowMeter.tsx-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorGuard the meter against non-finite percentages.
formatPercentage()already treatsNaN/Infinityas invalid, but Lines 18-21 and 72-74 still consumeusage.usedPercentagedirectly. If this snapshot ever contains a non-finite value, the SVG offset becomesNaNand the badge can literally renderNaN/Infinityinstead of falling back to token counts.🩹 Suggested fix
const { usage } = props; const usedPercentage = formatPercentage(usage.usedPercentage); - const normalizedPercentage = Math.max(0, Math.min(100, usage.usedPercentage ?? 0)); + const hasFinitePercentage = + usage.usedPercentage !== null && Number.isFinite(usage.usedPercentage); + const normalizedPercentage = hasFinitePercentage + ? Math.max(0, Math.min(100, usage.usedPercentage)) + : 0; const radius = 9.75; const circumference = 2 * Math.PI * radius; const dashOffset = circumference - (normalizedPercentage / 100) * circumference; @@ - {usage.usedPercentage !== null - ? Math.round(usage.usedPercentage) + {hasFinitePercentage + ? Math.round(normalizedPercentage) : formatContextWindowTokens(usage.usedTokens)}Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ContextWindowMeter.tsx` around lines 17 - 21, The meter calculation uses usage.usedPercentage directly which can be NaN/Infinity; change the normalization to first coerce and validate the raw value (e.g., const raw = usage.usedPercentage ?? 0; const safe = Number.isFinite(raw) ? raw : 0) then compute normalizedPercentage = Math.max(0, Math.min(100, safe)); use that safe/normalized value wherever used (normalizedPercentage, dashOffset/circumference calculations and the later SVG strokeDashoffset computation around the same logic at the other block referenced) and keep using formatPercentage(usage.usedPercentage) for display but ensure the math uses the finite-guarded value so the SVG never receives NaN/Infinity.apps/server/src/git/Layers/GitManager.test.ts-2085-2112 (1)
2085-2112:⚠️ Potential issue | 🟡 MinorThis test does not actually assert event order.
toContainplusarrayContainingonly proves the events exist somewhere in the stream. A reordered hook lifecycle would still pass, so the "ordered progress events" contract is currently untested.Suggested fix
- expect(events.map((event) => event.kind)).toContain("action_started"); - expect(events).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - kind: "phase_started", - phase: "commit", - }), - expect.objectContaining({ - kind: "hook_started", - hookName: "pre-commit", - }), - expect.objectContaining({ - kind: "hook_output", - text: "hook: start", - }), - expect.objectContaining({ - kind: "hook_output", - text: "hook: end", - }), - expect.objectContaining({ - kind: "hook_finished", - hookName: "pre-commit", - }), - expect.objectContaining({ - kind: "action_finished", - }), - ]), - ); + const orderedKinds = events + .filter((event) => + [ + "action_started", + "phase_started", + "hook_started", + "hook_output", + "hook_finished", + "action_finished", + ].includes(event.kind), + ) + .map((event) => event.kind); + + expect(orderedKinds).toEqual([ + "action_started", + "phase_started", + "hook_started", + "hook_output", + "hook_output", + "hook_finished", + "action_finished", + ]); + + expect(events).toEqual( + expect.arrayContaining([ + expect.objectContaining({ kind: "hook_output", text: "hook: start" }), + expect.objectContaining({ kind: "hook_output", text: "hook: end" }), + ]), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 2085 - 2112, The current assertions only verify presence, not order; update the test in GitManager.test.ts to assert the sequence of lifecycle events for the commit/hook by checking ordering (not just containment). For example, derive an ordered list from events (e.g., events.map(e => e.kind) and/or matching hookName/text) and assert that "phase_started" (phase: "commit") comes before "hook_started" (hookName: "pre-commit"), which comes before "hook_output" ("hook: start"), then "hook_output" ("hook: end"), then "hook_finished" ("pre-commit"), and finally "action_finished"; you can implement this by locating indices for each expected object (using event.kind plus hookName/text) and asserting the indices are strictly increasing instead of using expect.arrayContaining.apps/web/src/routes/_chat.settings.tsx-455-474 (1)
455-474:⚠️ Potential issue | 🟡 MinorTrack every resettable setting in
changedSettingLabels.This list omits settings that
restoreDefaults()still resets, includingaccentColor,customAccentPresets,providerLogoAppearance, andgitTextGenerationModelByProvider. If one of those is the only change, the new header-level restore button stays disabled and the confirmation text underreports what will be cleared.Also applies to: 563-598
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 455 - 474, changedSettingLabels doesn't include all keys that restoreDefaults() clears (accentColor, customAccentPresets, providerLogoAppearance, gitTextGenerationModelByProvider), so update the construction of changedSettingLabels to include checks for those four settings (e.g. compare settings.accentColor !== defaults.accentColor, settings.customAccentPresets length/contents, settings.providerLogoAppearance !== defaults.providerLogoAppearance, and settings.gitTextGenerationModelByProvider !== defaults.gitTextGenerationModelByProvider) so the header restore button and confirmation text reflect all resettable fields; apply the same additions to the other occurrence of changedSettingLabels later in the file (the block around the 563–598 range) so both places stay in sync with restoreDefaults().apps/server/src/provider/Layers/ClaudeAdapter.test.ts-1185-1194 (1)
1185-1194:⚠️ Potential issue | 🟡 MinorThis can still pass if the SDK prompt consumer never exits.
The assertion only checks that no error was captured after a fixed delay. If
stopSession()leaves thefor awaitloop hanging forever,promptConsumerErrorstaysundefinedand this regression test still passes. Record consumer completion and assert that it actually resolves before finishing the test.Also applies to: 1217-1229
🤖 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 1185 - 1194, The test currently spawns an async IIFE that iterates "for await (const _message of input.prompt)" and only checks that promptConsumerError is undefined after a delay, which can pass if the consumer never exits; change the test to track consumer completion by creating a Promise or completion flag that is resolved when the for-await loop exits (either normally or via catch) and rejected if an error occurs, update the IIFE to resolve/reject that promise (and still set promptConsumerError on catch), then await that completion promise (in addition to checking promptConsumerError) before finishing the test and asserting the consumer actually completed; reference the async IIFE that consumes input.prompt, the promptConsumerError variable, and stopSession() to ensure you wait for the consumer to finish.apps/web/src/components/Sidebar.logic.test.ts-365-390 (1)
365-390:⚠️ Potential issue | 🟡 Minor
...overridescurrently wipes out the mergedmodelSelection.If a caller passes a partial
overrides.modelSelection, the trailing spread replaces the whole object and drops the defaultprovider/model. This helper should destructuremodelSelectionthe same waymakeProject()already does.🛠️ Proposed fix
function makeThread(overrides: Partial<Thread> = {}): Thread { + const { modelSelection, ...rest } = overrides; return { id: ThreadId.makeUnsafe("thread-1"), codexThreadId: null, projectId: ProjectId.makeUnsafe("project-1"), title: "Thread", modelSelection: { provider: "codex", model: "gpt-5.4", - ...overrides?.modelSelection, + ...modelSelection, }, runtimeMode: DEFAULT_RUNTIME_MODE, interactionMode: DEFAULT_INTERACTION_MODE, session: null, messages: [], proposedPlans: [], error: null, createdAt: "2026-03-09T10:00:00.000Z", updatedAt: "2026-03-09T10:00:00.000Z", latestTurn: null, branch: null, worktreePath: null, turnDiffSummaries: [], activities: [], - ...overrides, + ...rest, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.logic.test.ts` around lines 365 - 390, The makeThread helper currently spreads ...overrides last which replaces the merged modelSelection object; change makeThread so it pulls modelSelection from overrides (e.g., const { modelSelection: overridesModelSelection, ...rest } = overrides) and then construct modelSelection by spreading defaults then overrides (modelSelection: { provider: "codex", model: "gpt-5.4", ...overridesModelSelection }), then spread the remaining rest of overrides into the returned Thread so partial overrides.modelSelection merge correctly without losing default provider/model.apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts-107-110 (1)
107-110:⚠️ Potential issue | 🟡 MinorMake the fake session reflect the requested
modelSelection.The mock
startSessionresponse always usesthreadModelSelection, even when the dispatched turn requested a differentmodelSelection. That makes the stored session diverge from the request under test and can hide regressions in the model-switch logic these tests are trying to cover.Also applies to: 124-135
🤖 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 107 - 110, The fake session returned by startSession in ProviderCommandReactor.test.ts always uses threadModelSelection instead of reflecting the dispatched turn's requested modelSelection, causing tests to miss model-switch regressions; update the mock startSession implementation to accept and propagate the requested modelSelection (use the input or dispatched turn's modelSelection when building the session) so the returned session's modelSelection matches the one in the test, updating any places referencing threadModelSelection to use the provided modelSelection value (affecting startSession mock and the two blocks around lines where modelSelection is set).apps/web/src/lib/contextWindow.ts-31-40 (1)
31-40:⚠️ Potential issue | 🟡 MinorDon't resurrect an older snapshot after a zero-token update.
This reverse scan skips
usedTokens === 0and keeps looking, so a reset/compaction activity can bring back an older non-zero snapshot and keep the meter stale. Treat the latest zero-token activity as authoritative instead of falling through to older data.Suggested fix
- if (usedTokens === null || usedTokens <= 0) { + if (usedTokens === null || usedTokens < 0) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/contextWindow.ts` around lines 31 - 40, The reverse-scan loop over activities is currently skipping entries where usedTokens === 0 and continues scanning older snapshots, which can resurrect an outdated non-zero snapshot; change the logic in the loop (the block that reads activity, payload, usedTokens) so that when you encounter a valid activity with usedTokens === 0 you treat it as authoritative and stop scanning (return or break and set the snapshot/meter state to indicate zero/no snapshot) instead of continuing to older entries; update the handling around variables activities, activity, payload, and usedTokens to immediately stop and apply the zero-token result.apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts-591-616 (1)
591-616:⚠️ Potential issue | 🟡 MinorThis test never actually requests an in-session model switch.
As written it only proves that the thread's default selection is reused. To cover "session model switch unsupported", the second dispatch needs a different
modelSelectionand the assertion should verify that the reactor still sends the original one.Suggested test change
await Effect.runPromise( harness.engine.dispatch({ type: "thread.turn.start", commandId: CommandId.makeUnsafe("cmd-turn-start-unsupported-2"), threadId: ThreadId.makeUnsafe("thread-1"), message: { messageId: asMessageId("user-message-unsupported-2"), role: "user", text: "second", attachments: [], }, + modelSelection: { + provider: "codex", + model: "gpt-5.3-codex", + }, interactionMode: DEFAULT_PROVIDER_INTERACTION_MODE, runtimeMode: "approval-required", createdAt: now, }), );🤖 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 591 - 616, The test currently dispatches a second "thread.turn.start" but never requests a different in-session model, so it only proves the default selection is reused; change the second harness.engine.dispatch call to include a different modelSelection (different provider and/or model than the thread default) to simulate an in-session switch request, then keep the assertion that harness.sendTurn.mock.calls[1]?.[0] still matches the original/default modelSelection; update references in the test to modify the dispatched payload's modelSelection and leave the expect against harness.sendTurn unchanged to assert the reactor rejects the in-session switch.apps/web/src/components/chat/TraitsPicker.browser.tsx-1-362 (1)
1-362:⚠️ Potential issue | 🟡 MinorRun
oxfmton this file before merging.CI is already red on the formatter check here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/TraitsPicker.browser.tsx` around lines 1 - 362, This file fails the project's formatter (oxfmt); run oxfmt over this test file to fix formatting errors and update the commit before merging. Specifically, format apps/web/src/components/chat/TraitsPicker.browser.tsx (which contains functions like ClaudeTraitsPickerHarness, mountClaudePicker, mountCodexPicker and the describe blocks for "TraitsPicker (Claude)" and "TraitsPicker (Codex)") so that CI's formatter check passes. Ensure you stage the formatted changes and re-run the formatter check locally.
🧹 Nitpick comments (6)
apps/web/src/lib/terminalFocus.test.ts (1)
35-56: Consider adding coverage for the.closest()terminal path.Current tests validate the helper-textarea path well, but adding one case for a connected non-helper element that still resolves via
.closest(".thread-terminal-drawer .xterm")would lock down the other success branch.➕ Suggested test addition
describe("isTerminalFocused", () => { + it("returns true for connected elements inside terminal container", () => { + const attached = new MockHTMLElement(); + attached.className = "some-other-class"; + attached.isConnected = true; + + globalThis.HTMLElement = MockHTMLElement as unknown as typeof HTMLElement; + globalThis.document = { activeElement: attached } as Document; + + expect(isTerminalFocused()).toBe(true); + }); + it("returns false for detached xterm helper textareas", () => { const detached = new MockHTMLElement(); detached.className = "xterm-helper-textarea";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/terminalFocus.test.ts` around lines 35 - 56, Add a test for isTerminalFocused that covers the .closest() path: create a MockHTMLElement instance that is not an xterm-helper-textarea but has isConnected = true and a mocked closest method that returns a truthy element for selector ".thread-terminal-drawer .xterm"; assign globalThis.HTMLElement to MockHTMLElement and globalThis.document.activeElement to that instance, then assert isTerminalFocused() returns true so the branch using element.closest(".thread-terminal-drawer .xterm") is exercised.apps/web/src/lib/threadProvider.test.ts (1)
34-40: Add a regression case that proves persisted provider is honoredThis fixture still passes via model-heuristic inference. Add a closed-session case where
modelSelection.providerconflicts with heuristic mapping so provider-aware fallback is actually verified.✅ Test addition example
describe("resolveThreadProvider", () => { it("falls back to the thread model when the session is closed", () => { expect( resolveThreadProvider({ modelSelection: { provider: "cursor", model: "composer" }, session: null, }), ).toBe("cursor"); }); + + it("prefers persisted modelSelection provider when session is closed", () => { + expect( + resolveThreadProvider({ + modelSelection: { provider: "cursor", model: "gpt-5.3-codex" }, + session: null, + }), + ).toBe("cursor"); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/threadProvider.test.ts` around lines 34 - 40, Add a regression test to verify that resolveThreadProvider honors an explicit persisted provider when the session is closed by adding a case where modelSelection.provider intentionally conflicts with the heuristic mapping (e.g., set modelSelection.provider to "cursor" while the heuristic for the given model would map a closed session to "thread"), call resolveThreadProvider with session: null and the conflicting modelSelection, and assert it returns the explicit provider ("cursor"); reference resolveThreadProvider and modelSelection.provider/modelSelection.model to locate where to add the test.apps/server/src/wsServer.ts (1)
646-670: Use the shared default-model map for the bootstrap fallback.Lines 652-669 hard-code
"gpt-5-codex"twice. That will drift the next time the Codex default changes upstream, and auto-bootstrapped projects/threads will start with a stale model selection. Pull the value fromDEFAULT_MODEL_BY_PROVIDERinstead.♻️ Suggested refactor
import { CommandId, + DEFAULT_MODEL_BY_PROVIDER, DEFAULT_PROVIDER_INTERACTION_MODE, type ClientOrchestrationCommand, type OrchestrationCommand, @@ let welcomeBootstrapProjectId: ProjectId | undefined; let welcomeBootstrapThreadId: ThreadId | undefined; + const defaultCodexModelSelection = { + provider: "codex" as const, + model: DEFAULT_MODEL_BY_PROVIDER.codex, + }; if (autoBootstrapProjectFromCwd) { @@ - bootstrapProjectDefaultModelSelection = { - provider: "codex" as const, - model: "gpt-5-codex", - }; + bootstrapProjectDefaultModelSelection = defaultCodexModelSelection; @@ - bootstrapProjectDefaultModelSelection = existingProject.defaultModelSelection ?? { - provider: "codex" as const, - model: "gpt-5-codex", - }; + bootstrapProjectDefaultModelSelection = + existingProject.defaultModelSelection ?? defaultCodexModelSelection;🤖 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 646 - 670, The code hard-codes "gpt-5-codex" when building bootstrapProjectDefaultModelSelection; replace those literals with the shared map lookup from DEFAULT_MODEL_BY_PROVIDER for provider "codex" so the bootstrap fallback stays in sync. Concretely, set bootstrapProjectDefaultModelSelection.model to DEFAULT_MODEL_BY_PROVIDER.codex (or DEFAULT_MODEL_BY_PROVIDER["codex"]) in both the new-project branch (before dispatch) and the existingProject fallback (where existingProject.defaultModelSelection is nullish), keeping the provider value as "codex" and ensuring DEFAULT_MODEL_BY_PROVIDER is imported/available.apps/server/src/persistence/Layers/ProjectionRepositories.test.ts (1)
75-91: Make the thread repository test self-contained.This case inserts a thread for
project-null-optionswithout creating that project first. It only passes if data leaks from the previous test or if the relation is currently unenforced.Suggested setup
Effect.gen(function* () { + const projects = yield* ProjectionProjectRepository; const threads = yield* ProjectionThreadRepository; const sql = yield* SqlClient.SqlClient; + yield* projects.upsert({ + projectId: ProjectId.makeUnsafe("project-null-options"), + title: "Null options project", + workspaceRoot: "/tmp/project-null-options", + defaultModelSelection: null, + scripts: [], + createdAt: "2026-03-24T00:00:00.000Z", + updatedAt: "2026-03-24T00:00:00.000Z", + deletedAt: null, + }); + yield* threads.upsert({ threadId: ThreadId.makeUnsafe("thread-null-options"), projectId: ProjectId.makeUnsafe("project-null-options"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/persistence/Layers/ProjectionRepositories.test.ts` around lines 75 - 91, The test inserts a thread with threads.upsert using ThreadId.makeUnsafe("thread-null-options") and ProjectId.makeUnsafe("project-null-options") but never creates the corresponding project, making the test reliant on leaked state; update the test to create the required project first (e.g., call the project repository/create helper or use projects.upsert/create with ProjectId.makeUnsafe("project-null-options") and appropriate fields) before calling threads.upsert so the test is self-contained and does not depend on prior tests or unenforced relations.apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.ts (1)
246-353: Key these migration assertions byevent_id, notrowid.Ordering by
rowidcouples this test to how the migration updates rows. A future table rewrite orINSERT ... SELECTimplementation can reshuffle rowids without changing correctness and make this fail noisily.Suggested refactor
- const eventRows = yield* sql<{ - readonly payloadJson: string; - }>` - SELECT payload_json AS "payloadJson" - FROM orchestration_events - ORDER BY rowid ASC - `; + const eventRows = yield* sql<{ + readonly eventId: string; + readonly payloadJson: string; + }>` + SELECT + event_id AS "eventId", + payload_json AS "payloadJson" + FROM orchestration_events + `; + const payloadsByEventId = new Map( + eventRows.map((row) => [row.eventId, JSON.parse(row.payloadJson)] as const), + ); - assert.deepStrictEqual(JSON.parse(eventRows[0]!.payloadJson), { + assert.deepStrictEqual(payloadsByEventId.get("event-project-created"), { projectId: "project-1", title: "Project", workspaceRoot: "/tmp/project", defaultModelSelection: { provider: "claudeAgent",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.ts` around lines 246 - 353, The test currently orders assertions by rowid (the SQL in the query that populates eventRows) which is fragile; change the SELECT to include and ORDER BY event_id (or otherwise key the returned rows by the event_id column) so the assertions reference events by their event_id instead of relying on physical rowid ordering—update the SQL that fills eventRows and the subsequent assertions that use eventRows[...] to look up rows by matching event_id values (symbols: eventRows, the SQL SELECT from orchestration_events, and the assertions that JSON-parse eventRows[i].payloadJson).apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts (1)
2301-2325: Seed a Claude-bound thread/session in the Claude usage test.
createHarness()leavesthread-1bound to Codex, but this case emits aclaudeAgentevent. That can pass without exercising the Claude-bound path and will get brittle if provider-mismatch guards tighten later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts` around lines 2301 - 2325, The test emits a claudeAgent usage event for thread-1 but createHarness() leaves thread-1 bound to Codex; before calling harness.emit(...) for the "thread.token-usage.updated" event, seed a Claude-bound session for thread-1 (so the Claude-specific path is exercised) by creating or emitting a provider-binding event for that thread (e.g., emit a "thread.provider.bound" or similar event with threadId: asThreadId("thread-1") and provider: "claudeAgent"), or update createHarness to initialize thread-1 with provider "claudeAgent" so the token-usage handler in ProviderRuntimeIngestion sees a Claude-bound thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 532365dd-7149-4610-b3be-0d62f5a8af23
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (125)
.gitignoreapps/desktop/package.jsonapps/server/integration/orchestrationEngine.integration.test.tsapps/server/package.jsonapps/server/src/ampServerManager.tsapps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.tsapps/server/src/geminiCliServerManager.test.tsapps/server/src/geminiCliServerManager.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Layers/SessionTextGeneration.tsapps/server/src/git/Services/GitCore.tsapps/server/src/git/Services/GitManager.tsapps/server/src/kilo/types.tsapps/server/src/kiloServerManager.tsapps/server/src/opencode/types.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.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/commandInvariants.test.tsapps/server/src/orchestration/decider.projectScripts.test.tsapps/server/src/orchestration/decider.tsapps/server/src/orchestration/projector.test.tsapps/server/src/orchestration/projector.tsapps/server/src/persistence/Layers/OrchestrationEventStore.test.tsapps/server/src/persistence/Layers/ProjectionProjects.tsapps/server/src/persistence/Layers/ProjectionRepositories.test.tsapps/server/src/persistence/Layers/ProjectionThreads.tsapps/server/src/persistence/Layers/Sqlite.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.tsapps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.tsapps/server/src/persistence/Services/ProjectionProjects.tsapps/server/src/persistence/Services/ProjectionThreads.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/terminal/Services/Manager.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/package.jsonapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.logic.tsapps/web/src/components/ChatView.tsxapps/web/src/components/CommandPalette.tsxapps/web/src/components/DiffPanel.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ChatHeader.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/ContextWindowMeter.tsxapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/ProviderHealthBanner.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/components/chat/TraitsPicker.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/components/ui/select.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/lib/contextWindow.test.tsapps/web/src/lib/contextWindow.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/terminalFocus.test.tsapps/web/src/lib/terminalFocus.tsapps/web/src/lib/threadDraftDefaults.test.tsapps/web/src/lib/threadDraftDefaults.tsapps/web/src/lib/threadProvider.test.tsapps/web/src/lib/threadProvider.tsapps/web/src/projectScripts.test.tsapps/web/src/projectScripts.tsapps/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/src/worktreeCleanup.test.tsapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.test.tsapps/web/src/wsTransport.tspackages/contracts/package.jsonpackages/contracts/src/git.test.tspackages/contracts/src/git.tspackages/contracts/src/ipc.tspackages/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/contracts/src/ws.test.tspackages/contracts/src/ws.tspackages/shared/src/model.test.tspackages/shared/src/model.ts
💤 Files with no reviewable changes (4)
- apps/web/src/components/chat/ClaudeTraitsPicker.browser.tsx
- apps/web/src/components/chat/CodexTraitsPicker.browser.tsx
- apps/web/src/components/chat/CodexTraitsPicker.tsx
- apps/web/src/components/chat/ClaudeTraitsPicker.tsx
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts
Show resolved
Hide resolved
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts
Show resolved
Hide resolved
Comment on lines
+2931
to
2935
| if (modelSelection?.model) { | ||
| yield* Effect.tryPromise({ | ||
| try: () => context.query.setModel(input.model), | ||
| try: () => context.query.setModel(modelSelection.model), | ||
| catch: (cause) => toRequestError(input.threadId, "turn/setModel", cause), | ||
| }); |
There was a problem hiding this comment.
Clear the cached usage baseline after Claude setModel() succeeds.
Line 2933 can switch the live session to a different Claude model, but lastKnownContextWindow / lastKnownTokenUsage survive that switch. The next task_progress snapshot is then normalized against the previous model's limits, and context.session.model keeps reporting the old slug.
Possible fix
if (modelSelection?.model) {
+ const modelChanged = context.session.model !== modelSelection.model;
yield* Effect.tryPromise({
try: () => context.query.setModel(modelSelection.model),
catch: (cause) => toRequestError(input.threadId, "turn/setModel", cause),
});
+ if (modelChanged) {
+ context.lastKnownContextWindow = undefined;
+ context.lastKnownTokenUsage = undefined;
+ context.session = {
+ ...context.session,
+ model: modelSelection.model,
+ updatedAt: yield* nowIso,
+ };
+ }
}📝 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 (modelSelection?.model) { | |
| yield* Effect.tryPromise({ | |
| try: () => context.query.setModel(input.model), | |
| try: () => context.query.setModel(modelSelection.model), | |
| catch: (cause) => toRequestError(input.threadId, "turn/setModel", cause), | |
| }); | |
| if (modelSelection?.model) { | |
| const modelChanged = context.session.model !== modelSelection.model; | |
| yield* Effect.tryPromise({ | |
| try: () => context.query.setModel(modelSelection.model), | |
| catch: (cause) => toRequestError(input.threadId, "turn/setModel", cause), | |
| }); | |
| if (modelChanged) { | |
| context.lastKnownContextWindow = undefined; | |
| context.lastKnownTokenUsage = undefined; | |
| context.session = { | |
| ...context.session, | |
| model: modelSelection.model, | |
| updatedAt: yield* nowIso, | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 2931 - 2935,
After a successful context.query.setModel(...) call in the setModel branch,
clear the cached usage baseline by resetting lastKnownContextWindow and
lastKnownTokenUsage to undefined and update context.session.model to the new
model slug (modelSelection.model) so subsequent task_progress normalization and
reported session model reflect the new Claude model; perform these resets
immediately after the Effect.tryPromise succeeds (in the same branch that calls
setModel) to ensure snapshots are normalized against the new model limits.
Comment on lines
+1362
to
+1367
| ...(input.modelSelection?.provider === "codex" | ||
| ? { model: input.modelSelection.model } | ||
| : {}), | ||
| ...(input.modelSelection?.provider === "codex" && input.modelSelection.options?.fastMode | ||
| ? { serviceTier: "fast" } | ||
| : {}), |
There was a problem hiding this comment.
Reject cross-provider modelSelection values instead of silently dropping them.
If modelSelection.provider is anything other than "codex", these branches just omit model/serviceTier and continue with the Codex default. That turns a caller bug into a wrong-model request. Fail fast here, and mirror the same guard in both startSession and sendTurn.
Suggested guard for startSession (apply the same check in sendTurn)
+ if (
+ input.modelSelection?.provider !== undefined &&
+ input.modelSelection.provider !== PROVIDER
+ ) {
+ return Effect.fail(
+ new ProviderAdapterValidationError({
+ provider: PROVIDER,
+ operation: "startSession",
+ issue: `Expected modelSelection.provider '${PROVIDER}' but received '${input.modelSelection.provider}'.`,
+ }),
+ );
+ }
+
const managerInput: CodexAppServerStartSessionInput = {
threadId: input.threadId,
provider: "codex",Also applies to: 1423-1433
🤖 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 1362 - 1367,
The code silently ignores input.modelSelection when modelSelection.provider !==
"codex", producing requests with the wrong model; update CodexAdapter to
validate and reject cross-provider modelSelection in both startSession and
sendTurn: add a guard that if input.modelSelection?.provider is defined and !==
"codex" then throw or return a rejected Error (e.g., "modelSelection.provider
must be 'codex' for CodexAdapter"), otherwise continue to apply
model/serviceTier; reference the methods startSession and sendTurn and the
modelSelection property to locate where to add this check so callers fail fast
instead of silently dropping values.
Comment on lines
+251
to
+257
| const GitActionHookOutputEvent = Schema.Struct({ | ||
| ...GitActionProgressBase.fields, | ||
| kind: Schema.Literal("hook_output"), | ||
| hookName: Schema.NullOr(TrimmedNonEmptyStringSchema), | ||
| stream: GitActionProgressStream, | ||
| text: TrimmedNonEmptyStringSchema, | ||
| }); |
There was a problem hiding this comment.
Don't use a trimmed contract for raw hook output.
hook_output.text is modeling verbatim stdout/stderr lines, but TrimmedNonEmptyStringSchema will reject legitimate output that starts or ends with whitespace. That means valid hook output can fail validation and disappear from the progress stream.
🛠️ Proposed fix
const GitActionHookOutputEvent = Schema.Struct({
...GitActionProgressBase.fields,
kind: Schema.Literal("hook_output"),
hookName: Schema.NullOr(TrimmedNonEmptyStringSchema),
stream: GitActionProgressStream,
- text: TrimmedNonEmptyStringSchema,
+ text: Schema.String,
});📝 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
| const GitActionHookOutputEvent = Schema.Struct({ | |
| ...GitActionProgressBase.fields, | |
| kind: Schema.Literal("hook_output"), | |
| hookName: Schema.NullOr(TrimmedNonEmptyStringSchema), | |
| stream: GitActionProgressStream, | |
| text: TrimmedNonEmptyStringSchema, | |
| }); | |
| const GitActionHookOutputEvent = Schema.Struct({ | |
| ...GitActionProgressBase.fields, | |
| kind: Schema.Literal("hook_output"), | |
| hookName: Schema.NullOr(TrimmedNonEmptyStringSchema), | |
| stream: GitActionProgressStream, | |
| text: Schema.String, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/contracts/src/git.ts` around lines 251 - 257, The
GitActionHookOutputEvent currently uses TrimmedNonEmptyStringSchema for the text
field which trims and rejects leading/trailing-whitespace lines; replace that
schema with an untrimmed string schema (e.g., Schema.String) so verbatim
stdout/stderr lines are accepted (update the text field in
GitActionHookOutputEvent and remove/reference to TrimmedNonEmptyStringSchema
accordingly).
Comment on lines
+514
to
+536
| export const MODEL_CAPABILITIES_INDEX = Object.fromEntries( | ||
| Object.entries(MODEL_OPTIONS_BY_PROVIDER).map(([provider, models]) => [ | ||
| provider, | ||
| Object.fromEntries( | ||
| (models as readonly ModelDefinition[]) | ||
| .filter((m): m is ModelDefinition & { capabilities: ModelCapabilities } => m.capabilities != null) | ||
| .map((m) => [m.slug, m.capabilities]), | ||
| ), | ||
| ]), | ||
| ) as unknown as Record<ProviderKind, Record<string, ModelCapabilities>>; | ||
|
|
||
| // ── Provider display names ──────────────────────────────────────────── | ||
|
|
||
| export const PROVIDER_DISPLAY_NAMES: Record<ProviderKind, string> = { | ||
| codex: "Codex", | ||
| copilot: "Copilot", | ||
| claudeAgent: "Claude", | ||
| cursor: "Cursor", | ||
| opencode: "OpenCode", | ||
| geminiCli: "Gemini CLI", | ||
| amp: "Amp", | ||
| kilo: "Kilo", | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move this derived/provider metadata out of packages/contracts.
MODEL_CAPABILITIES_INDEX is computed runtime state, and PROVIDER_DISPLAY_NAMES is presentation metadata. That pushes the contracts package beyond raw definitions and makes the boundary harder to keep clean. Keep the base model definitions here, but build indexes/display labels in a shared/runtime module instead.
As per coding guidelines, packages/contracts should "maintain schema-only definitions—do not add runtime logic."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/contracts/src/model.ts` around lines 514 - 536, The file defines
runtime-derived metadata (MODEL_CAPABILITIES_INDEX) and presentation strings
(PROVIDER_DISPLAY_NAMES) inside packages/contracts which should only contain
schema/definitions; move both symbols out of packages/contracts into a new or
existing shared/runtime module (e.g., shared/model-metadata or
runtime/model-index) and export them from there, leaving only the raw
ModelDefinition and MODEL_OPTIONS_BY_PROVIDER in contracts; update any
imports/usages of MODEL_CAPABILITIES_INDEX and PROVIDER_DISPLAY_NAMES to import
from the new shared module so contracts contains only schema-only definitions.
- Fix 016_CanonicalizeModelSelections migration to detect all 8 providers (copilot, cursor, opencode, geminiCli, amp, kilo) instead of collapsing them into codex - Fix composerDraftStore normalizeModelSelection to handle all provider options dynamically instead of only codex/claudeAgent - Fix threadProvider.ts to use modelSelection.provider when session is null instead of inferring from slug only - Fix store.ts to always compute defaultModelSelection from read model data so server-side changes propagate correctly - Fix GitManager.ts to pass input.filePaths into runFeatureBranchStep - Fix useHandleNewThread to apply model overrides when reusing existing draft threads, not just new ones - Fix formatting across 9 files
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts (1)
18-24:⚠️ Potential issue | 🟠 MajorIncomplete provider inference still rewrites supported providers to
codex.These branches still never emit
copilot,opencode,amp, orkilo, so any legacy row/event missing explicit provider metadata will be canonicalized ascodex. That also makes the nested options lookup at Lines 117-133 and Lines 190-206 unwrap the wrong child object for those same records. Please extend the inference to all supported providers, or prefer the sole key present in the legacy options map before falling back.Also applies to: 47-54, 88-100, 117-133, 161-173, 190-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts` around lines 18 - 24, The CASE that maps lower(default_model) to provider strings currently only covers claude/gemini/composer/kimi and falls back to 'codex', causing legacy rows to be mis-tagged; update all similar CASE branches (the default_model -> provider mappings) to also detect 'copilot', 'opencode', 'amp', and 'kilo' (and any other supported provider names) so they emit the correct provider instead of 'codex', and change the downstream canonicalization logic that unwraps nested options to, before falling back to the hardcoded default, inspect the legacy options map and return its sole key when exactly one key exists; apply this change to every affected block referenced (the CASE mappings and the nested options lookups).
🧹 Nitpick comments (3)
apps/web/src/components/Sidebar.tsx (1)
1540-1862: Consider extractingrenderProjectItemto a separate component.This inline function is approximately 320 lines and captures many component-scope values. While functionally correct, extracting it to a dedicated
ProjectItemcomponent would:
- Improve readability and testability
- Allow React to better optimize re-renders via memoization
- Reduce the cognitive load when maintaining the Sidebar component
This is optional and can be deferred given the scope of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 1540 - 1862, Extract the large inline renderProjectItem function into a new React component (e.g., ProjectItem) to improve readability and enable memoization; move the JSX and all local logic currently inside renderProjectItem into ProjectItem and pass in needed props such as project, threads (or projectThreads), appSettings, routeThreadId, expandedThreadListsByProject, selectedThreadIds, renamingThreadId, renamingTitle, renamingInputRef, prByThreadId, terminalStateByThreadId, isManualProjectSorting, dragHandleProps, and all handler callbacks used inside (handleThreadClick, handleThreadContextMenu, handleProjectTitleClick/KeyDown, handleProjectContextMenu, handleNewThread, expandThreadListForProject, collapseThreadListForProject, commitRename, cancelRename, clearSelection, setSelectionAnchor, navigate); keep utility calls (sortThreadsForSidebar, resolveThreadStatusPill, resolveProjectStatusIndicator, prStatusIndicator, resolveThreadProvider, selectThreadTerminalState, terminalStatusFromRunningIds, formatRelativeTime, derivePendingApprovals/UserInputs) available either by importing them or by passing needed results as props, and wrap the new ProjectItem with React.memo to limit re-renders.apps/web/src/routes/_chat.settings.tsx (2)
438-440: Non-null assertion is safe but could be defensive.The
!assertion onselectedCustomModelProviderSettingsis safe becauseselectedCustomModelProvideris initialized to"codex"and only updated to values fromMODEL_PROVIDER_SETTINGS. For extra safety, consider a fallback.♻️ Optional: Add fallback for defensive coding
- const selectedCustomModelProviderSettings = MODEL_PROVIDER_SETTINGS.find( - (providerSettings) => providerSettings.provider === selectedCustomModelProvider, - )!; + const selectedCustomModelProviderSettings = + MODEL_PROVIDER_SETTINGS.find( + (providerSettings) => providerSettings.provider === selectedCustomModelProvider, + ) ?? MODEL_PROVIDER_SETTINGS[0]!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 438 - 440, The non-null assertion on selectedCustomModelProviderSettings (from MODEL_PROVIDER_SETTINGS.find(...)) is fragile; replace it with a defensive fallback by handling the case where find returns undefined—e.g., compute selectedCustomModelProviderSettings = MODEL_PROVIDER_SETTINGS.find(...) ?? MODEL_PROVIDER_SETTINGS[0] (or a safe default), or explicitly throw a clear error if no match is acceptable; ensure references to selectedCustomModelProvider and MODEL_PROVIDER_SETTINGS remain unchanged and update any downstream usage to assume the fallback value.
603-640: Consider extracting the "Restore defaults" button.The same button markup is duplicated in both the Electron and non-Electron headers. This is a minor refactor opportunity to reduce duplication.
♻️ Optional: Extract shared button
const RestoreDefaultsButton = () => ( <Button size="xs" variant="outline" disabled={changedSettingLabels.length === 0} onClick={() => void restoreDefaults()} > <RotateCcwIcon className="size-3.5" /> Restore defaults </Button> );Then use
<RestoreDefaultsButton />in both headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 603 - 640, Extract the duplicated "Restore defaults" button into a small React component (e.g., RestoreDefaultsButton) and use it in both conditional blocks to remove duplication; the component should render the same Button with RotateCcwIcon, call restoreDefaults on click, and compute disabled using changedSettingLabels.length === 0 (or accept a disabled prop tied to changedSettingLabels if you prefer props), so replace the two inline Button usages with <RestoreDefaultsButton /> referencing the existing restoreDefaults and changedSettingLabels symbols.
🤖 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/git/Layers/GitManager.ts`:
- Around line 824-841: onHookFinished currently destructures durationMs but that
value is never provided by GitCommitProgress.onHookFinished/GitCore.commit so
emitted hook_finished events get undefined for durationMs; update the
onHookFinished handler (in GitManager.ts) to normalize the value before emitting
by using a default of null when durationMs is undefined (e.g., const
normalizedDuration = durationMs ?? null) and pass that to emit, or alternatively
plumb the duration through GitCore.commit/GitCommitProgress.onHookFinished so
durationMs is a number | null; update references to currentHookName and emit to
use the normalized value to keep the hook_finished payload shape consistent.
In `@apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts`:
- Around line 38-59: The migration currently writes
{"provider":"codex","model":null} when the source model column is NULL, making
ProjectionThreads (deserialized as ModelSelection) unreadable; change the
migration logic in the projection_threads UPDATE to skip rows where model IS
NULL or substitute the same default model string used by the fallback (e.g., use
COALESCE(model, 'codex')) so model_selection_json never contains null for
"model"; apply the same fix to the other affected blocks that set
model_selection_json or rebuild model (the later sections around the second
UPDATE/INSERT and the repair logic referenced in the review).
In `@apps/web/src/components/chat/TraitsPicker.browser.tsx`:
- Line 188: The test title string in the test case it("shows a th inking on/off
dropdown for Haiku", ...) contains a typo splitting "thinking"; update the test
description to "shows a thinking on/off dropdown for Haiku" (or similar correct
phrasing) wherever that exact string appears (e.g., the it(...) call in
TraitsPicker.browser test) to restore readability and consistency.
- Around line 138-146: The Claude suite's afterEach resets only in-memory state
(useComposerDraftStore) but doesn't clear persisted draft storage; update the
afterEach cleanup to also clear the persisted drafts by invoking the store's
persistence clear method or removing the draft key(s) from storage (e.g., call
the persistence clear API on useComposerDraftStore or remove
localStorage/sessionStorage keys used for drafts) so no drafts leak into
subsequent browser tests.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 563-598: The confirmation dialog is misleading because
restoreDefaults() calls resetSettings() which resets all settings
(DEFAULT_APP_SETTINGS) while the message only lists changedSettingLabels; update
the function to either (A) compute and include every actually-modified key in
changedSettingLabels before showing the dialog (compare current app settings to
DEFAULT_APP_SETTINGS and expand changedSettingLabels), or (B) replace the dialog
text to warn that all settings will be reset (e.g., "This will reset all
settings to defaults"), or (C) implement a targeted reset that only clears the
keys present in changedSettingLabels instead of calling resetSettings(); locate
restoreDefaults(), changedSettingLabels, resetSettings(), and
DEFAULT_APP_SETTINGS in the file to apply one of these fixes and ensure the
dialog and actual behavior match.
- Around line 443-474: totalCustomModels currently only sums
settings.customCodexModels and settings.customClaudeModels but
savedCustomModelRows is built from MODEL_PROVIDER_SETTINGS; change
totalCustomModels to count all custom models by iterating
MODEL_PROVIDER_SETTINGS and summing getCustomModelsForProvider(settings,
providerSettings.provider). Use that new totalCustomModels wherever you rely on
the count (e.g., reset button visibility) and update the "Custom models"
condition in changedSettingLabels to check totalCustomModels > 0 instead of only
codex/claude lengths so all providers are covered (references:
totalCustomModels, MODEL_PROVIDER_SETTINGS, getCustomModelsForProvider,
savedCustomModelRows, changedSettingLabels).
- Around line 1108-1119: The resetAction currently only resets customCodexModels
and customClaudeModels; update it to clear all custom model state by
constructing an updateSettings payload that resets every custom model key (not
just customCodexModels/customClaudeModels) — e.g., derive the keys from the
defaults object (all properties starting with "custom") or from
savedCustomModelRows and set each corresponding setting to defaults.<property>
so call updateSettings({...allCustomDefaults}) inside the SettingResetButton
onClick, and keep the existing calls to setCustomModelErrorByProvider({}) and
setShowAllCustomModels(false); reference functions/props: resetAction,
SettingResetButton, updateSettings, defaults, savedCustomModelRows,
setCustomModelErrorByProvider, setShowAllCustomModels.
---
Duplicate comments:
In `@apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts`:
- Around line 18-24: The CASE that maps lower(default_model) to provider strings
currently only covers claude/gemini/composer/kimi and falls back to 'codex',
causing legacy rows to be mis-tagged; update all similar CASE branches (the
default_model -> provider mappings) to also detect 'copilot', 'opencode', 'amp',
and 'kilo' (and any other supported provider names) so they emit the correct
provider instead of 'codex', and change the downstream canonicalization logic
that unwraps nested options to, before falling back to the hardcoded default,
inspect the legacy options map and return its sole key when exactly one key
exists; apply this change to every affected block referenced (the CASE mappings
and the nested options lookups).
---
Nitpick comments:
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 1540-1862: Extract the large inline renderProjectItem function
into a new React component (e.g., ProjectItem) to improve readability and enable
memoization; move the JSX and all local logic currently inside renderProjectItem
into ProjectItem and pass in needed props such as project, threads (or
projectThreads), appSettings, routeThreadId, expandedThreadListsByProject,
selectedThreadIds, renamingThreadId, renamingTitle, renamingInputRef,
prByThreadId, terminalStateByThreadId, isManualProjectSorting, dragHandleProps,
and all handler callbacks used inside (handleThreadClick,
handleThreadContextMenu, handleProjectTitleClick/KeyDown,
handleProjectContextMenu, handleNewThread, expandThreadListForProject,
collapseThreadListForProject, commitRename, cancelRename, clearSelection,
setSelectionAnchor, navigate); keep utility calls (sortThreadsForSidebar,
resolveThreadStatusPill, resolveProjectStatusIndicator, prStatusIndicator,
resolveThreadProvider, selectThreadTerminalState, terminalStatusFromRunningIds,
formatRelativeTime, derivePendingApprovals/UserInputs) available either by
importing them or by passing needed results as props, and wrap the new
ProjectItem with React.memo to limit re-renders.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 438-440: The non-null assertion on
selectedCustomModelProviderSettings (from MODEL_PROVIDER_SETTINGS.find(...)) is
fragile; replace it with a defensive fallback by handling the case where find
returns undefined—e.g., compute selectedCustomModelProviderSettings =
MODEL_PROVIDER_SETTINGS.find(...) ?? MODEL_PROVIDER_SETTINGS[0] (or a safe
default), or explicitly throw a clear error if no match is acceptable; ensure
references to selectedCustomModelProvider and MODEL_PROVIDER_SETTINGS remain
unchanged and update any downstream usage to assume the fallback value.
- Around line 603-640: Extract the duplicated "Restore defaults" button into a
small React component (e.g., RestoreDefaultsButton) and use it in both
conditional blocks to remove duplication; the component should render the same
Button with RotateCcwIcon, call restoreDefaults on click, and compute disabled
using changedSettingLabels.length === 0 (or accept a disabled prop tied to
changedSettingLabels if you prefer props), so replace the two inline Button
usages with <RestoreDefaultsButton /> referencing the existing restoreDefaults
and changedSettingLabels symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33cb8f7f-2896-4ccd-a68d-35a384a3b286
📒 Files selected for processing (14)
apps/server/src/git/Layers/GitManager.tsapps/server/src/kiloServerManager.tsapps/server/src/opencodeServerManager.tsapps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.tsapps/web/src/components/CommandPalette.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/composerDraftStore.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/lib/threadProvider.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/store.test.tsapps/web/src/store.tspackages/contracts/src/model.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/components/CommandPalette.tsx
- apps/server/src/kiloServerManager.ts
- apps/web/src/hooks/useHandleNewThread.ts
- apps/web/src/store.test.ts
- apps/web/src/composerDraftStore.ts
- packages/contracts/src/model.ts
Comment on lines
+824
to
+841
| onHookFinished: ({ | ||
| hookName, | ||
| exitCode, | ||
| durationMs, | ||
| }: { | ||
| hookName: string; | ||
| exitCode: number | null; | ||
| durationMs: number | null; | ||
| }) => { | ||
| if (currentHookName === hookName) { | ||
| currentHookName = null; | ||
| } | ||
| return emit({ | ||
| kind: "hook_finished", | ||
| hookName, | ||
| exitCode, | ||
| durationMs, | ||
| }); |
There was a problem hiding this comment.
durationMs is never populated here.
GitCommitProgress.onHookFinished currently only exposes { hookName, exitCode } in apps/server/src/git/Services/GitCore.ts, and GitCore.commit just forwards that shape. Destructuring durationMs here therefore yields undefined, so hook_finished events can publish a payload shape this code treats as number | null. Either plumb the duration through GitCore.commit first or emit null here until that contract exists.
Suggested fix
onHookFinished: ({
hookName,
exitCode,
- durationMs,
}: {
hookName: string;
exitCode: number | null;
- durationMs: number | null;
}) => {
if (currentHookName === hookName) {
currentHookName = null;
}
return emit({
kind: "hook_finished",
hookName,
exitCode,
- durationMs,
+ durationMs: 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
| onHookFinished: ({ | |
| hookName, | |
| exitCode, | |
| durationMs, | |
| }: { | |
| hookName: string; | |
| exitCode: number | null; | |
| durationMs: number | null; | |
| }) => { | |
| if (currentHookName === hookName) { | |
| currentHookName = null; | |
| } | |
| return emit({ | |
| kind: "hook_finished", | |
| hookName, | |
| exitCode, | |
| durationMs, | |
| }); | |
| onHookFinished: ({ | |
| hookName, | |
| exitCode, | |
| }: { | |
| hookName: string; | |
| exitCode: number | null; | |
| }) => { | |
| if (currentHookName === hookName) { | |
| currentHookName = null; | |
| } | |
| return emit({ | |
| kind: "hook_finished", | |
| hookName, | |
| exitCode, | |
| durationMs: null, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitManager.ts` around lines 824 - 841,
onHookFinished currently destructures durationMs but that value is never
provided by GitCommitProgress.onHookFinished/GitCore.commit so emitted
hook_finished events get undefined for durationMs; update the onHookFinished
handler (in GitManager.ts) to normalize the value before emitting by using a
default of null when durationMs is undefined (e.g., const normalizedDuration =
durationMs ?? null) and pass that to emit, or alternatively plumb the duration
through GitCore.commit/GitCommitProgress.onHookFinished so durationMs is a
number | null; update references to currentHookName and emit to use the
normalized value to keep the hook_finished payload shape consistent.
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+563
to
+598
| async function restoreDefaults() { | ||
| if (changedSettingLabels.length === 0) return; | ||
|
|
||
| const api = readNativeApi(); | ||
| const confirmed = await (api ?? ensureNativeApi()).dialogs.confirm( | ||
| ["Restore default settings?", `This will reset: ${changedSettingLabels.join(", ")}.`].join( | ||
| "\n", | ||
| ), | ||
| ); | ||
| if (!confirmed) return; | ||
|
|
||
| setTheme("system"); | ||
| resetSettings(); | ||
| setOpenInstallProviders({ | ||
| codex: false, | ||
| copilot: false, | ||
| claudeAgent: false, | ||
| cursor: false, | ||
| opencode: false, | ||
| geminiCli: false, | ||
| amp: false, | ||
| kilo: false, | ||
| }); | ||
| setSelectedCustomModelProvider("codex"); | ||
| setCustomModelInputByProvider({ | ||
| codex: "", | ||
| copilot: "", | ||
| claudeAgent: "", | ||
| cursor: "", | ||
| opencode: "", | ||
| geminiCli: "", | ||
| amp: "", | ||
| kilo: "", | ||
| }); | ||
| setCustomModelErrorByProvider({}); | ||
| } |
There was a problem hiding this comment.
Misleading confirmation dialog — resetSettings() resets more than listed.
The confirmation message says "This will reset: [changedSettingLabels]", but resetSettings() actually resets all settings to DEFAULT_APP_SETTINGS (per appSettings.ts:644-652). This includes fields not tracked in changedSettingLabels:
accentColorandcustomAccentPresetssidebarProjectSortOrderandsidebarThreadSortOrderbackgroundColorOverride,foregroundColorOverrideuiFont,codeFont,uiFontSize,codeFontSizegitTextGenerationModelByProviderproviderAccentColors,contrast,translucency
Users could lose customizations without warning.
Consider one of these approaches:
- Expand
changedSettingLabelsto track all modified fields so the dialog is accurate - Create a targeted reset that only resets the fields in
changedSettingLabels - Add a generic warning like "This will reset all settings to defaults"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_chat.settings.tsx` around lines 563 - 598, The
confirmation dialog is misleading because restoreDefaults() calls
resetSettings() which resets all settings (DEFAULT_APP_SETTINGS) while the
message only lists changedSettingLabels; update the function to either (A)
compute and include every actually-modified key in changedSettingLabels before
showing the dialog (compare current app settings to DEFAULT_APP_SETTINGS and
expand changedSettingLabels), or (B) replace the dialog text to warn that all
settings will be reset (e.g., "This will reset all settings to defaults"), or
(C) implement a targeted reset that only clears the keys present in
changedSettingLabels instead of calling resetSettings(); locate
restoreDefaults(), changedSettingLabels, resetSettings(), and
DEFAULT_APP_SETTINGS in the file to apply one of these fixes and ensure the
dialog and actual behavior match.
The dynamic options extraction only checked the resolved provider key, losing options stored under a different provider key (e.g., codex options on a claudeAgent model). Extend COALESCE to try all provider keys as fallback, matching upstream's cascading behavior.
- Extend custom model reset/count to all 8 providers in settings page
- Guard selectedModelSelection against null/undefined selectedModel
- Add cross-provider warning in CodexAdapter startSession/sendTurn
- Skip NULL model rows in migration to avoid model_selection with null model
- Clear persisted draft storage in TraitsPicker Claude test afterEach
- Fix typo in TraitsPicker test name ("th inking" → "thinking")
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/components/ChatView.tsx (1)
3002-3026:⚠️ Potential issue | 🟠 MajorInconsistent handling:
modelSelectioncan beundefinedhere.Unlike the guarded pattern used elsewhere (e.g., Line 2730, 2993, 3132), this dispatch passes
modelSelectionunconditionally. SinceselectedModelSelectioncan beundefined, this may send an invalid payload.🐛 Proposed fix: Use the same guarded pattern
await api.orchestration.dispatchCommand({ type: "thread.turn.start", commandId: newCommandId(), threadId: threadIdForSend, message: { messageId: messageIdForSend, role: "user", text: outgoingMessageText, attachments: [], }, - modelSelection: selectedModelSelection, + ...(selectedModelSelection ? { modelSelection: selectedModelSelection } : {}), ...(providerOptionsForDispatch ? { providerOptions: providerOptionsForDispatch } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 3002 - 3026, The dispatchCommand call passes modelSelection unconditionally, but selectedModelSelection may be undefined; update the payload to include modelSelection only when selectedModelSelection is defined (same guarded/spread pattern used elsewhere) so replace the unconditional modelSelection: selectedModelSelection with a conditional spread like ...(selectedModelSelection ? { modelSelection: selectedModelSelection } : {}); target the api.orchestration.dispatchCommand call and the selectedModelSelection symbol.
🤖 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/components/ChatView.tsx`:
- Around line 3002-3026: The dispatchCommand call passes modelSelection
unconditionally, but selectedModelSelection may be undefined; update the payload
to include modelSelection only when selectedModelSelection is defined (same
guarded/spread pattern used elsewhere) so replace the unconditional
modelSelection: selectedModelSelection with a conditional spread like
...(selectedModelSelection ? { modelSelection: selectedModelSelection } : {});
target the api.orchestration.dispatchCommand call and the selectedModelSelection
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f677e5-c1a6-48ff-b986-236661dc0edc
📒 Files selected for processing (5)
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/web/src/components/ChatView.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/routes/_chat.settings.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/components/chat/TraitsPicker.browser.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts
This was referenced Mar 26, 2026
This was referenced Apr 4, 2026
Merged
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
6 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
pingdotgg/t3codeincluding provider-aware model selections, settings UI refactor (SettingsSection/SettingsRow), sidebar sorting, git hook progress streaming, context window meter, and terminal improvementsModelSelectiondiscriminated union pattern, extended to all 8 providers (codex, claudeAgent, copilot, cursor, opencode, geminiCli, amp, kilo)provider/model/modelOptionsfields throughout the stack with unifiedmodelSelection: ModelSelectionMODEL_CAPABILITIES_INDEXandPROVIDER_DISPLAY_NAMESfor all providersProviderCommandReactorto useEqual.equals()for model change detectionModelSelectionTest plan
bun typecheckpasses across all 7 packagesSummary by CodeRabbit
New Features
Improvements