Persist provider-aware model selections#1371
Open
juliusmarminge wants to merge 25 commits intomainfrom
Open
Conversation
- Replace model-only fields with provider/model selections across orchestration - Add projection schema and migration updates for provider-backed snapshots - Update server and web tests to use the new selection shape
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Model picker clears reasoning effort and fast mode
- Preserved existing composerDraft.modelSelection.options in onProviderModelSelect when the provider remains the same, preventing reasoning effort and fast mode settings from being silently cleared on model switch.
Or push these changes by commenting:
@cursor push 569dd7bcab
Preview (569dd7bcab)
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -3135,10 +3135,14 @@
return;
}
const resolvedModel = resolveAppModelSelection(provider, customModelsByProvider, model);
+ const existingOptions = composerDraft.modelSelection?.options;
+ const preserveOptions =
+ existingOptions !== undefined && composerDraft.modelSelection?.provider === provider;
const nextModelSelection: ModelSelection = {
provider,
model: resolvedModel,
- };
+ ...(preserveOptions ? { options: existingOptions } : {}),
+ } as ModelSelection;
setComposerDraftModelSelection(activeThread.id, nextModelSelection);
setStickyComposerModelSelection(nextModelSelection);
scheduleComposerFocus();
@@ -3150,6 +3154,7 @@
setComposerDraftModelSelection,
setStickyComposerModelSelection,
customModelsByProvider,
+ composerDraft.modelSelection,
],
);
const setPromptFromTraits = useCallback(![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
Outdated
Show resolved
Hide resolved
- Keep draft model options when switching models within the same provider - Decode SQL errors correctly in projection snapshot model selection - Default missing sticky provider to codex during draft migration
- Preserve default provider model selections in the draft state - Update composer draft store expectations for sticky/provider-specific models
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Runtime-mode-set handler may trigger spurious session restarts
- Removed the fallback
?? toProviderModelOptions(thread.modelSelection)so that when no cached model options exist,cachedModelOptionsremainsundefinedand themodelOptionskey is omitted from the options passed toensureSessionForThread, preventing the spurious restart.
- Removed the fallback
- ✅ Fixed: Unreachable fallback chain in plan implementation model selection
- Simplified
nextThreadModelSelectionto assignselectedModelSelectiondirectly, removing the unreachable??fallback chain sinceselectedModelSelectionis always a non-nullModelSelectionfromuseMemo.
- Simplified
Or push these changes by commenting:
@cursor push 14070d6c45
Preview (14070d6c45)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -705,9 +705,7 @@
return;
}
const cachedProviderOptions = threadProviderOptions.get(event.payload.threadId);
- const cachedModelOptions =
- threadModelOptions.get(event.payload.threadId) ??
- toProviderModelOptions(thread.modelSelection);
+ const cachedModelOptions = threadModelOptions.get(event.payload.threadId);
yield* ensureSessionForThread(event.payload.threadId, event.occurredAt, {
...(cachedProviderOptions !== undefined
? { providerOptions: cachedProviderOptions }
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -3035,12 +3035,7 @@
text: implementationPrompt,
});
const nextThreadTitle = truncateTitle(buildPlanImplementationThreadTitle(planMarkdown));
- const nextThreadModelSelection: ModelSelection = selectedModelSelection ??
- activeThread.modelSelection ??
- activeProject.defaultModelSelection ?? {
- provider: "codex",
- model: DEFAULT_MODEL_BY_PROVIDER.codex,
- };
+ const nextThreadModelSelection: ModelSelection = selectedModelSelection;
sendInFlightRef.current = true;
beginSendPhase("sending-turn");
Contributor
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Or push these changes by commenting: Preview (e0f5c02e35)diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -16,6 +16,7 @@
} from "@t3tools/contracts";
import { Cache, Cause, Duration, Effect, Layer, Option, Schema, Stream } from "effect";
import { makeDrainableWorker } from "@t3tools/shared/DrainableWorker";
+import { toProviderModelOptions } from "@t3tools/shared/model";
import { resolveThreadWorkspaceCwd } from "../../checkpointing/Utils.ts";
import { GitCore } from "../../git/Services/GitCore.ts";
@@ -81,17 +82,6 @@
right: ProviderModelOptions | undefined,
): boolean => JSON.stringify(left ?? null) === JSON.stringify(right ?? null);
-function toProviderModelOptions(
- modelSelection: ModelSelection | undefined,
-): ProviderModelOptions | undefined {
- if (!modelSelection?.options) {
- return undefined;
- }
- return modelSelection.provider === "codex"
- ? { codex: modelSelection.options }
- : { claudeAgent: modelSelection.options };
-}
-
function isUnknownPendingApprovalRequestError(cause: Cause.Cause<ProviderServiceError>): boolean {
const error = Cause.squash(cause);
if (Schema.is(ProviderAdapterRequestError)(error)) {
diff --git a/apps/server/src/persistence/Layers/ProjectionProjects.ts b/apps/server/src/persistence/Layers/ProjectionProjects.ts
--- a/apps/server/src/persistence/Layers/ProjectionProjects.ts
+++ b/apps/server/src/persistence/Layers/ProjectionProjects.ts
@@ -77,7 +77,7 @@
${row.workspaceRoot},
${row.defaultModelSelection?.provider ?? null},
${row.defaultModelSelection?.model ?? null},
- ${JSON.stringify(row.defaultModelSelection?.options ?? null)},
+ ${row.defaultModelSelection?.options != null ? JSON.stringify(row.defaultModelSelection.options) : null},
${JSON.stringify(row.scripts)},
${row.createdAt},
${row.updatedAt},
diff --git a/apps/server/src/persistence/Layers/ProjectionThreads.ts b/apps/server/src/persistence/Layers/ProjectionThreads.ts
--- a/apps/server/src/persistence/Layers/ProjectionThreads.ts
+++ b/apps/server/src/persistence/Layers/ProjectionThreads.ts
@@ -89,7 +89,7 @@
${row.title},
${row.modelSelection.provider},
${row.modelSelection.model},
- ${JSON.stringify(row.modelSelection.options ?? null)},
+ ${row.modelSelection.options != null ? JSON.stringify(row.modelSelection.options) : null},
${row.runtimeMode},
${row.interactionMode},
${row.branch},
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -28,6 +28,7 @@
getDefaultModel,
normalizeModelSlug,
resolveModelSlugForProvider,
+ toProviderModelOptions,
} from "@t3tools/shared/model";
import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react";
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
@@ -211,16 +212,6 @@
return provider === "codex" ? modelOptions?.codex : modelOptions?.claudeAgent;
}
-function toProviderModelOptions(
- modelSelection: ModelSelection | null | undefined,
-): ProviderModelOptions | undefined {
- if (!modelSelection?.options) {
- return undefined;
- }
- return modelSelection.provider === "codex"
- ? { codex: modelSelection.options }
- : { claudeAgent: modelSelection.options };
-}
const COMPOSER_PATH_QUERY_DEBOUNCE_MS = 120;
const SCRIPT_TERMINAL_COLS = 120;
const SCRIPT_TERMINAL_ROWS = 30;
diff --git a/packages/shared/src/model.ts b/packages/shared/src/model.ts
--- a/packages/shared/src/model.ts
+++ b/packages/shared/src/model.ts
@@ -10,7 +10,9 @@
type ClaudeCodeEffort,
type CodexModelOptions,
type CodexReasoningEffort,
+ type ModelSelection,
type ModelSlug,
+ type ProviderModelOptions,
type ProviderReasoningEffort,
type ProviderKind,
} from "@t3tools/contracts";
@@ -266,4 +268,15 @@
return `Ultrathink:\n${trimmed}`;
}
+export function toProviderModelOptions(
+ modelSelection: ModelSelection | null | undefined,
+): ProviderModelOptions | undefined {
+ if (!modelSelection?.options) {
+ return undefined;
+ }
+ return modelSelection.provider === "codex"
+ ? { codex: modelSelection.options }
+ : { claudeAgent: modelSelection.options };
+}
+
export { CLAUDE_CODE_EFFORT_OPTIONS, CODEX_REASONING_EFFORT_OPTIONS }; |
- Keep sticky selection in sync when provider options change - Add regression test for creating the initial sticky snapshot
- Store missing model options as SQL NULL in project and thread projections - Stop rehydrating derived model options when restarting provider sessions - Share model option conversion logic in `packages/shared`
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Redundant double call to
extractModelSelectionOptions- Stored the result of extractModelSelectionOptions in a local variable and reused it for both the truthiness check and the value assignment.
- ✅ Fixed: Model options always injected on session restart fallback
- Changed the fallback to only derive model options from requestedModelSelection (explicit user request) rather than from the thread's persisted modelSelection, preventing silent injection during runtime-mode-triggered restarts.
Or push these changes by commenting:
@cursor push 73c8d110db
Preview (73c8d110db)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -249,7 +249,10 @@
const desiredModelSelection = requestedModelSelection ?? thread.modelSelection;
const desiredModel = desiredModelSelection.model;
const desiredModelOptions =
- options?.modelOptions ?? toProviderModelOptions(desiredModelSelection);
+ options?.modelOptions ??
+ (requestedModelSelection !== undefined
+ ? toProviderModelOptions(requestedModelSelection)
+ : undefined);
const effectiveCwd = resolveThreadWorkspaceCwd({
thread,
projects: readModel.projects,
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -643,21 +643,14 @@
);
const selectedPromptEffort = composerProviderState.promptEffort;
const selectedModelOptionsForDispatch = composerProviderState.modelOptionsForDispatch;
- const selectedModelSelection = useMemo<ModelSelection>(
- () => ({
+ const selectedModelSelection = useMemo<ModelSelection>(() => {
+ const options = extractModelSelectionOptions(selectedProvider, selectedModelOptionsForDispatch);
+ return {
provider: selectedProvider,
model: selectedModel,
- ...(extractModelSelectionOptions(selectedProvider, selectedModelOptionsForDispatch)
- ? {
- options: extractModelSelectionOptions(
- selectedProvider,
- selectedModelOptionsForDispatch,
- ),
- }
- : {}),
- }),
- [selectedModel, selectedModelOptionsForDispatch, selectedProvider],
- );
+ ...(options ? { options } : {}),
+ };
+ }, [selectedModel, selectedModelOptionsForDispatch, selectedProvider]);
const providerOptionsForDispatch = useMemo(() => getProviderStartOptions(settings), [settings]);
const selectedModelForPicker = selectedModel;
const modelOptionsByProvider = useMemo(
- Add migration for legacy provider/model payloads - Cover project, thread, and orchestration event records
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dead
providerparameter instartProviderSessionclosure- Removed the unused
readonly provider?: ProviderKindfield from the startProviderSession closure's input type, since the body only uses the outerpreferredProvidervariable.
- Removed the unused
Or push these changes by commenting:
@cursor push e7ae3f40f4
Preview (e7ae3f40f4)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -260,10 +260,7 @@
.listSessions()
.pipe(Effect.map((sessions) => sessions.find((session) => session.threadId === threadId)));
- const startProviderSession = (input?: {
- readonly resumeCursor?: unknown;
- readonly provider?: ProviderKind;
- }) =>
+ const startProviderSession = (input?: { readonly resumeCursor?: unknown }) =>
providerService.startSession(threadId, {
threadId,
...(preferredProvider ? { provider: preferredProvider } : {}),- Replace provider-specific model options with typed modelSelection - Persist and replay selected provider, model, and options end to end - Update adapter, service, and chat composer tests for the new shape
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Broader comparison causes unnecessary claudeAgent session restarts
- Changed sameModelSelectionOptions to compare only left?.options and right?.options instead of the full ModelSelection, so model-name-only changes no longer trigger unnecessary session restarts for claudeAgent.
Or push these changes by commenting:
@cursor push de46c2f9bd
Preview (de46c2f9bd)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -78,7 +78,7 @@
const sameModelSelectionOptions = (
left: ModelSelection | undefined,
right: ModelSelection | undefined,
-): boolean => JSON.stringify(left ?? null) === JSON.stringify(right ?? null);
+): boolean => JSON.stringify(left?.options ?? null) === JSON.stringify(right?.options ?? null);
function isUnknownPendingApprovalRequestError(cause: Cause.Cause<ProviderServiceError>): boolean {
const error = Cause.squash(cause);
Contributor
There was a problem hiding this comment.
🟡 Medium
asClaudeModelOptions returns undefined when modelOptions only contains fastMode (no effort or thinking), causing fast mode settings to be silently dropped. Similarly, asCodexModelOptions misses fastMode when only reasoningEffort is checked. Consider adding fastMode to the property checks in both helpers.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/chat/composerProviderRegistry.tsx around line 52:
`asClaudeModelOptions` returns `undefined` when `modelOptions` only contains `fastMode` (no `effort` or `thinking`), causing fast mode settings to be silently dropped. Similarly, `asCodexModelOptions` misses `fastMode` when only `reasoningEffort` is checked. Consider adding `fastMode` to the property checks in both helpers.
Evidence trail:
- `apps/web/src/components/chat/composerProviderRegistry.tsx` lines 52-62: `asCodexModelOptions` checks only `"reasoningEffort"`, `asClaudeModelOptions` checks only `"effort" || "thinking"`
- `packages/contracts/src/model.ts` lines 10-21: Both `CodexModelOptions` and `ClaudeModelOptions` include optional `fastMode` field
- `packages/shared/src/model.ts` lines 211-224: `normalizeCodexModelOptions` can produce objects with only `fastMode` when `reasoningEffort` equals default
- `packages/shared/src/model.ts` lines 226-249: `normalizeClaudeModelOptions` similarly can produce objects with only `fastMode`
- Switch project and thread fixtures to provider-scoped model selection - Keep sidebar logic tests aligned with the new Codex provider model shape
Contributor
There was a problem hiding this comment.
🟢 Low
makeProject spreads overrides after setting defaultModelSelection, so passing defaultModelSelection: { provider: "anthropic" } replaces the entire object and drops the default model. The nested spread on line 353 is never effective because ...overrides overwrites it. Reorder so the specific property spread comes after ...overrides.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/Sidebar.logic.test.ts around line 345:
`makeProject` spreads `overrides` after setting `defaultModelSelection`, so passing `defaultModelSelection: { provider: "anthropic" }` replaces the entire object and drops the default `model`. The nested spread on line 353 is never effective because `...overrides` overwrites it. Reorder so the specific property spread comes after `...overrides`.
Evidence trail:
apps/web/src/components/Sidebar.logic.test.ts lines 345-360 at REVIEWED_COMMIT. The `makeProject` function shows: `defaultModelSelection` object with nested spread at lines 350-354, then `...overrides` at line 359. JavaScript spread semantics confirm that `...overrides` at line 359 will completely replace the `defaultModelSelection` key if `overrides.defaultModelSelection` exists, negating the nested merge at line 353.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: sendTurn overwrites runtimePayload losing persisted model selection
- Fixed by reading the existing binding's runtimePayload and spreading it as the base object before applying sendTurn-specific fields, so previously persisted fields like cwd, model, and providerOptions are preserved.
Or push these changes by commenting:
@cursor push 2c97a21b07
Preview (2c97a21b07)
diff --git a/apps/server/src/provider/Layers/ProviderService.ts b/apps/server/src/provider/Layers/ProviderService.ts
--- a/apps/server/src/provider/Layers/ProviderService.ts
+++ b/apps/server/src/provider/Layers/ProviderService.ts
@@ -368,12 +368,20 @@
allowRecovery: true,
});
const turn = yield* routed.adapter.sendTurn(input);
+ const existingBinding = Option.getOrUndefined(yield* directory.getBinding(input.threadId));
+ const existingPayload =
+ existingBinding?.runtimePayload &&
+ typeof existingBinding.runtimePayload === "object" &&
+ !Array.isArray(existingBinding.runtimePayload)
+ ? existingBinding.runtimePayload
+ : {};
yield* directory.upsert({
threadId: input.threadId,
provider: routed.adapter.provider,
status: "running",
...(turn.resumeCursor !== undefined ? { resumeCursor: turn.resumeCursor } : {}),
runtimePayload: {
+ ...existingPayload,
...(input.modelSelection !== undefined ? { modelSelection: input.modelSelection } : {}),
activeTurnId: turn.turnId,
lastRuntimeEvent: "provider.sendTurn",| status: "running", | ||
| ...(turn.resumeCursor !== undefined ? { resumeCursor: turn.resumeCursor } : {}), | ||
| runtimePayload: { | ||
| ...(input.modelSelection !== undefined ? { modelSelection: input.modelSelection } : {}), |
Contributor
There was a problem hiding this comment.
sendTurn overwrites runtimePayload losing persisted model selection
Medium Severity
When sendTurn persists the runtime payload via directory.upsert, it constructs a new runtimePayload object that only includes modelSelection, activeTurnId, lastRuntimeEvent, and lastRuntimeEventAt. This completely overwrites any previously persisted payload fields like cwd, model, providerOptions, etc. that were stored by startSession via toRuntimePayloadFromSession. On a subsequent session recovery (e.g., after app restart), readPersistedCwd will return undefined because the cwd field was lost, potentially causing the session to restart without the correct working directory.
Comment on lines
+91
to
+93
| export const runMigrations = ({ toMigrationInclusive }: RunMigrationsOptions = {}) => | ||
| Effect.gen(function* () { | ||
| yield* Effect.log(`Running migrations 1 through ${toMigrationInclusive}...`); |
Contributor
There was a problem hiding this comment.
🟢 Low persistence/Migrations.ts:91
When runMigrations() is called without arguments, the log message outputs "Running migrations 1 through undefined..." because toMigrationInclusive defaults to undefined. This incorrectly implies only migration 1 runs, when actually all migrations execute. Consider using a conditional message that shows the actual range or "all migrations" when no limit is specified.
- yield* Effect.log(`Running migrations 1 through ${toMigrationInclusive}...`);
+ yield* Effect.log(
+ toMigrationInclusive === undefined
+ ? "Running all migrations..."
+ : `Running migrations 1 through ${toMigrationInclusive}...`
+ );🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/persistence/Migrations.ts around lines 91-93:
When `runMigrations()` is called without arguments, the log message outputs `"Running migrations 1 through undefined..."` because `toMigrationInclusive` defaults to `undefined`. This incorrectly implies only migration 1 runs, when actually all migrations execute. Consider using a conditional message that shows the actual range or "all migrations" when no limit is specified.
Evidence trail:
apps/server/src/persistence/Migrations.ts lines 91-94 (log message with `toMigrationInclusive` directly interpolated), lines 62-66 (makeMigrationLoader showing that `undefined` throughId means all migrations run), lines 44-60 (showing 16 migrations total) at REVIEWED_COMMIT.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Log message always shows "undefined" in production
- Conditionally formatted the log message to show the migration range only when toMigrationInclusive is defined, otherwise logging a generic "Running migrations..." message.
Or push these changes by commenting:
@cursor push 919971ae24
Preview (919971ae24)
diff --git a/apps/server/src/persistence/Migrations.ts b/apps/server/src/persistence/Migrations.ts
--- a/apps/server/src/persistence/Migrations.ts
+++ b/apps/server/src/persistence/Migrations.ts
@@ -90,7 +90,11 @@
*/
export const runMigrations = ({ toMigrationInclusive }: RunMigrationsOptions = {}) =>
Effect.gen(function* () {
- yield* Effect.log(`Running migrations 1 through ${toMigrationInclusive}...`);
+ yield* Effect.log(
+ toMigrationInclusive !== undefined
+ ? `Running migrations 1 through ${toMigrationInclusive}...`
+ : "Running migrations...",
+ );
const executedMigrations = yield* run({ loader: makeMigrationLoader(toMigrationInclusive) });
yield* Effect.log("Migrations ran successfully");
return executedMigrations;| }); | ||
| export const runMigrations = ({ toMigrationInclusive }: RunMigrationsOptions = {}) => | ||
| Effect.gen(function* () { | ||
| yield* Effect.log(`Running migrations 1 through ${toMigrationInclusive}...`); |
Contributor
There was a problem hiding this comment.
Log message always shows "undefined" in production
Low Severity
The log message interpolates toMigrationInclusive which is undefined in the default production call path (runMigrations() with no arguments). Every production startup will log "Running migrations 1 through undefined..." instead of something meaningful like "Running all migrations...".
Additional Locations (1)
| ADD COLUMN model_selection_json TEXT | ||
| `; | ||
|
|
||
| yield* sql` |
Contributor
There was a problem hiding this comment.
🟡 Medium Migrations/016_CanonicalizeModelSelections.ts:34
The projection_threads UPDATE (lines 34-54) creates {"provider": "codex", "model": null} when model is NULL, instead of leaving model_selection_json as NULL. This is inconsistent with the projection_projects migration (lines 14-26) which preserves NULL when default_model is NULL, and with orchestration_events updates (lines 182-184) which filter out NULL models. Consider adding AND model IS NOT NULL to the WHERE clause, or using a CASE expression to set NULL when model is NULL.
- UPDATE projection_threads
- SET model_selection_json = json_object(
- 'provider',
- COALESCE(
- (
- SELECT provider_name
- FROM projection_thread_sessions
- WHERE projection_thread_sessions.thread_id = projection_threads.thread_id
- ),
- CASE
- WHEN lower(model) LIKE '%claude%' THEN 'claudeAgent'
- ELSE 'codex'
- END,
- 'codex'
- ),
- 'model',
- model
- )
- WHERE model_selection_json IS NULL🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts around line 34:
The `projection_threads` UPDATE (lines 34-54) creates `{"provider": "codex", "model": null}` when `model` is NULL, instead of leaving `model_selection_json` as NULL. This is inconsistent with the `projection_projects` migration (lines 14-26) which preserves NULL when `default_model` is NULL, and with `orchestration_events` updates (lines 182-184) which filter out NULL models. Consider adding `AND model IS NOT NULL` to the WHERE clause, or using a CASE expression to set NULL when `model` is NULL.
Evidence trail:
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.ts at REVIEWED_COMMIT:
- Lines 14-26: `projection_projects` UPDATE uses `CASE WHEN default_model IS NULL THEN NULL ELSE json_object(...) END`
- Lines 34-54: `projection_threads` UPDATE uses `json_object('provider', COALESCE(...), 'model', model)` with no NULL guard
- Lines 128-138: First `orchestration_events` UPDATE has `AND json_type(payload_json, '$.defaultModel') IS NOT NULL`
- Lines 182-185: Second `orchestration_events` UPDATE has `AND json_type(payload_json, '$.model') IS NOT NULL`
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Equal.equals on plain objects uses reference equality
- Replaced Equal.equals (which falls back to === reference equality for plain objects) with JSON.stringify-based deep value comparison, restoring the correct behavior that avoids unnecessary session restarts when ModelSelection values are identical.
Or push these changes by commenting:
@cursor push 7d1604932b
Preview (7d1604932b)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -13,7 +13,7 @@
type RuntimeMode,
type TurnId,
} from "@t3tools/contracts";
-import { Cache, Cause, Duration, Effect, Equal, Layer, Option, Schema, Stream } from "effect";
+import { Cache, Cause, Duration, Effect, Layer, Option, Schema, Stream } from "effect";
import { makeDrainableWorker } from "@t3tools/shared/DrainableWorker";
import { resolveThreadWorkspaceCwd } from "../../checkpointing/Utils.ts";
@@ -301,7 +301,7 @@
const shouldRestartForModelSelectionChange =
currentProvider === "claudeAgent" &&
requestedModelSelection !== undefined &&
- !Equal.equals(previousModelSelection, requestedModelSelection);
+ JSON.stringify(previousModelSelection) !== JSON.stringify(requestedModelSelection);
if (
!runtimeModeChanged &&- Simplify orchestration contracts to canonical model-selection shapes - Add a Cursor ACP probe for listing available models
- keep the active Codex model across turns when in-session switching is unavailable - adjust reactor coverage for unsupported session model switching
- Store Codex and Claude composer traits separately - Keep sticky and draft model options in sync across provider switches - Update registry and picker tests for provider-scoped options
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
modelfields with provider-awaremodelSelectionrecords across projects, threads, and orchestration flows.Testing
bun fmtbun lintbun typecheckbun run testNote
High Risk
High risk because it changes persisted schema and event payload shapes (new JSON columns, dropped legacy columns, rewritten historical events) and updates provider session/turn routing logic, so a missed migration or contract mismatch could break reads, projections, or provider execution.
Overview
Persists provider-aware model configuration end-to-end by replacing standalone
model/provider/modelOptionsfields with a unifiedModelSelection(modelSelection/defaultModelSelection) across orchestration commands/events, projections, provider service/adapters, and websocket snapshot responses.Updates persistence and projections to store/read model selections as JSON (
default_model_selection_json,model_selection_json), including a new016_CanonicalizeModelSelectionsmigration that backfills these columns, rewrites legacyorchestration_events.payload_jsoninto the new shape, and drops the olddefault_model/modelcolumns; adds repository tests to verify NULL/JSON handling.Refines provider session behavior in
ProviderCommandReactorto restart/reuse sessions based onModelSelectionequality and provider capabilities, and to preserve the active session model when in-session model switching is unsupported.Written by Cursor Bugbot for commit 4fe00a5. This will update automatically on new commits. Configure here.
Note
Replace separate provider/model/options fields with a unified
ModelSelectionobject across all layersModelSelectioncontract type (a union ofCodexModelSelectionandClaudeModelSelection) that bundles provider, model slug, and provider-specific options into a single object.provider,model, andmodelOptionsfields in orchestration commands, event payloads, provider adapter inputs, projection schemas, and composer draft store state withmodelSelection/defaultModelSelection.model_selection_json/default_model_selection_jsoncolumns from legacy data, infers missing providers, rewrites historical event payload JSON, and drops the olddefault_model/modelcolumns.setStickyModel,setStickyModelOptions,setProvider,setModel, andsetModelOptionsintosetModelSelectionandsetStickyModelSelection; options are preserved when only the model changes within the same provider and dropped when the provider changes.Macroscope summarized 0a81215.