feat: add organization chat model migration and telemetry - #27956
feat: add organization chat model migration and telemetry#27956ethanndickson wants to merge 1 commit into
Conversation
|
/coder-agents-review |
|
@codex review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 5 findings (1 P1, 1 P3, 3 Note), COMMENT. Review Finding inventoryFinding inventory: PR 27956Findings
Contested and acknowledged(none) Round logRound 1Netero-only (P1 present, panel gated). 1 P1, 1 P3, 3 Note. Reviewed against 425aa7a..9f6d499. Orchestrator independently verified the CRF-1 rewind/re-apply structure in migrate_test.go. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f6d4996e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UPDATE chats c | ||
| SET last_model_config_id = m.copy_id | ||
| FROM model_config_copy_map m | ||
| WHERE c.last_model_config_id = m.orig_id | ||
| AND m.org_id = c.organization_id; |
There was a problem hiding this comment.
Resolve new chat model IDs within the requested organization
This one-time remap only fixes chats that exist when migration 566 runs. In the inspected create-chat flow, coderd/exp_chats.go:1321 still resolves the model without passing req.OrganizationID, and defaultCreateChatModelConfigID at lines 4480-4497 explicitly selects the default organization's config. Consequently, every later chat created without an explicit model in a non-default organization is again persisted with a cross-organization last_model_config_id; explicit IDs are likewise not checked against the requested organization. Pass the target organization through model resolution and handle organizations created after the migration so new writes preserve the same-org invariant established here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
First-pass review only: these are mechanical findings from Netero. The full review panel has not yet reviewed this PR and will review after these findings are addressed.
The migration itself holds up well under scrutiny: the unique partial default index cannot be violated by the copy insert (pre-566 all configs live in the default org), the 530-line test covers per-org counts, all four reference remaps, ACL re-key, threshold fan-out with hostile keys, and a full down/up round-trip, and the down's accepted fidelity loss is documented in both the file and the PR description. Severity count: 1 P1, 1 P3, 3 Notes.
The P1 is the one that matters given the stack: the test's rewind-and-step-to-latest re-applies every migration after 566, so the stacked RBAC PR adding 000567 will redden this test on its merge ref the moment it rebases onto this commit. Netero verified this by simulation with a fake 000567, and I independently confirmed the rewind/re-apply structure in the test.
Fun quote from the first pass: "The false rationale actively misleads: it hides the fact that stopping the stepper at a target version is safe, which is precisely the fix for the P1 above."
🤖 This review was automatically generated with Coder Agents.
| // rewinding the version row to the predecessor and stepping to latest, | ||
| // which re-applies this migration's up over the seeded rows. | ||
| _, err := sqlDB.ExecContext(ctx, fmt.Sprintf( | ||
| "TRUNCATE schema_migrations; INSERT INTO schema_migrations (version, dirty) VALUES (%d, false)", migrationVersion-1)) |
There was a problem hiding this comment.
P1 [CRF-1] The schema_migrations rewind re-applies every migration after 566, so the test deterministically fails as soon as any later migration exists; the stacked RBAC PR that this PR must merge before adds exactly that migration (000567). (Netero)
The test rewinds the version row to 565 and steps back to latest (twice: lines 3177 and 3466). The stepper then re-runs not just 566 but every later migration against a schema that already has them applied. Verified by simulation: I added a fake
000567containing a singleADD COLUMNand the test failed withcolumn "test_rbac_col" of relation "chat_model_configs" already exists. ThestepperUpToLatestcomment (lines 2965-2970) explicitly designs for "a stacked PR may add a later migration" but only made the assertion tolerant; the rewind re-application is not.
Orchestrator: independently confirmed the rewind to migrationVersion-1 followed by step-to-latest in the test. Consequence: the child RBAC PR's merge ref reddens on this test the moment it rebases onto this commit, unless 000567 happens to be fully idempotent DDL. Fix: stop advancing the stepper once version >= migrationVersion after a rewind (safe: each next() call commits one migration, see pgTxnDriver.Unlock), or apply the up file directly via MigrationFS() exactly as the test already does for the down.
🤖
|
|
||
| // MigrationFS exposes the embedded migration files, for tests that need to | ||
| // execute a single migration's SQL outside the migrate driver (the driver's | ||
| // transaction commits only when a stepper exhausts, which mid-test |
There was a problem hiding this comment.
P3 [CRF-2] The stated rationale for MigrationFS ("the driver's transaction commits only when a stepper exhausts") is factually false; each stepper call commits. (Netero)
Verified against golang-migrate v4
Migrate.Steps(lock, run n migrations, unlock) andpgTxnDriver(Lock begins the tx, Unlock commits it). SoStepper'snext()commits one migration per call. The same false claim appears in the test comment at migrate_test.go:3415-3417. The real reasonMigrationFSis needed is that the package exposes no single-step down API (Stepperonly callsSteps(1)upward).
The false rationale hides the fact that stopping the stepper at a target version is safe, which is precisely the fix for CRF-1. Correct both comments to name the actual constraint (no down-stepping API).
🤖
| WHERE d.model_config_id = cmc.id AND c.organization_id = o.id) | ||
| ); | ||
|
|
||
| INSERT INTO chat_model_configs |
There was a problem hiding this comment.
Note [CRF-3] The 18-column INSERT for soft-deleted copies duplicates the live-copy INSERT at line 41 verbatim except for the WHERE cmc.deleted filter. (Netero)
Staging both map populations first and running one unfiltered INSERT joined to the map would eliminate the duplication. Drift risk is confined to the pre-merge window (migrations freeze after merge), so this is informational only.
🤖
| -- Copies keep deleted/deleted_at so every historical reference has an | ||
| -- FK-valid, attribution-preserving target without resurrecting the config. | ||
| INSERT INTO model_config_copy_map (orig_id, org_id, copy_id) | ||
| SELECT DISTINCT ON (cmc.id, o.id) cmc.id, o.id, gen_random_uuid() |
There was a problem hiding this comment.
Note [CRF-4] DISTINCT ON (cmc.id, o.id) without ORDER BY is redundant. (Netero)
The FROM clause joins two tables on their primary keys, so each (cmc.id, o.id) pair occurs at most once; the EXISTS predicates do not multiply rows. Harmless, but implies a duplication risk that does not exist.
🤖
| -- Keys with a malformed or empty suffix are guarded BEFORE the uuid cast | ||
| -- (they cannot name an existing config, so they are pruned like any other | ||
| -- dangling key) instead of aborting the down. | ||
| DELETE FROM user_configs uc |
There was a problem hiding this comment.
Note [CRF-5] The down deletes pre-existing dangling threshold keys the up never created (malformed suffixes and keys referencing hard-deleted configs), not just the fanned-out copies. (Netero)
Documented in the file and asserted in the test (the two seeded hostile keys are pruned). Consistent with the accepted best-effort down; recorded so the behavior is a known decision, not an oversight.
🤖
Part of CODAGT-709.
The data cutover. Migration
000566copies the default organisation's live configs into every other live organisation, copies soft-deleted configs only into organisations that reference them, remapschats.last_model_config_id(and the message, queued-message, and debug-run references) onto the per-org copies, fans out per-userchat_compaction_threshold_pct:<id>overrides inuser_configs, and backfills any missinggroup_acl. Also addsmigrations.MigrationFS()so the migration test can run the down file directly, andorganization_idto the chat model config telemetry snapshot.This must merge before the RBAC PR above it, which adds migration
000567; the stack order enforces that.The down migration is best-effort: the copy map lives in a temp table, so rollback matches copies by
(ai_provider_id, model). A config an org created organically after the migration that happens to share a provider and model with a default-org config would be deleted on rollback. We've accepted this; rolling back immediately after deploy is safe, and the alternative was persisting provenance for a one-off cutover.