fix(chat): write the resolved model to chats.model_id at provision time - #830
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change centralizes the chat model default as ChangesChat model default and validation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🔵 Low · up to The change correctly persists the resolved model for new chats, but chats with an empty or whitespace-only stored model ID could still start with an invalid model value instead of the default. This is a bounded follow-up risk and does not otherwise prevent merging. Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant validateChatRunRequest
participant handleStartChatRun
participant provisionRunSession
participant createSessionWithInitialChat
participant ChatDatabase
Client->>validateChatRunRequest: submit model value
validateChatRunRequest->>handleStartChatRun: return trimmed modelId or default
handleStartChatRun->>provisionRunSession: pass modelId
provisionRunSession->>createSessionWithInitialChat: pass modelId
createSessionWithInitialChat->>ChatDatabase: insert chat with model_id
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| * `chats.model_id` at provision time (chat#1956). Every chat writer persists | ||
| * a model explicitly; nothing relies on a database column default. | ||
| */ | ||
| export const DEFAULT_CHAT_MODEL_ID = "anthropic/claude-haiku-4.5"; |
There was a problem hiding this comment.
YAGNI / DRY - Why is this needed when we already have DEFAULT_MODEL?
There was a problem hiding this comment.
Kept, but the comment now answers this inline: DEFAULT_MODEL (gpt-5.4-nano) powers internal utility LLM calls — evals, catalog batch analysis, general agent — while DEFAULT_CHAT_MODEL_ID is the user-facing chat/run default written to chats.model_id. Different domains, different values, both real. Also updated to kimi-k3 per the other threads. Fixed in 9bb063b.
There was a problem hiding this comment.
Resolved per your call in the follow-up discussion: DEFAULT_CHAT_MODEL_ID is removed entirely. DEFAULT_MODEL is now the single constant, value moonshotai/kimi-k3, used by every consumer (chat writers, UI defaultModelId via getSessionChatsHandler, generalAgent fallback, evals, catalog batch). Your one-constant instinct also surfaced a pre-existing three-way disagreement — the UI's defaultModelId was nano while the run default and column default were haiku — which this collapse eliminates. Full lib suite 4,383 tests green. Commit: latest on branch.
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 3/5
- In
lib/sessions/createSessionWithInitialChat.ts, allowingmodel: ""to persist as an emptymodel_idcan break model provenance and may cause the wrong model behavior downstream when the run is provisioned—normalize empty model IDs to a valid default or reject them before insert/provisioning. - In
lib/sessions/createSessionWithInitialChat.ts, the new invariant depends on everyinsertChatcaller settingmodel_id, butscripts/backfill/migrat...is called out as another path; once the DB default is removed, that path could fail or write inconsistent rows—update that caller (or guard the insert contract) before the follow-up schema change.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sessions/createSessionWithInitialChat.ts">
<violation number="1" location="lib/sessions/createSessionWithInitialChat.ts:60">
P3: This PR centralizes the invariant that "every chat writer persists a model explicitly" and the follow-up DB PR drops the `chats.model_id` column default. There is another `insertChat` caller — `scripts/backfill/migrateRoom.ts` — that constructs a chat row without `model_id`. If that idempotent backfill is re-run after the column default is dropped, it will insert chats with a NULL `model_id`, which breaks the same provenance invariant this change is meant to guarantee. Consider setting `model_id` explicitly in `migrateRoom.ts` (or confirming the DB migration backfills existing and skips new NULL inserts) before dropping the default.</violation>
<violation number="2" location="lib/sessions/createSessionWithInitialChat.ts:60">
P2: When a run request supplies `model: ""`, this line persists an empty `model_id` instead of a usable default. Reject or normalize empty model IDs before provisioning so the persisted provenance and the model sent to the workflow are valid.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client
participant API as API Routes
participant Handler as Session/Chat Handlers
participant Run as Run Services
participant DB as Database
participant Const as lib/const.ts
Note over Client,DB: Chat Session Creation Flows
Client->>API: POST /api/chat/sessions
API->>Handler: createSessionHandler()
Handler->>Const: DEFAULT_CHAT_MODEL_ID
Handler->>DB: insertSession() + insertChat() with model_id = default
DB-->>Handler: Session + Chat row
Handler-->>API: Session created
API-->>Client: 200 + session
Client->>API: POST /api/chat/sessions/:id/chats
API->>Handler: createSessionChatHandler()
Handler->>Const: DEFAULT_CHAT_MODEL_ID
Handler->>DB: insertChat() with model_id = default
DB-->>Handler: Chat row
Handler-->>API: Chat created
API-->>Client: 200 + chat
Note over Client,DB: Headless Run Flow (POST /api/chat/runs)
Client->>API: POST /api/chat/runs with model: X
API->>Run: handleStartChatRun()
Run->>Run: validateChatRunRequest() - validates model (default = DEFAULT_RUN_MODEL_ID)
Run->>Run: provisionRunSession() with modelId
Run->>Run: createSessionWithInitialChat() with modelId
Run->>DB: insertSession() + insertChat() with model_id = X
DB-->>Run: Session + Chat row (model_id = X)
Run-->>API: 202 Accepted
API-->>Client: 202 + run started
Note over Run,DB: Model Provenance Guarantee
Run->>DB: usage_events records actual billed model
Run->>DB: chats.model_id records resolved model
DB-->>Run: Both written atomically
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| id: generateUUID(), | ||
| session_id: session.id, | ||
| title: chatTitle, | ||
| model_id: modelId, |
There was a problem hiding this comment.
P2: When a run request supplies model: "", this line persists an empty model_id instead of a usable default. Reject or normalize empty model IDs before provisioning so the persisted provenance and the model sent to the workflow are valid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/createSessionWithInitialChat.ts, line 60:
<comment>When a run request supplies `model: ""`, this line persists an empty `model_id` instead of a usable default. Reject or normalize empty model IDs before provisioning so the persisted provenance and the model sent to the workflow are valid.</comment>
<file context>
@@ -50,7 +53,12 @@ export async function createSessionWithInitialChat({
+ id: generateUUID(),
+ session_id: session.id,
+ title: chatTitle,
+ model_id: modelId,
+ });
if (!chat) {
</file context>
| id: generateUUID(), | ||
| session_id: session.id, | ||
| title: chatTitle, | ||
| model_id: modelId, |
There was a problem hiding this comment.
P3: This PR centralizes the invariant that "every chat writer persists a model explicitly" and the follow-up DB PR drops the chats.model_id column default. There is another insertChat caller — scripts/backfill/migrateRoom.ts — that constructs a chat row without model_id. If that idempotent backfill is re-run after the column default is dropped, it will insert chats with a NULL model_id, which breaks the same provenance invariant this change is meant to guarantee. Consider setting model_id explicitly in migrateRoom.ts (or confirming the DB migration backfills existing and skips new NULL inserts) before dropping the default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/createSessionWithInitialChat.ts, line 60:
<comment>This PR centralizes the invariant that "every chat writer persists a model explicitly" and the follow-up DB PR drops the `chats.model_id` column default. There is another `insertChat` caller — `scripts/backfill/migrateRoom.ts` — that constructs a chat row without `model_id`. If that idempotent backfill is re-run after the column default is dropped, it will insert chats with a NULL `model_id`, which breaks the same provenance invariant this change is meant to guarantee. Consider setting `model_id` explicitly in `migrateRoom.ts` (or confirming the DB migration backfills existing and skips new NULL inserts) before dropping the default.</comment>
<file context>
@@ -50,7 +53,12 @@ export async function createSessionWithInitialChat({
+ id: generateUUID(),
+ session_id: session.id,
+ title: chatTitle,
+ model_id: modelId,
+ });
if (!chat) {
</file context>
There was a problem hiding this comment.
Valid concern, deliberate non-fix: migrateRoom.ts backfills legacy rooms whose actual model is unknown. After recoupable/database#56, an omitted model_id inserts NULL — which for a legacy room is the honest value ("not recorded"), exactly the semantics the migration's column comment defines. Writing DEFAULT_CHAT_MODEL_ID there would fabricate provenance for historical rows, the precise failure chat#1956 exists to end. The invariant is better stated as: every writer of NEW chats records the model that will run; the backfill records what it knows, which is nothing.
Preview verification — all Done-when criteria passPreview:
Case 1 is the discriminator: this morning's reproduction (recoupable/chat#1956) ran four different explicit models and got four Contract check: request/response bodies unchanged (202 Note: cases 1–2 started real (tiny-prompt) workflow runs on the test account; they complete on their own and cost a few credits. 🤖 Generated with Claude Code |
Headless runs (POST /api/chat/runs) resolved the model correctly and threaded it into the workflow, but the chat insert dropped it one call before the row was written — the chats.model_id column default then fabricated 'anthropic/claude-haiku-4.5' for every headless run (recoupable/chat#1956). - createSessionWithInitialChat now requires modelId and writes it on the chat insert; provisionRunSession threads it; handleStartChatRun passes the validated modelId. - Interactive writers (createSessionHandler, createSessionChatHandler) set the default explicitly, so no writer relies on the column default and the database default can be dropped (recoupable/database follow-up). - Single source for the default: DEFAULT_CHAT_MODEL_ID in lib/const; DEFAULT_RUN_MODEL_ID and handleChatWorkflowStream's local constant now alias it instead of repeating the string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
39db0ea to
24d5188
Compare
| /** Default model for headless generation when the caller omits `model`. */ | ||
| export const DEFAULT_RUN_MODEL_ID = "anthropic/claude-haiku-4.5"; | ||
| /** Default model for headless generation when the caller omits `model` (alias of the shared default). */ | ||
| export const DEFAULT_RUN_MODEL_ID = DEFAULT_CHAT_MODEL_ID; |
There was a problem hiding this comment.
DRY
- actual: wrapper DEFAULT_RUN_MODEL_ID alias
- required: delete DEFAULT_RUN_MODEL_ID and replace with DEFAULT_CHAT_MODEL_ID.
- make sure DEFAULT_CHAT_MODEL_ID is kimi k3.
There was a problem hiding this comment.
Done in 9bb063b: DEFAULT_RUN_MODEL_ID deleted (it had no importers outside this file), validateChatRunRequest uses DEFAULT_CHAT_MODEL_ID directly, and the value is moonshotai/kimi-k3. Bonus from the same pass: model: "" or whitespace now normalizes to the default (|| not ??) instead of persisting an empty model_id — cubic's P2.
| import generateUUID from "@/lib/uuid/generateUUID"; | ||
|
|
||
| const DEFAULT_MODEL_ID = "anthropic/claude-haiku-4.5"; | ||
| const DEFAULT_MODEL_ID = DEFAULT_CHAT_MODEL_ID; |
There was a problem hiding this comment.
DRY
- actual: wrapper DEFAULT_RUN_MODEL_ID alias
- required: delete DEFAULT_RUN_MODEL_ID and replace with DEFAULT_CHAT_MODEL_ID.
- make sure DEFAULT_CHAT_MODEL_ID is kimi k3.
There was a problem hiding this comment.
Done in 9bb063b: the local DEFAULT_MODEL_ID alias is deleted; the null-fallback at the use site reads chat.model_id ?? DEFAULT_CHAT_MODEL_ID, and the constant is moonshotai/kimi-k3. The pinned test ("falls back to the default model when chat.model_id is null") was flipped to kimi-k3 first and confirmed RED before the change.
| @@ -10,6 +10,13 @@ export const USDC_ADDRESS = "0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913" as Addr | |||
| export const PAYMASTER_URL = `https://api.developer.coinbase.com/rpc/v1/base/${process.env.PAYMASTER_KEY}`; | |||
| export const IMAGE_GENERATE_PRICE = "0.15"; | |||
| export const DEFAULT_MODEL = "openai/gpt-5.4-nano"; | |||
There was a problem hiding this comment.
update to use kimi k3
There was a problem hiding this comment.
Done in 9bb063b — moonshotai/kimi-k3, with the A/B rationale in the doc comment. Note this comment sits on DEFAULT_MODEL (gpt-5.4-nano, internal utility calls); I read the intent as the chat default and changed DEFAULT_CHAT_MODEL_ID. If you also want the utility-call model changed, say so — left untouched deliberately.
Re-verified on the current head after rebaseThe earlier verification ran against pre-rebase head
All checks green on this head (format/lint/test/Vercel). Ready to merge; recoupable/database#56 follows after this is live. 🤖 Generated with Claude Code |
…; normalize empty model Review feedback on #830: - DEFAULT_RUN_MODEL_ID and the local DEFAULT_MODEL_ID alias are deleted; every consumer imports DEFAULT_CHAT_MODEL_ID directly (DRY). - DEFAULT_CHAT_MODEL_ID is moonshotai/kimi-k3 — best recall and lowest cost in the 2026-08-12 4-model A/B on a production roster-brief task. Distinct from DEFAULT_MODEL (internal utility LLM calls). - cubic P2: model: "" / whitespace now normalizes to the default instead of persisting "" as provenance and sending "" to the workflow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verification on the review-fix head
|
| Case | POST /api/chat/runs body |
chats.model_id observed |
|
|---|---|---|---|
| Explicit | model: "xai/grok-4.6" |
xai/grok-4.6 |
✅ |
| Omitted | (no model) |
moonshotai/kimi-k3 — the new default |
✅ |
| Empty string | model: "" |
moonshotai/kimi-k3 — normalized, not "" (cubic P2) |
✅ |
CI green on this head. Merge order unchanged: this PR, then recoupable/database#56.
🤖 Generated with Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/chat/runs/provisionRunSession.ts (1)
56-61: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSet
model_idexplicitly in the backfill writer.scripts/backfill/migrateRoom.tsinserts chats withoutmodel_id. Sincechats.model_idis nullable, removing the default creates migrated chats without model provenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/chat/runs/provisionRunSession.ts` around lines 56 - 61, Update the chat insert logic in scripts/backfill/migrateRoom.ts to set model_id explicitly for every migrated chat, using the appropriate model identifier available in the backfill data or established default. Preserve the existing migration behavior while ensuring migrated records retain model provenance.
🧹 Nitpick comments (2)
lib/chat/runs/validateChatRunRequest.ts (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the request validator into focused steps.
validateChatRunRequestspans 47 lines. It parses JSON, validates the schema, checks prompt/message exclusivity, authenticates, and resolves the model. Extract these responsibilities into focused helpers.As per coding guidelines, “Flag functions longer than 20 lines or classes with >200 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/chat/runs/validateChatRunRequest.ts` around lines 79 - 88, Refactor validateChatRunRequest into focused helpers for JSON parsing, schema validation, prompt/message exclusivity checks, authentication, and model resolution. Keep validateChatRunRequest as the orchestration entry point, preserve the existing validation behavior, and retain trimmedModel’s fallback to DEFAULT_CHAT_MODEL_ID for empty or whitespace-only models.Source: Coding guidelines
lib/chat/runs/provisionRunSession.ts (1)
40-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce oversized orchestration functions across the changed flow.
The changed functions combine multiple responsibilities and exceed the repository’s function-length guidance. Extract focused helpers while preserving the explicit model propagation contract.
lib/chat/runs/provisionRunSession.ts#L40-L53: separate repository, session, sandbox, and skill provisioning.lib/chat/handleChatWorkflowStream.ts#L92-L92: separate ownership, stream-slot, sandbox, workflow, and response phases.lib/chat/runs/handleStartChatRun.ts#L45-L50: separate resource, key, workflow, and response handling.lib/chat/runs/validateChatRunRequest.ts#L79-L88: separate parsing, schema validation, authentication, and model resolution.lib/sessions/createSessionWithInitialChat.ts#L38-L46: separate repository, session, chat, and rollback logic.lib/sessions/createSessionHandler.ts#L47-L53: separate title resolution, persistence, error mapping, and response construction.lib/sessions/chats/createSessionChatHandler.ts#L53-L58: separate idempotent lookup, conflict handling, insertion, and response mapping.As per coding guidelines, “Flag functions longer than 20 lines or classes with >200 lines.”
As per path instructions,lib/**/*.tsfunctions must follow single responsibility and stay under 50 lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/chat/runs/provisionRunSession.ts` around lines 40 - 53, Refactor the oversized orchestration functions into focused helpers while preserving behavior and explicit modelId propagation. In lib/chat/runs/provisionRunSession.ts lines 40-53, extract repository, session, sandbox, and skill provisioning; in lib/chat/handleChatWorkflowStream.ts line 92, extract ownership, stream-slot, sandbox, workflow, and response phases; in lib/chat/runs/handleStartChatRun.ts lines 45-50, extract resource, key, workflow, and response handling; in lib/chat/runs/validateChatRunRequest.ts lines 79-88, extract parsing, schema validation, authentication, and model resolution; in lib/sessions/createSessionWithInitialChat.ts lines 38-46, extract repository, session, chat, and rollback logic; in lib/sessions/createSessionHandler.ts lines 47-53, extract title resolution, persistence, error mapping, and response construction; and in lib/sessions/chats/createSessionChatHandler.ts lines 53-58, extract idempotent lookup, conflict handling, insertion, and response mapping. Keep each function single-purpose and within the repository’s length guidance.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/chat/handleChatWorkflowStream.ts`:
- Line 92: Update the modelId assignment near buildRunAgentInput to trim
persisted chat.model_id values and fall back to DEFAULT_CHAT_MODEL_ID when the
result is empty or whitespace-only, while preserving valid IDs. Add regression
coverage for both blank and whitespace-only model IDs.
---
Outside diff comments:
In `@lib/chat/runs/provisionRunSession.ts`:
- Around line 56-61: Update the chat insert logic in
scripts/backfill/migrateRoom.ts to set model_id explicitly for every migrated
chat, using the appropriate model identifier available in the backfill data or
established default. Preserve the existing migration behavior while ensuring
migrated records retain model provenance.
---
Nitpick comments:
In `@lib/chat/runs/provisionRunSession.ts`:
- Around line 40-53: Refactor the oversized orchestration functions into focused
helpers while preserving behavior and explicit modelId propagation. In
lib/chat/runs/provisionRunSession.ts lines 40-53, extract repository, session,
sandbox, and skill provisioning; in lib/chat/handleChatWorkflowStream.ts line
92, extract ownership, stream-slot, sandbox, workflow, and response phases; in
lib/chat/runs/handleStartChatRun.ts lines 45-50, extract resource, key,
workflow, and response handling; in lib/chat/runs/validateChatRunRequest.ts
lines 79-88, extract parsing, schema validation, authentication, and model
resolution; in lib/sessions/createSessionWithInitialChat.ts lines 38-46, extract
repository, session, chat, and rollback logic; in
lib/sessions/createSessionHandler.ts lines 47-53, extract title resolution,
persistence, error mapping, and response construction; and in
lib/sessions/chats/createSessionChatHandler.ts lines 53-58, extract idempotent
lookup, conflict handling, insertion, and response mapping. Keep each function
single-purpose and within the repository’s length guidance.
In `@lib/chat/runs/validateChatRunRequest.ts`:
- Around line 79-88: Refactor validateChatRunRequest into focused helpers for
JSON parsing, schema validation, prompt/message exclusivity checks,
authentication, and model resolution. Keep validateChatRunRequest as the
orchestration entry point, preserve the existing validation behavior, and retain
trimmedModel’s fallback to DEFAULT_CHAT_MODEL_ID for empty or whitespace-only
models.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5cfd9f0-7aef-4b31-b652-d363fb8b60fc
⛔ Files ignored due to path filters (7)
lib/chat/__tests__/handleChatWorkflowStream.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/runs/__tests__/handleStartChatRun.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/runs/__tests__/provisionRunSession.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/runs/__tests__/validateChatRunRequest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sessions/__tests__/createSessionHandler.persistence.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sessions/__tests__/createSessionWithInitialChat.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sessions/chats/__tests__/createSessionChatHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (9)
app/api/chat/runs/route.tslib/chat/handleChatWorkflowStream.tslib/chat/runs/handleStartChatRun.tslib/chat/runs/provisionRunSession.tslib/chat/runs/validateChatRunRequest.tslib/const.tslib/sessions/chats/createSessionChatHandler.tslib/sessions/createSessionHandler.tslib/sessions/createSessionWithInitialChat.ts
| void persistLatestUserMessage(validated.chatId, validated.messages as never); | ||
|
|
||
| const modelId = chat.model_id ?? DEFAULT_MODEL_ID; | ||
| const modelId = chat.model_id ?? DEFAULT_CHAT_MODEL_ID; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target outline ---'
ast-grep outline lib/chat/handleChatWorkflowStream.ts || true
printf '%s\n' '--- target lines ---'
sed -n '1,180p' lib/chat/handleChatWorkflowStream.ts
printf '%s\n' '--- relevant symbols ---'
rg -n --glob '!node_modules' --glob '!dist' 'buildRunAgentInput|model_id|DEFAULT_CHAT_MODEL_ID' .Repository: recoupable/api
Length of output: 15824
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant outlines ---'
for f in \
lib/chat/buildRunAgentInput.ts \
lib/chat/runs/validateChatRunRequest.ts \
lib/sessions/chats/patchSessionChatHandler.ts \
lib/sessions/chats/createSessionChatHandler.ts \
lib/sessions/createSessionWithInitialChat.ts \
lib/supabase/chats/updateChat.ts \
lib/chat/__tests__/handleChatWorkflowStream.test.ts
do
if [ -f "$f" ]; then
ast-grep outline "$f" || true
fi
done
printf '%s\n' '--- builder ---'
sed -n '1,180p' lib/chat/buildRunAgentInput.ts
printf '%s\n' '--- run validation ---'
sed -n '1,150p' lib/chat/runs/validateChatRunRequest.ts
printf '%s\n' '--- patch handler and updater ---'
sed -n '1,120p' lib/sessions/chats/patchSessionChatHandler.ts
sed -n '1,140p' lib/supabase/chats/updateChat.ts
printf '%s\n' '--- chat creation paths ---'
sed -n '1,120p' lib/sessions/chats/createSessionChatHandler.ts
sed -n '1,120p' lib/sessions/createSessionWithInitialChat.ts
printf '%s\n' '--- focused workflow tests ---'
sed -n '240,330p' lib/chat/__tests__/handleChatWorkflowStream.test.ts
printf '%s\n' '--- modelId validation/write references ---'
rg -n --glob '!node_modules' --glob '!dist' 'modelId|model_id|CREATE TABLE.*chats|CHECK.*model' lib supabase migrations db . 2>/dev/null | head -n 300Repository: recoupable/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- patch validation ---'
sed -n '1,100p' lib/sessions/chats/validatePatchSessionChatRequest.ts
printf '%s\n' '--- insertChat/updateChat call sites ---'
rg -n --glob '*.ts' --glob '*.tsx' 'insertChat\(|updateChat\(' lib app/api
printf '%s\n' '--- schema and migration files ---'
fd -t f -i '(migration|schema|sql)$' . | head -n 200
rg -n -i --glob '*.sql' 'model_id|create table.*chats|check' . | head -n 200 || true
printf '%s\n' '--- runtime fallback probe ---'
node - <<'JS'
const values = [null, undefined, "", " ", " openai/gpt-5-mini "];
for (const value of values) {
const nullish = value ?? "moonshotai/kimi-k3";
const normalized = value?.trim() || "moonshotai/kimi-k3";
console.log(JSON.stringify(value), JSON.stringify(nullish), JSON.stringify(normalized));
}
JSRepository: recoupable/api
Length of output: 5082
Normalize blank persisted model IDs before starting the workflow.
?? leaves empty and whitespace-only IDs unchanged. buildRunAgentInput forwards them to the workflow. Use chat.model_id?.trim() || DEFAULT_CHAT_MODEL_ID and add regression coverage for both cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/chat/handleChatWorkflowStream.ts` at line 92, Update the modelId
assignment near buildRunAgentInput to trim persisted chat.model_id values and
fall back to DEFAULT_CHAT_MODEL_ID when the result is empty or whitespace-only,
while preserving valid IDs. Add regression coverage for both blank and
whitespace-only model IDs.
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Confidence score: 5/5
- In
lib/chat/handleChatWorkflowStream.ts, using??for the model fallback can let an emptychat.model_idpass through, creating inconsistent behavior withvalidateChatRunRequestand risking workflow calls with an invalid model ID; align this path to the same empty/whitespace normalization (||or equivalent trim check) to de-risk regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/chat/handleChatWorkflowStream.ts">
<violation number="1" location="lib/chat/handleChatWorkflowStream.ts:92">
P3: This fallback uses `??`, so an empty-string `chat.model_id` is kept and sent to the workflow, while the headless path this PR touches (validateChatRunRequest) deliberately uses `||` to normalize empty/whitespace models to the default. For interactive chats where `chat.model_id` is already an empty string (as opposed to null), the empty id reaches the workflow and is not replaced.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| void persistLatestUserMessage(validated.chatId, validated.messages as never); | ||
|
|
||
| const modelId = chat.model_id ?? DEFAULT_MODEL_ID; | ||
| const modelId = chat.model_id ?? DEFAULT_CHAT_MODEL_ID; |
There was a problem hiding this comment.
P3: This fallback uses ??, so an empty-string chat.model_id is kept and sent to the workflow, while the headless path this PR touches (validateChatRunRequest) deliberately uses || to normalize empty/whitespace models to the default. For interactive chats where chat.model_id is already an empty string (as opposed to null), the empty id reaches the workflow and is not replaced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/handleChatWorkflowStream.ts, line 92:
<comment>This fallback uses `??`, so an empty-string `chat.model_id` is kept and sent to the workflow, while the headless path this PR touches (validateChatRunRequest) deliberately uses `||` to normalize empty/whitespace models to the default. For interactive chats where `chat.model_id` is already an empty string (as opposed to null), the empty id reaches the workflow and is not replaced.</comment>
<file context>
@@ -91,7 +89,7 @@ export async function handleChatWorkflowStream(request: NextRequest): Promise<Re
void persistLatestUserMessage(validated.chatId, validated.messages as never);
- const modelId = chat.model_id ?? DEFAULT_MODEL_ID;
+ const modelId = chat.model_id ?? DEFAULT_CHAT_MODEL_ID;
// Connect the sandbox up-front so we can (a) read the real working
</file context>
Review decision on #830: no separate DEFAULT_CHAT_MODEL_ID. DEFAULT_MODEL (now moonshotai/kimi-k3) is the single default everywhere a caller picks none: interactive chats, headless runs (persisted to chats.model_id), the session-chats endpoint's defaultModelId exposed to the UI, the general agent's fallback (incl. inbound email replies), evals, and catalog batch analysis. This closes a previously unnoticed three-way disagreement: the UI's defaultModelId was gpt-5.4-nano (DEFAULT_MODEL), the chats column default was haiku-4.5, and the run default was haiku-4.5 — three different answers to "what model do I get by default". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 7 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Preview verification — head
|
| # | Case | Call | Expected | Observed (DB-verified) | |
|---|---|---|---|---|---|
| 1 | Explicit model | POST /api/chat/runs model: "xai/grok-4.6" → chat 6ad77729 |
xai/grok-4.6 |
xai/grok-4.6 |
✅ |
| 2 | Omitted model | POST /api/chat/runs, no model → chat 26ab66c4 |
moonshotai/kimi-k3 |
moonshotai/kimi-k3 |
✅ |
| 3 | Empty model | POST /api/chat/runs model: "" → chat 616dfb98 |
normalized to moonshotai/kimi-k3, never "" |
moonshotai/kimi-k3 |
✅ |
| 4 | Interactive create | POST /api/sessions → chat be507dd1 |
moonshotai/kimi-k3 written explicitly |
moonshotai/kimi-k3 |
✅ |
| 5 | UI default surface | GET /api/sessions/{id}/chats |
defaultModelId: "moonshotai/kimi-k3" |
moonshotai/kimi-k3 (was gpt-5.4-nano before this head) |
✅ |
| 6 | Auth | POST /api/chat/runs, no credential |
401 | 401 | ✅ |
Case 5 is the new surface from the one-constant collapse: the model default the web app is told now agrees with what runs and what's persisted — previously three different values (nano / haiku / haiku). Rows verified directly in the shared database immediately after each response.
CI green on this head. Merge order unchanged: this PR → recoupable/database#56.
🤖 Generated with Claude Code
…956) (#56) The default fabricated provenance: writers that omitted model_id got a plausible real model id instead of an honest NULL. After recoupable/api#830 every writer sets the field explicitly, so the default has zero remaining writers. No backfill — pre-fix rows can't be classified retroactively; fence by date, usage_events stays the historical record. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Closes the api row of recoupable/chat#1956 (model provenance for headless/task runs).
What
Every chat writer now records the model explicitly on the chat row:
createSessionWithInitialChattakes a requiredmodelIdand writesmodel_idon the insert (lib/sessions/createSessionWithInitialChat.ts)provisionRunSession→handleStartChatRunthread the validated model from the request, soPOST /api/chat/runswithmodel: Xyields a chat row withmodel_id = Xfrom the moment the 202 returnscreateSessionHandler,createSessionChatHandler) pass the default explicitlyDEFAULT_MODELinlib/const.ts, valuemoonshotai/kimi-k3(review decision).DEFAULT_RUN_MODEL_ID, the localDEFAULT_MODEL_IDalias, and the interimDEFAULT_CHAT_MODEL_IDare all gone. This also fixes a pre-existing three-way disagreement: the UI'sdefaultModelId(session-chats endpoint) wasgpt-5.4-nanowhile the run default and the DB column default werehaiku-4.5Why
Reproduced 2026-08-12 (table in the issue): four
POST /api/chat/runscalls with four explicit models — all four chat rows readanthropic/claude-haiku-4.5(the column default) whileusage_eventsbilled the real models. The interactive path readschats.model_idas its source of truth (chat#1767), so the column being wrong for headless runs made model debugging three-sources-disagree work.Merge sequencing
This PR first, then the
databasePR dropping thechats.model_idcolumn default (hard dependency: dropping the default before every writer is explicit would leave interactive chats with NULL). No docs PR — no response schema documents the resolved model today; checked in the issue.Tests
createSessionWithInitialChat(writes caller's modelId),provisionRunSession(forwards),handleStartChatRun(passes validated model),createSessionHandlerpersistence +createSessionChatHandler(explicit default pinned to the literal"anthropic/claude-haiku-4.5", not the constant, so a broken import can't false-green)lib/sessions+lib/chatsuites: 642 passedtsc --noEmit: 202 pre-existing errors on clean main, 202 with this diff — zero introduced. Lint clean on touched files.Preview verification to follow as a PR comment.
🤖 Generated with Claude Code
Summary by cubic
Writes the resolved chat model to
chats.model_idat provision time and unifies the default toDEFAULT_MODEL = "moonshotai/kimi-k3". Previously headless runs used the column default (anthropic/claude-haiku-4.5) and different paths had divergent defaults; now every path persists the requested model or the unified default, and empty/whitespacemodelvalues normalize to the default.Review notes
createSessionWithInitialChatnow requiresmodelIdand writes it;handleStartChatRun→provisionRunSessionforwards it to the chat insert.createSessionHandler,createSessionChatHandler, workflow stream) setmodel_idexplicitly usingDEFAULT_MODEL.DEFAULT_RUN_MODEL_ID, local constants); all consumers importDEFAULT_MODEL. API route docs reflect the new default.getSessionChatsdefaultModelIdnow useDEFAULT_MODEL.modeland falls back when empty/whitespace.Migration and sequencing
createSessionWithInitialChatmust passmodelId.chats.model_idcolumn default.Written for commit bb879e6. Summary will update on new commits.
Summary by CodeRabbit
moonshotai/kimi-k3.