feat!: scope chat model configs to organizations - #27955
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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 history
deep-review v0.9.0 | Round 4 | Last posted: Round 4, 76 findings (2 P1, 11 P2, 23 P3, 1 P4, 22 Nit, 17 Note), REQUEST_CHANGES. Review Finding inventoryFinding inventory: PR #27955Findings
| CRF-58 | P2 | Open (escalated: recurrence of the CRF-4 renumber-artifact class after correction) | coderd/database/migrations/testdata/fixtures/000574_chat_model_config_organization.up.sql:1 | Fixture header and seeded identifiers still say 571 after the 000574 renumber (same class as CRF-4's 552 artifact) | R4 | Mafu-san P2, Netero Nit | Yes | Law analysisR3
R1
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)
CRF-7 (Note, coderd/exp_chats.go:6600) - cross-org lock independence untested
CRF-12 (Note, coderd/exp_chats.go:6600) - rolling-upgrade lock disjointness
CRF-7 panel closure (R3, 3/4)
CRF-12 panel closure (R3, 4/5)
Orchestrator notes (R3)
CRF-44 (P3, coderd/exp_chats.go:6616) - deploy contract documentation
CRF-44 panel closure (R4, 4/5)
Orchestrator notes (R4)
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. Round 3 churn guardPROCEED. 25 addressed, 2 acknowledged (CRF-8, CRF-14), 2 contested (CRF-7, CRF-12), 0 silent. Head 2c7f10b (commits 7a713c7, 8c03537). Major restructure: fallback machinery replaced by single GetEffectiveChatModelConfigsByOrganization query; chatd org grant removed; org-binding checks added in chatd resolution (CRF-27). PR description regenerated; title now "scope chat models to organizations". New unresolved codex P1 at exp_chats.go:4535: omitted-model chat creation still resolves the deployment default org while chatd.CreateChat now requires the config to belong to req.OrganizationID. CI failing: test-go-pg, test-go-pg-17, test-go-race-pg. Round 3Full panel, 23 reviewers (wildcards Luffy, Melody). Netero advisory (CRF-37 P2, CRF-38 Nit); Law re-ran: Don't split (advisory). Dominant finding: the CRF-27 org-binding fix landed while the coderd resolvers still pin the deployment default org, turning the acknowledged CRF-14/CRF-8 seams into live P1 breakage (CRF-39 creation, CRF-40 pre-existing chats); 7 reviewers converged, codex P1 confirmed. CRF-7 and CRF-12 closed by panel vote; CRF-33 re-raised at root cause. New: 2 P1, 2 P2 (+CRF-37), 7 P3, 4 Nit, 3 Note posted; CRF-43/56 merged to body; CRF-57 dropped (disproved). CI failing on head (test-go-pg, test-go-pg-17, test-go-race-pg); review credentials could not read CI logs; reported in body. Event REQUEST_CHANGES. Reviewed against c0950c1..2c7f10b. Round 4 churn guardPROCEED. 17 addressed, 1 contested (CRF-44), 0 silent. Head ad400e1. CI green (19 passed). Migration renumbered 000571 -> 000574. Both P1s fixed per author: resolver org-threading plus foreign-LastModelConfigID fall-through to local default. Queued-head resolution moved into chatstate transitions (CRF-46/49/50 fixed structurally). Round 4Full panel, 22 reviewers (wildcards Zoro, Razor). CI green. Both R3 P1 fixes verified structurally (Kurapika chain-verified CRF-26/27/31; Knuckle verified every schema-domain fix; Bisky verified CRF-41/42; Komugi/Takumi/Ging-go clean). CRF-44 closed by panel (4/5). CRF-40 re-raised at P1: the fall-through fix rescues only orgs owning a local default, which no non-default org can have in this PR; zombie chats persist with clearer errors, codified by four tests; needs a human decision (seed at migration, land write-path cutover with this PR, or explicit sign-off). New: 2 P2, 7 P3, 10 Nit, 5 Note. CRF-33 remains re-raised and author-silent. Event REQUEST_CHANGES. Reviewed against 033bf39..ad400e1. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
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.
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".
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.
|
Follow-up from the automated review of stacked PR #27957: the chatd subject comment in Generated by mux on behalf of @ethanndickson. |
46e2aaa to
38f5109
Compare
ddd0964 to
c3e41d2
Compare
ethanndickson
changed the title
0615d01 to
2c7f10b
Compare
|
@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: 2c7f10b3a8
ℹ️ 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".
ethanndickson
changed the title
There was a problem hiding this comment.
Round 3, full panel (23 reviewers). The response to round 2 was excellent in volume and mostly in substance: 25 of 29 findings addressed with commit-linked evidence, and the structural rewrites are real improvements. Multiple reviewers verified the fixes: the single effective-configs query replaced the three-query fallback (CRF-15/29/30/32/34), the chatd subject's site-wide org grant is gone (CRF-16/26), cloneChatACL deep-clones including Permissions slices (CRF-31, now pinned by a mutation test), the org-binding checks landed everywhere (CRF-27, verified across six resolution sites), and LockIDChatModelConfigWrites is reserved again (CRF-11). Komugi, Kurapika, and Ging-go filed clean reports after real verification, including a race-clean run of the queued-promotion ordering test.
But this round the panel found the PR's first release-blocking defects. Findings: 2 P1, 2 P2, 7 P3, 4 Nit, 3 Note. REQUEST_CHANGES on the P1s.
The P1 story in one paragraph: the CRF-27 org-binding fix landed in this PR while every coderd resolver still pins the deployment default org. The seams that CRF-8 and CRF-14 acknowledged as "later API cutover" stopped being latent the moment chatd started enforcing config.OrganizationID == chat.OrganizationID. Seven panel reviewers plus codex converged: non-default-org chat creation now fails unconditionally on every entry path (CRF-39), and, worse, every pre-existing chat in a non-default org is bricked at runtime, sends, queue promotions, turn finishes, and edits all error (CRF-40). There is no in-API remediation because createChatModelConfig hardcodes the default org. As Hisoka put it: "the trap sprung on every chat that walked through the door before it was set."
CI is failing on this head (test-go-pg, test-go-pg-17, test-go-race-pg). The review environment's GitHub credentials could not read the CI logs, so the panel could not diagnose whether these failures share a root cause with the P1s; reviewers' targeted local runs of the new tests all pass. Failing checks are bugs: please fix them or state the cause.
Contested findings adjudicated: CRF-7 closed (3/4 accept; the lock property is throughput, not correctness, though the defense's claim that the behavior tests cover lock independence is inaccurate; they prove data isolation). CRF-12 closed (4/5 accept; the mixed-version stance is a legitimate deploy contract), with the one actionable residue filed as CRF-44: the contract exists only in a review reply, so write it down. CRF-33's fix converged three ACL literal copies without giving them an owner; re-raised on its thread at root cause.
Process observations. [CRF-43] The head commit 2c7f10b3 titled "repair organization model migration CI" bundles a genuine migration renumber with a load-bearing test inversion (SuccessNonDefaultOrgUsesDeploymentDefault became NonDefaultOrgDoesNotUseDeploymentDefault, assertion flipped, LastModelConfigID claim deleted) behind a mechanical label and an empty body; that inversion is exactly where the P1 hides, and a log reader scanning subjects will skip it. Split semantic test flips into their own honestly-titled commits. [CRF-56] The PR description says the change "scopes ... Chatd model selection ... to each chat organization" while the CRF-14 acknowledgment says creation stays deployment-default; one story, please. It also never says who needs per-org catalogs; a middle PR in a 7-PR stack that ships zero net user-visible behavior (and currently a regression) should carry its own justification.
A human decision is needed on CRF-39/CRF-40's direction: resolve per-org at creation now, or hold the chatd strict binding until the API cutover. Both are viable; shipping neither is not.
coderd/x/chatd/subagent.go:341
Note [CRF-55] Six unchanged slog messages in resolvePersonalSubagentModelConfigID (lines 341, 367, 393, 400, 408, 426) say "using deployment default" where the fall-through now resolves the chat organization's local default. (Pariston, Netero)
Pariston: the strings say the wrong thing about which default the caller will land on, which is the same premise gap the creation-path P1 catches on a different path (author's mental model still says "deployment default"). One fix line: replace "deployment default" with "chat organization default" across the six sites.
🤖
🤖 This review was automatically generated with Coder Agents.
4e42463 to
82110a6
Compare
|
$@codex review\n\nThis review request was generated by Coder Agents. |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad400e18d9
ℹ️ 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".
There was a problem hiding this comment.
Round 4, full panel (22 reviewers). The fix quality this round is high and the panel verified it hands-on: Kurapika chain-verified the org-binding enforcement end to end (CRF-26/27/31 all hold, including the new dbauthz test asserting the chatd subject is denied org read), Knuckle verified every schema-domain fix at head, Bisky confirmed the test-quality fixes (exact SDK message, positive siblings) and called the queued-promotion suite "real gems," and Komugi, Takumi, Ging-go, and Pariston filed clean reports after real verification. Moving the queued-head resolution inside the chatstate transaction closed CRF-46/49/50 at the root. CI is green.
Findings: 2 P2, 7 P3, 10 Nit, 5 Note, plus one P1 re-raise. REQUEST_CHANGES on the re-raise.
The re-raise (on the CRF-40 thread): the fall-through fix rescues chats only when the chat's org owns a local default, and after this migration no non-default organization can own one; the API writes configs exclusively to the default org. Every pre-existing chat in a non-default org still dies on send, promotion, and turn finish, now with a clearer error, and four new tests codify that state as intended. As Hisoka put it: "The R3 fix reads as though it rescued the described customer. It rescued the tests. The customer is untouched." The panel is not unanimous: Pariston reads ARCHITECTURE.md's "the default organization does not provide runtime fallback models" as a deliberate design with the write-path cutover legitimately deferred to its stack PR. Both positions are in the thread. What the panel agrees on: this cannot be closed by an agent's acknowledgment. A human must pick one: seed each org's default at migration time, land the org-scoped write path with this PR, gate the strict enforcement until the cutover, or explicitly sign off (with a ticket) on multi-org deployments being degraded between this merge and the cutover PR. The new CRF-64 raises the stakes: the same population triggers unbounded task-runner retry loops, not just clean 400s.
Contested finding adjudicated: CRF-44 closed (4/5 accept; standard schema-migration semantics, not a PR-specific contract; Meruem's dissent that the general contract is documented nowhere is recorded in the thread).
Still open with no author response: the CRF-33 re-raise from round 3 (three inline copies of the everyone-in-org ACL with no shared owner). Two rounds without a response; fix it, file a ticket, or explain why not.
Process observations (Mafu-san, Leorio): the 000571 -> 000574 renumber repeated the exact CRF-4 class from round 1, updating identifiers in migrate_test.go while leaving nine 571 artifacts in the fixture it loads; that recurrence after correction is why CRF-58 is P2. Eight of nine round-4 commits have empty bodies, including the two structural fixes for the R3 P1s, and two commits share the identical subject "fix(coderd): enforce organization-local chat models"; the CRF-43 signal from round 3 was applied to the flagged commit and not retained as a practice. Commit bodies naming the finding being addressed would have made this round's churn audit mechanical.
coderd/x/chatd/tasks.go:649
P2 [CRF-64] normalizeTaskTransitionError does not classify chatstate.ErrNoDefaultChatModelConfig, so async task loops retry a durable admin-configuration failure forever. (Melody P3; orchestrator escalated to P2 on the consequence chain)
Melody:
chatstate.ErrNoDefaultChatModelConfigfalls into the else arm, andrunTaskWithRetryloops ontaskRetryableErrorwithdefaultTaskRetryMaxBackoff = 5 * time.Seconduntil the context is canceled. When an org's local default is gone and a chat has a queued row, FinishTurn/FinishInterruption produce an admin-durable error.
Orchestrator verified: the sentinel is xerrors.New (does not wrap ErrNoRows), so no existing arm catches it; it is retryable today. The escalation: combined with the CRF-40 residual (every pre-existing non-default-org chat has no usable local default after this migration), each such chat with a queued head becomes a permanent 5s-backoff retry loop across FinishTurn and FinishInterruption. That is invisible resource drain at exactly the population the P1 re-raise names. Classify the sentinel as non-retryable (the ErrTransitionNotAllowed/ErrInvalidState arm is the right shape).
🤖
coderd/exp_chats.go:3189
P3 [CRF-63] promoteChatQueuedMessage's error switch is missing the chatd.ErrNoDefaultChatModelConfig arm that four sibling handlers gained this round. (Melody)
Consumer side, four other handlers were updated to pair with the new sentinel:
postChatMessages(2883),patchChatMessage(3048),regenerateChatTitle(3609), andproposeChatTitle(3658) each map it to 400 "No default chat model config is configured." ...transitions_test.go:172assertsPromoteQueuedMessagereturns exactly this sentinel from the "missing local default" path, so the case is reachable from this endpoint.
Orchestrator verified the switch at head: ErrChatArchived, ErrQueuedMessageNotFound, ErrChatNotFound, invalid-state, ErrTransitionNotAllowed, default. The reachable no-default path 500s here and 400s everywhere else. Add the arm.
🤖
🤖 This review was automatically generated with Coder Agents.
| @@ -0,0 +1,91 @@ | |||
| -- Fixture for 000571 (org-scope chat model configs). Fixtures apply at | |||
There was a problem hiding this comment.
P2 [CRF-58] The 000571 -> 000574 renumber updated identifiers in migrate_test.go and left the fixture it loads untouched: the header and nine seeded identifiers (fixture-openai-571, Fixture Default 571, 571c000{1,2,3}-... UUIDs) still say 571. (Mafu-san P2, Netero Nit)
Mafu-san: CRF-4 (R1, "Migration 000565 test/fixture identifiers suffixed 552") corrected exactly this pattern in Round 1. Not searching for siblings during a renumber recurred in Round 4, and the fix commit that renumbers eight identifiers in one file demonstrates that the pattern was in the author's hands at that moment.
P2 because it is a repeated same-class mistake after correction, not because of the artifact's own weight. Fix: sed -i 's/571/574/g' on the fixture (Netero verified all nine matches are renumber artifacts) and grep the tree for remaining 571 before landing.
🤖
| } | ||
| if !xerrors.Is(err, sql.ErrNoRows) { | ||
| if modelConfig.Enabled && modelConfig.OrganizationID == chat.OrganizationID && modelConfig.AIProviderID.Valid { | ||
| provider, providerErr := p.db.GetAIProviderByID( |
There was a problem hiding this comment.
P3 [CRF-62] The warm-cache path of resolveModelConfig now pays an uncached GetAIProviderByID every turn, and the caller immediately fetches the same provider again; CRF-45's hot-path goal is not met. (Killua P3, Ryosuke P3, Knov P3, Hisoka Note)
Knov: The fix restored
p.configCache.ModelConfigByID, but immediately below it addedp.db.GetAIProviderByID(...)to testprovider.Enabled. The only caller in production isresolveChatModel, which on success doesresolveModelRouteForConfig->gatewayProviderForConfig->enabledAIProviderByID->p.db.GetAIProviderByIDon the same provider ID a handful of lines later. Pre-diff,resolveChatModelpaid one provider read per turn; post-diff it pays two.
Four reviewers converged with the same two remedies: read provider enabled-state from configCache.EnabledProviders(ctx) (already cached under the same TTL and invalidation events), or return the provider from resolveModelConfig and thread it into resolveModelRouteForConfig so the read happens once. Ryosuke's framing: one decision ("is this config still usable") made twice at two layers; pick one owner.
🤖
| var errInvalidModelOverrideMetadata = xerrors.New("invalid model override metadata") | ||
| var ( | ||
| errInvalidModelOverrideMetadata = xerrors.New("invalid model override metadata") | ||
| errModelConfigOutsideOrganization = xerrors.Errorf("%w: model config belongs to another organization", sql.ErrNoRows) |
There was a problem hiding this comment.
P3 [CRF-65] errModelConfigOutsideOrganization silently wraps sql.ErrNoRows, making four switch sites order-dependent with nothing stating the coupling, and one site misses the arm entirely. (Gon P3, Knov Note, Mafuuu Nit)
Gon:
subagent.go:249(soft mode): theerrModelConfigOutsideOrganizationcase must come before thesql.ErrNoRowscase. Reorder the switch and the outside-org log message disappears;chatd.go:2828andtitle_override.go:87have the same ordering dependency.TestResolveConfiguredModelOverride_OrganizationMismatch.Softonly exercises the current ordering, so a reorder passes tests and silently loses the log.
Mafuuu:
resolveExplicitSpawnOverrides(subagent.go:606) checkssql.ErrNoRowswithout a preceding sentinel case, so the subagent tool returns "not found or is disabled" for a model that is neither.
Fix per Gon/Knov: either a one-line comment at the sentinel definition stating the wrap and the check-me-first rule, or drop the wrap and make each site chain both checks explicitly; and add the missing arm at subagent.go:606 either way.
🤖
| case chatModelConfigAvailable: | ||
| return modelConfig, 0, nil | ||
| case chatModelConfigUnavailableModelNotFoundOrDisabled: | ||
| case chatModelConfigUnavailableModelNotFoundOrDisabled, |
There was a problem hiding this comment.
P3 [CRF-66] validateUserChatModelConfigAvailable folds chatModelConfigUnavailableOutsideOrganization into the "model config not found or disabled" 400 while the create path returns the precise diagnosis, so the same condition gets two different answers by endpoint. (Mafuuu P3, Leorio P3, Chopper P3)
Leorio: The user posts to a chat with a model_config_id that lives in a sibling org. Create-chat says "does not belong". Send-message and edit-message say "not found or disabled". They then load the model list, see the model there, and cannot reconcile the two answers.
Affects postChatMessages, patchChatMessage, and putUserChatPersonalModelOverride. Kurapika's counter (folding hides foreign-org existence by design) is undercut by the create path already disclosing the precise message at exp_chats.go:4405; pick one policy. If the precise message wins, PUTModelRejectsChatOrgModel (exp_chats_test.go:15259) currently asserts the wrong substring and must move with it (Chopper).
🤖
| modelOpts, | ||
| ) | ||
| if overrideErr != nil { | ||
| if errors.Is(overrideErr, errModelConfigOutsideOrganization) { |
There was a problem hiding this comment.
P3 [CRF-67] The foreign-org title-override path diverges from every other unusable-override case three ways, silently. (Chopper P3, Zoro Note, Hisoka Note)
- Chopper:
resolveManualTitleModelcatches the sentinel and returns with no log, while the async path Debug-logs the same class; an admin asking "why didn't my override run on Regenerate Title?" gets nothing. - Zoro: the shortcut returns the org default via
resolveDefaultManualTitleModel, while other unusable-override cases fall through to the preferred-short-text scan; user-visible divergence whenever the org default is not the preferred short-text model, codified by the test without commentary. - Hisoka:
title_override.go:88's silentoverrideSet=falserewrite is load-bearing forquickgen.go:301and dead code forchatd.go:2828; a caller reorder flips semantics silently.
Either unify the foreign-org case with the other unusable-override fall-through (deleting the one-caller resolveDefaultManualTitleModel), or keep the shortcut and state the rationale at all three sites. A Debug log on the manual path is the floor.
🤖
| }) | ||
| } | ||
|
|
||
| func TestEnabledChatModelConfigsForOrganization(t *testing.T) { |
There was a problem hiding this comment.
Nit [CRF-80] TestEnabledChatModelConfigsForOrganization is TestGetEnabledChatModelConfigsByOrganization (querier_test.go) rebuilt in a second file: same 3-org fixture, same four configs, same four assertions, exercised through the one-line passthrough CRF-59 flags. (Bisky)
Delete the subagent test; the querier test already proves the query row-for-row. Two settings, one stone.
If CRF-59's inline-the-wrapper lands, this test loses its subject entirely.
🤖
| // wraps this sentinel. | ||
| ErrMessageQueueFull = xerrors.New("chat message queue is full") | ||
|
|
||
| // ErrNoDefaultChatModelConfig is returned when queued-message |
There was a problem hiding this comment.
Nit [CRF-81] The doc on ErrNoDefaultChatModelConfig says it is returned by queued-message promotion, which is one of its four producers. (Razor)
The sentinel is also returned by
resolveFallbackModelConfigID(chatd.go:1696),resolveDefaultManualTitleModel(chatd.go:2895), andresolveModelConfig(chatd.go:4454).
Align with the generic phrasing the chatd alias already carries.
🤖
| return database.ChatModelConfig{}, sql.ErrNoRows | ||
| } | ||
|
|
||
| func enabledChatModelConfigsForOrganization( |
There was a problem hiding this comment.
Nit [CRF-59] enabledChatModelConfigsForOrganization is a zero-value passthrough over store.GetEnabledChatModelConfigsByOrganization: no logging, no error context, no cache, no policy. (Netero)
Every call site passes exactly ctx, store, organizationID. Inline it or drop it; CRF-80 (the duplicated test built on it) goes with it.
🤖
| } | ||
| } | ||
| if _, status, resp := api.validateUserChatModelConfigAvailable(ctx, userID, *req.ModelConfigID); resp != nil { | ||
| _, reason, err := api.userCanUseChatModelConfig(ctx, userID, req.OrganizationID, *req.ModelConfigID) |
There was a problem hiding this comment.
Note [CRF-60] resolveCreateChatModelConfigID runs userCanUseChatModelConfig twice for every non-org failure of an explicit req.ModelConfigID: the inline call computes reason, then validateUserChatModelConfigAvailable re-invokes it with identical arguments to map the same reason to a response. (Netero)
Extract the reason -> (status, response) switch and pass the already-computed reason in. Interacts with CRF-66: that fix edits the same switch.
🤖
| return nil | ||
| } | ||
|
|
||
| func effectiveDefaultChatModelConfig( |
There was a problem hiding this comment.
Note [CRF-61] The load-bearing definition of "the org's usable default" (load GetEnabledChatModelConfigsByOrganization, return the IsDefault row, else no-rows) now exists twice: effectiveDefaultChatModelConfig here and chatstate.Tx.resolveQueuedMessageModelConfigID (transitions.go:319). (Netero)
Package boundary (
chatstatecannot importchatd) makes a direct shared helper awkward, but the pattern is now the load-bearing definition ... call it out before the third copy shows up.
A helper in a package both can import (database or chatstate) retires the copy.
🤖
ethanndickson
changed the title
ad400e1 to
419208d
Compare
419208d to
b346208
Compare
4453eb1 to
87c91ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87c91abb2f
ℹ️ 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".
| } | ||
|
|
||
| func TestMigration000574MCPServerConfigsOrganizationID(t *testing.T) { | ||
| func TestMigration000580ChatModelConfigOrganization(t *testing.T) { |
There was a problem hiding this comment.
Keep this new migration 580 test in addition to the existing TestMigration000574MCPServerConfigsOrganizationID and TestMigration000576MCPServerConfigACL tests rather than replacing them. This hunk removes both prior tests, so the suite no longer verifies migration 574's organization backfill and rollback cleanup or migration 576's ACL backfill and rollback behavior; regressions in those migration paths can now reach upgrade or rollback testing undetected.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
ethanndickson
changed the title
github-actions
Bot
added
the
release/breaking
87c91ab to
18c6eff
Compare
…#27956) Include the owning organization ID for each chat model configuration in telemetry snapshots. Depends on coder#27955 _This pull request description was generated by Coder Agents._
## Summary Scope chat model configurations and runtime model resolution to organizations. Each model configuration now belongs to exactly one organization, each organization can have its own default model, and Chatd only uses models that belong to the chat's organization. Depends on #27968. ## Problem Chat model configurations are currently deployment-wide even though chats belong to organizations. A default model, explicitly selected model, or configured model override can therefore reference the same model configuration from every organization. This prevents model catalogs and defaults from being isolated by organization and allows a chat to retain or select a model that its organization does not own. Existing model configurations also have no organization ownership or organization-scoped ACL data, and the database enforces a single live default across the entire deployment. ## Fix Add required organization ownership and ACL storage to chat model configurations. The migration assigns every existing configuration to the default organization, seeds organization-wide read access, and replaces the deployment-wide default constraint with a per-organization constraint. Scope Chatd lookup, admission, fallback, and queue-promotion behavior to the chat's organization: - New chats use the usable default model from their organization. - Explicit model selections from another organization are rejected. - Deployment or personal overrides that point to a foreign model are not used for the chat. - Historical cross-organization model references remain unchanged in the database, but runtime generation falls back to a usable local default. - Queue promotion resolves unavailable, disabled, missing, or foreign stored models to the usable local default and fails when no usable local default exists. The existing model configuration management and availability routes remain compatibility routes for the default organization in this layer. A later layer in the stack adds organization-scoped management routes and updates clients to use them. ## Breaking change This changes chat model behavior from deployment-wide to organization-scoped. Existing configurations are migrated only to the default organization; they are not copied into every organization. As a result: - A non-default organization no longer inherits the deployment's existing default model. - Creating a chat in an organization without a usable local default fails with `400: No default chat model config is configured.` - Explicitly selecting a configuration owned by another organization fails with `400: Model config does not belong to the specified organization.` - Existing chats whose stored model belongs to another organization can generate only if a usable local default is available; otherwise generation fails. Deployments using Agents in non-default organizations must configure a usable default chat model separately for each organization after the organization-scoped management layers in this stack are available. _This pull request description was generated by Coder Agents._
Include the owning organization ID for each chat model configuration in telemetry snapshots. Depends on #27955 _This pull request description was generated by Coder Agents._
2 participants
Summary
Scope chat model configurations and runtime model resolution to organizations. Each model configuration now belongs to exactly one organization, each organization can have its own default model, and Chatd only uses models that belong to the chat's organization.
Depends on #27968.
Problem
Chat model configurations are currently deployment-wide even though chats belong to organizations. A default model, explicitly selected model, or configured model override can therefore reference the same model configuration from every organization. This prevents model catalogs and defaults from being isolated by organization and allows a chat to retain or select a model that its organization does not own.
Existing model configurations also have no organization ownership or organization-scoped ACL data, and the database enforces a single live default across the entire deployment.
Fix
Add required organization ownership and ACL storage to chat model configurations. The migration assigns every existing configuration to the default organization, seeds organization-wide read access, and replaces the deployment-wide default constraint with a per-organization constraint.
Scope Chatd lookup, admission, fallback, and queue-promotion behavior to the chat's organization:
The existing model configuration management and availability routes remain compatibility routes for the default organization in this layer. A later layer in the stack adds organization-scoped management routes and updates clients to use them.
Breaking change
This changes chat model behavior from deployment-wide to organization-scoped. Existing configurations are migrated only to the default organization; they are not copied into every organization.
As a result:
400: No default chat model config is configured.400: Model config does not belong to the specified organization.Deployments using Agents in non-default organizations must configure a usable default chat model separately for each organization after the organization-scoped management layers in this stack are available.
This pull request description was generated by Coder Agents.