feat(coderd): org-scope chat model config schema - #27955
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 425aa7af9a
ℹ️ 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".
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 34 findings (7 P2, 9 P3, 1 P4, 8 Nit, 9 Note), COMMENT. Review Finding inventoryFinding inventory: PR #27955Findings
Law analysis
Contested and acknowledgedCRF-3 (Nit, coderd/database/queries/chatmodelconfigs.sql:18) - ByOrganization suffix on modified queries
CRF-5 (Note, coderd/exp_chats.go:7133) - ensureDefaultChatModelConfig all-orgs scan
CRF-3 panel closure (R2, unanimous)
Orchestrator downgrades (R2)
Round logRound 1Netero-only (P2 findings gate the panel). Law ran (advisory split). 2 P2, 2 Nit, 1 Note. Reviewed against 1e578a6..425aa7a. Round 2 churn guardPROCEED. CRF-1, CRF-2, CRF-4 author fixed (ef9a1a7). CRF-3 contested. CRF-5 acknowledged. PR restructured after Law's advisory split: now second layer of a 7-PR stack; base PR #27968 carries the PATCH/DELETE locked-read rewrite and its three behavior changes. Base is now e19560c (the #27968 branch), not main. Author corrected two round-1 report claims: the three 412 assertions pre-date this PR (exp_chats_test.go:4674/4690/4712 on main), and the dropped compression_threshold re-validation is unobservable (DB CHECK bounds 0..100). An external reviewer (codex) P2, fallback gated on len(rows) so an org with all configs disabled borrowed default-org models, was fixed in ef9a1a7 via ownership check (defaultConfig.OrganizationID == organizationID) plus two subtests. Round 2First full panel round: 23 reviewers (21 trigger-matched incl. always-on and Gon/Leorio floor, wildcards Luffy and Nami drawn from full pool after the distilled set was exhausted). Netero ran first (1 Nit, 2 Notes, CRF-6/7/8). Panel produced CRF-9 through CRF-36: 5 P2, 9 P3, 1 P4, 6 Nit, 8 Note posted; 1 dropped (CRF-35), 1 merged to body (CRF-36). CRF-3 closed by unanimous panel acceptance of the author defense. CRF-1/2/4 and the codex P2 fix independently verified by multiple reviewers (tests run against live Postgres). Orchestrator verified default-org immutability (organizations.sql) to settle Komugi/Meruem contradiction, and Bisky's coverage-gap claim (3 dbgen.ChatModelConfig calls in exp_chats_test.go, none in a second org). Reviewed against e19560c..ef9a1a7. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
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 change is well built: the migration ships a dedicated up/down/re-up test with fixture, test density is 67.9%, dbauthz entries cover all three changed queries, the removed LockIDChatModelConfigWrites was the last iota entry so prior lock values are unchanged, and ARCHITECTURE.md was kept current. Findings: 2 P2, 2 Nit, 1 Note.
Because effective additions exceed 1000 lines, a decomposition analysis (Law) also ran. Verdict: split, advisory. Proposal: extract the PATCH/DELETE rewrite in exp_chats.go (~200 production lines) into its own PR that lands first against the existing global lock; it is independent of the schema change, and it carries three user-visible API behavior changes (412 on unconfigured/disabled provider, 404 on vanished row in DELETE, dropped compression-threshold re-validation) that currently have zero dedicated tests in this diff. As Law put it, three contract changes are "buried in a schema PR whose title says "schema"." The split is advisory, but if the PR stays whole, the minimum acceptable outcome is dedicated tests for those three paths in this PR; the panel will hold that line.
🤖 This review was automatically generated with Coder Agents.
Stage 1 of CODAGT-709 adds organization ownership and ACL storage to chat model configs. It scopes the live-default invariant, runtime model resolution, and cache entries by organization while retaining the temporary default-organization fallback. Model-config writes use organization-derived advisory locks. Update and delete paths re-read mutable state after they acquire the lock. Rollback preserves all model configs and retains only the default organization's live default before restoring the global index. Refs CODAGT-709 Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
425aa7a to
ef9a1a7
Compare
Review round 1 addressedHead is now Structural change since the last review: the review flagged this PR as a decomposition candidate, and I agreed. It is now the second layer of a 7 PR stack (#27969) rather than the first of 6. A new base PR #27968 carries the locked-read fix for the Fixed
Declined
Full reasoning is in each thread. Two corrections to the round 1 report
Generated by mux |
|
@codex review |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9a1a7537
ℹ️ 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".
| ) error { | ||
| return api.Database.InTx(func(tx database.Store) error { | ||
| if err := tx.AcquireLock(ctx, database.LockIDChatModelConfigWrites); err != nil { | ||
| lockID := database.GenLockID("chat_model_config_writes:" + organizationID.String()) |
There was a problem hiding this comment.
Preserve the legacy advisory lock during upgrades
During a rolling upgrade, older replicas still acquire LockIDChatModelConfigWrites, while upgraded replicas acquire this unrelated organization hash for the same default-organization writes. The two versions can therefore execute the read/decide/write transaction concurrently; for example, concurrent first-config creates can both decide to self-promote, causing one request to fail on the unique index instead of being serialized. Keep the legacy ID reserved and coordinate default-org writes with it while mixed-version replicas can coexist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
First full panel round: 23 reviewers across database, Go, concurrency, auth, and testing domains, following round 1's Netero-only first pass.
This PR holds up well under that much scrutiny. The migration earned repeated independent praise: correct expand ordering (nullable, backfill, SET NOT NULL), a down migration that demotes foreign defaults before re-creating the global index, and a test that round-trips up/down/up including the soft-deleted row, both unique-index rejections, and the ACL CHECK. Multiple reviewers verified ARCHITECTURE.md claim-by-claim against the code with no divergence, and the codex round-1 fallback fix was independently re-verified by nine reviewers running the new subtests against live Postgres. Round 1 fixes (CRF-1, CRF-2, CRF-4) all verified.
Findings: 5 P2, 9 P3, 1 P4, 6 Nit, 8 Note. No P0/P1. The recurring theme is the ownership invariant ("an org that owns configs owns a default"): it is load-bearing for the fallback (codex fix), maintained only by write-path discipline in another package, undocumented at its maintainer (CRF-18), untested for cross-org isolation (CRF-9), and inferable rather than structural (CRF-34). The M2 cutover migration is a write path that can bypass it entirely; several findings converge on making it either documented and pinned, or structural via an existence check.
CRF-3 (ByOrganization suffix) is closed: the panel unanimously accepts the author's defense, on the evidence that the convention is a repo minority (independently re-counted) and #27959 gives the suffix a conflicting meaning in the same file.
One flagged disagreement: on the chatd subject's site-wide org read (CRF-26), Kurapika argues for a purpose-built narrow subject while six reviewers deem the grant acceptable as the narrowest static option; both positions are in the comment.
Process observations: restructuring the stack to move the locked-read rewrite into #27968 was the right call and made this diff reviewable as one concern. One process miss (Mafu-san): the round-1 CRF-2 fix consolidated the duplicated fallback in configcache.go but did not generalize to the sibling duplicated default-org block in exp_chats.go (both copies existed at the round-1 head); a grep for GetDefaultOrganization(dbauthz.AsChatd at fix time would have caught both. That code change is tracked as CRF-6. Schema side effects were enumerated: the new ON DELETE CASCADE on organization_id is a dead path today since organizations are only ever soft-deleted, and the backfill cannot fail NOT NULL because migration 000198 guarantees the default org.
As Robin put it about the duplicated test-helper defaulting: "If the two copies drift, one of them will be quietly wrong with no one to mourn it."
coderd/x/chatd/chatd.go:1605
P4 [CRF-27] Explicit model-config IDs are validated for existence and enabled-ness, never for organization binding. (Kurapika P4, Kite Note)
Kurapika:
resolveFallbackModelConfigID(preserved-ID branch),requireEnabledChatModelConfig(chatd.go:1560), andvalidateCreateModelConfigID(chatd.go:1580) all accept any config UUID ... Nothing checksconfig.OrganizationID == chat.OrganizationID. Today this is unexploitable: the only producers put every config in the default org ... but the moment the cutover PR copies configs into every org, a user in org B who learns a config UUID from org A can pin their chat to it and burn org A's provider credentials.
The enforcement must land no later than the cutover migration; the API/RBAC PRs (#27957, #27959) should be verified to add this binding check. Recorded here because this PR creates the data model in which configs become org property.
🤖
coderd/x/chatd/configcache.go:54
P3 [CRF-31] cloneModelConfig claims to isolate cached state but does not clone the two map fields this PR adds to ChatModelConfig. (Razor, Nami)
Razor:
ChatModelConfignow carriesGroupACLandUserACLof typeChatACL = map[string]ChatACLEntry(models.go:5155), andcloneModelConfigcopies the struct shallowly, so the cache entry and every caller alias the same maps (and thePermissionsslices inside them). Today nothing in chatd reads or writes these fields ... The RBAC PR later in the stack makes them load-bearing: any in-place mutation of an ACL on a config obtained from the cache would corrupt the shared cache entry, and a concurrent mutation racing a reader is a map-access panic.
The fix belongs here because this PR introduces both the fields and the aliasing. Note (Nami): ChatACLEntry carries a Permissions slice, so a shallow maps.Clone still shares the slices; copy those too or document that entries are immutable.
🤖
🤖 This review was automatically generated with Coder Agents.
| // authorized the operation, and a custom role holding only | ||
| // deployment-config permissions cannot read the organization object. | ||
| //nolint:gocritic // Internal default-org resolution, scoped to this call. | ||
| defaultOrg, err := api.Database.GetDefaultOrganization(dbauthz.AsChatd(ctx)) |
There was a problem hiding this comment.
Nit [CRF-6] The default-org resolution block (AsChatd GetDefaultOrganization call, the "internal resolution step, not user-readable data" justification comment, and the 500 error response) is duplicated between createChatModelConfig (6698-6712) and defaultCreateChatModelConfigID (4483-4496). (Netero, Gon)
The two blocks carry the same rationale comment and the same
//nolint:gocriticescape. When the M3 cutover removes this pre-cutover resolution, two sites must be found and deleted instead of one. Extract a small(api *API) defaultOrgForChatModelConfigs(ctx)helper carrying the comment once.
Gon adds that the same rationale ("cannot read the default organization object") appears five times across production and test files in this PR; when this duplication is resolved, the rationale should live once on the helper and call sites can drop to a reference.
🤖
| ) error { | ||
| return api.Database.InTx(func(tx database.Store) error { | ||
| if err := tx.AcquireLock(ctx, database.LockIDChatModelConfigWrites); err != nil { | ||
| lockID := database.GenLockID("chat_model_config_writes:" + organizationID.String()) |
There was a problem hiding this comment.
Note [CRF-7] The switch from the deployment-wide LockIDChatModelConfigWrites to a per-org GenLockID has no test proving cross-org lock independence or lock-ID stability. (Netero)
Unverifiable cheaply and unobservable in this PR: every write path still resolves to the default org, so all writes contend on one lock exactly as before. The per-org property only becomes load-bearing after the cutover PRs add org-scoped routes; a contention test would belong there. Recording so the stack does not ship the property untested.
🤖
| } | ||
|
|
||
| configs, err := store.GetEnabledChatModelConfigs(ctx) | ||
| configs, err := store.GetEnabledChatModelConfigsByOrganization(ctx, chat.OrganizationID) |
There was a problem hiding this comment.
Note [CRF-8] Chat-time model resolution is now org-scoped with the default-org fallback, while getUserChatProviderAvailability (exp_chats.go:1010) still lists via the deployment-wide GetEnabledChatModelConfigs. (Netero)
The two views coincide pre-cutover (every config lives in the default org) and the PR description assigns the route/API changes to a later stack PR, so this is not a defect here. It is the seam to watch: if the cutover migration lands before the API PR, availability reporting and resolution diverge for non-default orgs.
Nami confirmed the seam is currently invisible to users: listChatModelConfigs and the availability endpoint see the same set as the resolver until the cutover lands. See CRF-14 for a sibling location of this seam.
🤖
| return xerrors.Errorf("list chat model configs: %w", err) | ||
| } | ||
| if len(modelConfigs) == 0 { | ||
| orgModelConfigs := make([]database.ChatModelConfig, 0, len(modelConfigs)) |
There was a problem hiding this comment.
P2 [CRF-9] Org isolation of default-config management on the write path has zero discriminating coverage: delete the org filter here, or the organization_id filter from UnsetDefaultChatModelConfigs, and every test in the suite stays green. (Bisky)
The PR's central invariant is that a write in org A never touches org B's defaults. Two pieces of code carry it: the SQL filter in
UnsetDefaultChatModelConfigs(chatmodelconfigs.sql:151) and the Go filter inensureDefaultChatModelConfig(exp_chats.go:7136-7141). I grepped every caller and every fixture: noexp_chats_test.gowrite-path test ever seeds a chat model config in a second organization ... Every promotion test runs with all rows in one org, so both filters are no-ops in every execution.
The test isn't hard: via the raw store, seed a config with
IsDefault: truein a second org (the migration test already proves the index allows it). Then via the API: (1) create a second config in the default org, PATCH it to default, assert the second org's default is stillis_default = true; (2) delete the default org's default with a non-default candidate present, assert the promoted config belongs to the default org and the second org's rows are byte-for-byte untouched.
Orchestrator verified the fixture claim: only 3 dbgen.ChatModelConfig calls exist in exp_chats_test.go, none in a second org. #27957 and #27959 rewrite this same call next; without this coverage, a rewrite that drops either filter ships silently.
🤖
| @@ -7035,8 +7086,9 @@ func (api *API) deleteChatModelConfig(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Note [CRF-10] The commit message sentence "Update and delete paths re-read mutable state after they acquire the lock" describes the locked-read rewrite that moved to base PR #27968 during the restack; this commit only captures and reuses the re-read results. (Mafu-san)
The re-read pattern pre-exists in the base (verified:
git show e19560c9:coderd/exp_chats.gohas the "Re-read the row under the write lock" comment). A future reader or bisect attributes the rewrite to this commit. One-sentence commit-message edit fixes it. The PR body, by contrast, correctly discloses that the handler behavior changes now live in #27968.
🤖
| // TODO(mafredri): remove after CODAGT-709 M3 (org-scoping cutover); | ||
| // orgs list strictly within their own configs. | ||
| func enabledChatModelConfigsWithDefaultOrgFallback( | ||
| ctx context.Context, |
There was a problem hiding this comment.
Note [CRF-29] Pre-cutover, every list-path call from a non-default org that owns no configs fans out to 5 sequential DB round trips where 1 sufficed; this is the common case, not the rare one, since configs only exist in the default org until the cutover PR. (Killua)
Derived from code, not estimated:
GetEnabledChatModelConfigsByOrganization(org)(empty) +GetDefaultChatModelConfig(org)(miss) +GetDefaultOrganization+GetDefaultChatModelConfig(defaultOrg)+GetEnabledChatModelConfigsByOrganization(defaultOrg). ... All indexed point queries on a tiny table, per-user-action not per-token, and the whole helper carries a removal TODO for the M3 cutover in the same stack. This does not need fixing; it is the quantified cost the cutover PR deletes.
🤖
| // exactly when it owns a default. | ||
| // TODO(mafredri): remove after CODAGT-709 M3 (org-scoping cutover); | ||
| // orgs list strictly within their own configs. | ||
| func enabledChatModelConfigsWithDefaultOrgFallback( |
There was a problem hiding this comment.
P3 [CRF-30] enabledChatModelConfigsWithDefaultOrgFallback makes every caller run the query it then judges; both call sites duplicate the same two-step dance, and the shape invites a third caller to run the query and forget the fallback. (Zoro P3, Meruem P3, Robin P3, Luffy Nit, Ryosuke Nit)
Zoro: The helper takes
rowsthat the caller must have fetched viaGetEnabledChatModelConfigsByOrganizationfor the sameorganizationIDit also passes. It absorbs no work: both callers (subagent.go:633-640, chatd.go:2784-2787) perform the identical query-then-fallback sequence, andresolveManualTitleModelneeds the awkwardif err == nil { ... }chaining to compose the two steps.
Robin: The miss this shape permits is not hypothetical:
exp_chats.go:1010already lists enabled configs without the fallback (the CRF-8 seam).
Five reviewers converged. Absorb the query into the helper (enabledChatModelConfigsForOrg(ctx, db, orgID); Zoro's sketch keeps behavior and query count identical, verified only these two callers exist) and co-locate it with defaultChatModelConfigForOrg in chatd.go, which it wraps and which shares its M3 removal deadline. The M3 removal becomes one contiguous deletion, and Komugi's race fix (CRF-15) becomes natural inside the absorbed helper.
🤖
| if !errors.Is(err, sql.ErrNoRows) { | ||
| return database.ChatModelConfig{}, err | ||
| } | ||
| defaultOrg, err := store.GetDefaultOrganization(ctx) |
There was a problem hiding this comment.
P3 [CRF-32] defaultChatModelConfigForOrg propagates a GetDefaultOrganization failure without step context, and no test exercises that branch. (Chopper)
The helper runs up to three queries ... and returns non-ErrNoRows errors from all of them raw. Every caller wraps the result as one signal ... an operator sees a chat fail with "get default chat model config: " and starts debugging
chat_model_configsqueries, when the actual failure was the default-organization read inside the pre-cutover fallback, a step that exists only until M3. The misdirection costs exactly the time the temporary code was supposed to save.
Wrap the middle hop (xerrors.Errorf("get default organization: %w", err)); sql.ErrNoRows matching at callers is unaffected since that branch returns before the wrap. The uncovered branch is one stub error away from tested.
🤖
| require.NoError(t, err, "get default organization") | ||
| organizationID = defaultOrg.ID | ||
| } | ||
| groupACL := seed.GroupACL |
There was a problem hiding this comment.
Note [CRF-33] dbgen.ChatModelConfig seeds group_acl as {} while every production write path seeds the everyone-in-org read entry. (Chopper, Ryosuke, Robin)
Chopper: ACLs are inert in this PR, so nothing breaks now, but every dbgen-created fixture represents a config no member can read once the RBAC PR evaluates ACLs. The RBAC PR's tests will either flush this out loudly or quietly pass against unrepresentative fixtures.
Robin adds that the org-and-ACL defaulting is now taught to two test helpers in parallel (dbgen.ChatModelConfig and insertChatModelConfigForTest in querier_test.go:12269); if CRF-24 lands, the ACL half of both blocks disappears, and the default-org lookup remains written twice. Align the dbgen default with the production seed here or in the RBAC PR.
🤖
| } | ||
| return nil, xerrors.Errorf("resolve default chat model config: %w", err) | ||
| } | ||
| if defaultConfig.OrganizationID == organizationID { |
There was a problem hiding this comment.
P3 [CRF-34] Fallback ownership is decided by default-config ownership, which encodes an invariant no schema, constraint, or test enforces; a direct existence check makes the bad state impossible instead of assumed away. (Ryosuke P3, Pariston Note)
Ryosuke: The fix works by inference:
defaultConfig.OrganizationID == organizationIDproves ownership only if "an org that owns live configs also owns a default" always holds. That invariant lives in write-path discipline (ensureDefaultChatModelConfigunder the per-org lock) in package coderd, while the code that depends on it lives in package chatd. Nothing mechanical connects them ... and the M2 cutover migration in the next PR is a write path that bypassesensureDefaultChatModelConfigentirely. If it copies configs without promoting per-org defaults, the codex bug returns silently.
Pariston: If you want the fallback to stop depending on the invariant entirely, the structural test is one query: decide ownership by
EXISTS(SELECT 1 FROM chat_model_configs WHERE organization_id = $1 AND deleted = false)... same query count on the fallback path.
This is a structural alternative, not a re-open of the closed codex P2: the EXISTS check costs one small query only when the org's enabled list is empty, eliminates the cross-package invariant dependence, and removes the M2 cutover's silent failure mode. If declined, the minimum is CRF-18's documentation plus an explicit check in the M2 cutover review that it promotes a default per org (Luffy independently flagged the cutover as exactly such a writer).
🤖
|
Follow-up from the automated review of stacked PR #27957: the chatd subject comment in Generated by mux on behalf of @ethanndickson. |
Part of CODAGT-709. Second of a seven PR stack scoping chat model configs to organisations for Coder Agents GA.
This adds the schema and the Go plumbing. Migration
000565giveschat_model_configsa cascadingorganization_idFK (backfilled to the default org), swaps the global single-default unique index for a per-organisation one, and addsgroup_acl/user_aclJSONB columns, seeding an everyone-in-org read entry so members keep read access once the RBAC PR later in the stack lands. Default and enabled model lookups now take an organisation, chatd caches configs per organisation, and model-config writes serialise on a per-org advisory lock (GenLockID) instead of the deployment-wide constant.Chatd keeps a one-hop fallback to the default organisation's configs until the data cutover in the next PR copies configs into every org; the API PR removes the fallback entirely.
The handler behaviour changes this PR used to carry (the
412for an unconfigured or disabled provider, theDELETE404, and the droppedcompression_thresholdre-validation) now live in #27968 at the base of the stack.