fix: drop the chats.model_id column default (model provenance, chat#1956) - #56
Conversation
…956) 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>
|
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. |
📝 WalkthroughWalkthroughThe migration removes the default from ChangesChat model provenance
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The migration removes the database default so omitted model IDs become NULL, but its schema comment overstates the reliability of historical non-NULL values, and deploying before the dependent writer rollout could leave interactive chats without a model ID. Merge should wait for the writer rollout and either correct the historical-data wording or explicitly accept that bounded risk. Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updates to Preview Branch (fix/drop-chats-model-id-default) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@supabase/migrations/20260812230000_drop_chats_model_id_default.sql`:
- Around line 30-31: Update the column comment on public.chats.model_id so its
provenance guarantee applies only to rows created after the default was removed,
while explicitly acknowledging that historical non-NULL values may have been
generated by the former database default. Preserve the existing NULL semantics
and avoid claiming that every present value was chosen by a writer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1862f9e7-71ab-4f26-9595-f0ea25755e99
📒 Files selected for processing (1)
supabase/migrations/20260812230000_drop_chats_model_id_default.sql
| comment on column public.chats.model_id is | ||
| 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope the provenance guarantee to new rows.
Existing rows remain unchanged. Lines 18-22 state that historical non-NULL values can be fabricated by the former database default. The current comment says every present value was chosen by a writer. That statement is false for some historical rows and can cause consumers to treat fabricated provenance as recorded provenance.
Proposed fix
comment on column public.chats.model_id is
- 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.';
+ 'For rows written after the explicit-writer rollout, this is the model recorded by the writer (chat#1956). NULL means "not recorded". Historical non-NULL values can reflect the former database default.';📝 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.
| comment on column public.chats.model_id is | |
| 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.'; | |
| comment on column public.chats.model_id is | |
| 'For rows written after the explicit-writer rollout, this is the model recorded by the writer (chat#1956). NULL means "not recorded". Historical non-NULL values can reflect the former database default.'; |
🤖 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 `@supabase/migrations/20260812230000_drop_chats_model_id_default.sql` around
lines 30 - 31, Update the column comment on public.chats.model_id so its
provenance guarantee applies only to rows created after the default was removed,
while explicitly acknowledging that historical non-NULL values may have been
generated by the former database default. Preserve the existing NULL semantics
and avoid claiming that every present value was chosen by a writer.
There was a problem hiding this comment.
1 issue found across 1 file
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="supabase/migrations/20260812230000_drop_chats_model_id_default.sql">
<violation number="1" location="supabase/migrations/20260812230000_drop_chats_model_id_default.sql:31">
P2: The persisted column comment makes a historical claim that is false: rows created before the `api#830` cutover can still contain the old database default even though no application writer chose it. Qualify the invariant with the cutover and mark historical rows untrustworthy, otherwise schema consumers may treat fabricated model IDs as provenance.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| alter column model_id drop default; | ||
|
|
||
| comment on column public.chats.model_id is | ||
| 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.'; |
There was a problem hiding this comment.
P2: The persisted column comment makes a historical claim that is false: rows created before the api#830 cutover can still contain the old database default even though no application writer chose it. Qualify the invariant with the cutover and mark historical rows untrustworthy, otherwise schema consumers may treat fabricated model IDs as provenance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260812230000_drop_chats_model_id_default.sql, line 31:
<comment>The persisted column comment makes a historical claim that is false: rows created before the `api#830` cutover can still contain the old database default even though no application writer chose it. Qualify the invariant with the cutover and mark historical rows untrustworthy, otherwise schema consumers may treat fabricated model IDs as provenance.</comment>
<file context>
@@ -0,0 +1,31 @@
+ alter column model_id drop default;
+
+comment on column public.chats.model_id is
+ 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.';
</file context>
| 'Model recorded at provision time by the writer (chat#1956). NULL means "not recorded" — never defaulted at the database layer, so a value present is a value some writer chose.'; | |
| 'Model recorded at provision time by the writer (chat#1956). For rows written after the api#830 cutover, NULL means "not recorded"; a value present is a value some writer chose. Rows created before that cutover may contain the old database default and are untrustworthy.'; |
Verified on the Supabase branch deployment (
|
| Check | Expected | Observed on branch | |
|---|---|---|---|
| Column default | gone | information_schema.columns → column_default: NULL |
✅ |
| Column comment | the "NULL means not recorded" semantics | col_description returns the full comment verbatim |
✅ |
Insert without model_id |
NULL — honest "not recorded", not a fabricated model |
model_id: NULL |
✅ |
Insert with model_id |
value kept | moonshotai/kimi-k3 |
✅ |
(Behavioral checks used throwaway account/session/chat rows on the branch, deleted after.)
Merge gate satisfied: recoupable/api#830 is merged and live on Production (fd18f18e, deployed 2026-08-13T02:28:56Z, status success) — every chats writer now sets model_id explicitly, so this default has zero remaining writers.
Reminder for whoever merges: this repo has no CI apply step — after merge the migration still needs to be applied to the main project.
🤖 Generated with Claude Code
Closes the database row of recoupable/chat#1956.
What
One migration:
alter table public.chats alter column model_id drop default;plus a column comment stating the new semantics — NULL means "not recorded"; a value present is a value some writer chose.Why
The
'anthropic/claude-haiku-4.5'default fabricated model provenance: any writer omittingmodel_idproduced a row indistinguishable from a deliberate model choice. Reproduced 2026-08-12 with fourPOST /api/chat/runscalls on four different explicit models — all four chat rows read the default whileusage_eventsbilled the real models (evidence table in recoupable/chat#1956).Merge sequencing — DO NOT merge before recoupable/api#830
Hard dependency: api#830 makes every
chatswriter (headless + interactive) setmodel_idexplicitly. Dropping the default first would leave interactive chats withNULLwhile the UI still expects a value. After api#830 is live, this default has zero remaining writers.No backfill, deliberately
Historical fabricated rows can't be told apart from genuinely-defaulted interactive rows — rewriting in either direction fabricates provenance the other way. Date-fence instead: rows before api#830's deploy are untrustworthy;
usage_eventsremains the record for completed runs.The read path is already NULL-safe (
chat.model_id ?? DEFAULT_MODEL_ID,api/lib/chat/handleChatWorkflowStream.ts:93).🤖 Generated with Claude Code
Summary by cubic
Drops the
public.chats.model_idcolumn default to stop fabricating model provenance. Previously the column defaulted to 'anthropic/claude-haiku-4.5' when omitted; now it has no default and NULL means "not recorded."usage_eventsfor historical records.model_idis NULL.Written for commit c22fe2c. Summary will update on new commits.