First-pass review (Netero only). This is a mechanical scan; the full review panel has not yet reviewed this PR.

The migration logic and provider-resolution rewrite are structurally sound, and the sequentialization of resolveChatModel (removing the errgroup) is a clean simplification now that the model config result feeds into the provider key lookup.

1 P1, 2 P3.

The P1 is the blocking item: 150+ new Go lines with zero test coverage on the new AI-provider path. Every existing test passes uuid.Nil, so the entire selectedAIProviderID != uuid.Nil branch, aiProviderConfig, and the AI provider lookups in resolveChatModel, resolveModelConfigAndNormalizedProvider, and resolveTitleGenerationModelOverride are untested. These are the paths that will be exercised for all users whose models are backed by AI providers.

The panel will review after these findings are addressed.

"163 new Go lines across chatd.go, subagent.go, and title_override.go; 99 new SQL lines in the migration; 0 new test lines. All 5 test changes are mechanical signature updates. The code-to-test ratio is effectively infinite." (Netero)

🤖 This review was automatically generated with Coder Agents.

Round 2 is blocked. DEREM-1 and DEREM-3 are addressed; thank you.

DEREM-2 (P3, duplicate GetAIProviderByID call) has no author response and no code change. The review panel cannot proceed until every open finding is addressed, acknowledged, or contested.

Please respond to DEREM-2: fix, acknowledge with reasoning, or contest with a technical argument.

🤖 This review was automatically generated with Coder Agents.

Round 3 is blocked (second consecutive block). DEREM-2 (P3, duplicate GetAIProviderByID call) remains without a response.

To unblock the panel review, please do one of:

1. Fix the duplicate fetch (pass the already-fetched database.AIProvider downstream).
2. Acknowledge with reasoning why the redundancy is acceptable.
3. Contest with a technical argument.

The /coder-agents-review re-triggers do not substitute for a response on the open finding.

🤖 This review was automatically generated with Coder Agents.

Panel review (19 reviewers). The migration design is solid and the sequentialization of resolveChatModel is a clean simplification. The precondition checks in the up migration are well-designed, failing loudly rather than producing duplicates.

1 P0, 6 P2, 5 P3, 2 Nit.

The P0 is a showstopper: the chatd RBAC subject has no ResourceAIProvider read permission. In production (where dbauthz wraps the store), every GetAIProviderByID and GetAIProviderKeysByProviderID call returns an authorization error. Tests pass because they use a raw database.Store without dbauthz. After migration 000499 sets ai_provider_id on all model configs, the entire AI provider resolution path is inoperative.

The P2 cluster centers on the title generation path, which resolves credentials from the legacy chat_providers table and ignores AI provider keys. Eight reviewers independently identified this. The closure at title_override.go:67 discards the AI provider ID parameter, so after key rotation, title generation authenticates with stale credentials.

"The admin has valid keys configured on the AI provider, but the title generation path never queries them." (Chopper)

"What happens when a BYOK-only user (no central key) tries to generate a title?" (Hisoka)

Nit [DEREM-17]: Commit 1a3338cf4 is labeled test(coderd/x/chatd) but includes the DEREM-3 production fix (Enabled check at title_override.go:85). Reverting a test() commit would silently reintroduce the bug. The fix should be a separate fix() commit or the commit type should be fix(). (Mafu-san)

🤖 This review was automatically generated with Coder Agents.

Round 5 is blocked. All 14 open findings from the panel review (R4) are silent: no author response on any thread, no code addressing them.

The most critical items awaiting response:

1. DEREM-5 (P0): chatd RBAC subject lacks ResourceAIProvider read. Every AI provider lookup fails in production under dbauthz. This needs a fix in dbauthz.go.
2. DEREM-6 (P2): Title generation uses legacy key resolution. 8 reviewers identified this independently.
3. DEREM-7 (P2): AI provider path skips modelConfig.Enabled check.

Commit 4407c234b refactors the resolution logic but does not address any of the posted findings. The /coder-agents-review re-triggers do not substitute for responses on open threads.

Please respond to or address the open findings to unblock the next review round.

🤖 This review was automatically generated with Coder Agents.

Round 6 is blocked (2nd consecutive block after panel). All 14 open findings remain silent.

The new commit (4407c234b, "reuse selected AI provider lookup") adds resolveUserProviderAPIKeysForProvider which eliminates the duplicate fetch in resolveChatModel. This is a good refactor related to the closed DEREM-2 but does not address any open finding.

The P0 (DEREM-5, RBAC) is still unpatched. The chatd subject in dbauthz.go still lacks ResourceAIProvider read permission. This PR will break all AI provider resolution in production.

To unblock: respond to or address any of the 14 open findings. At minimum, the P0 needs a fix before this can ship.

🤖 This review was automatically generated with Coder Agents.

Round 7 is blocked (3rd consecutive block after panel). All 14 open findings remain silent. CI is green, but the findings are unaddressed.

This is the 3rd round with zero response on any finding thread. The /coder-agents-review re-triggers cannot advance the review without author engagement on the open threads.

The P0 (DEREM-5) remains the critical blocker: subjectChatd in dbauthz.go still lacks ResourceAIProvider read permission. This PR will fail in production for every AI-provider-backed model config.

🤖 This review was automatically generated with Coder Agents.

Re-review panel (8 reviewers). Thank you for responding to all findings.

DEREM-10 (P2, hardcoded CentralAPIKeyEnabled) is closed. The panel unanimously accepts the defense (7/7). Kurapika, who originally raised it, withdraws: the Enabled flag is the meaningful security control and is preserved. The per-provider key policy removal is a deliberate product decision, not an oversight.

DEREM-9 (P2, sql.ErrNoRows for disabled provider) is closed. The panel accepts the acknowledgment (6/6). Behavior is correct for soft-failure override semantics.

The "fixed in the stack" claims (DEREM-5, 6, 7, 8) are unanimously unverifiable. The head SHA is unchanged from R7; no fix is present in this PR's diff. This PR's code still has:

• No ResourceAIProvider permission on the chatd RBAC subject (DEREM-5, P0)
• Title generation resolving keys via uuid.Nil / legacy path (DEREM-6, P2)
• No modelConfig.Enabled check in the AI provider branch (DEREM-7, P2)
• No happy-path test for enabled AI provider (DEREM-8, P2)

If these fixes are in parent PRs in the stack, this PR is safe as long as the stack merges in order. This needs a human decision: a human reviewer should verify the parent PRs contain these fixes before approving merge.

All acknowledged findings (DEREM-4, 11, 12, 13, 14, 15, 16, 17) have reasonable author responses and are closed by the panel.

"The tests mask the RBAC gap because they bypass dbauthz entirely. Tests pass when production code path would fail with authorization error." (Hisoka, Chopper)

🤖 This review was automatically generated with Coder Agents.

Round 9. Head and base SHAs changed, CI green. Orchestrator verified the code directly: the 4 remaining "in stack" findings are still present.

• DEREM-5 (P0): subjectChatd in dbauthz.go still lists only ResourceChat, ResourceWorkspace, ResourceDeploymentConfig, ResourceUser. No ResourceAIProvider.
• DEREM-6 (P2): Lines 2941 and 3005 still call resolveUserProviderAPIKeys with uuid.Nil.
• DEREM-7 (P2): subagent.go:521-533 still checks only provider.Enabled, not modelConfig.Enabled.
• DEREM-8 (P2): No new happy-path test.

All other findings are closed (panel-accepted or panel-closed contest). No new findings from Netero or the panel in this round.

These 4 findings need a human decision: verify the parent PRs in the stack contain the fixes before approving this PR for merge. If this PR merges before or without the parent fixes, the P0 (RBAC) will break all AI-provider-backed chat in production.

🤖 This review was automatically generated with Coder Agents.

Round 10. Same head SHA as R9, no new code, no new author responses.

The 5 remaining findings (DEREM-5 P0, DEREM-6/7/8 P2, DEREM-15 P3) have been evaluated by the panel twice (R4, R8). The code is unchanged and the author's "fixed in the stack" defense is unchanged. Re-running the panel on identical inputs will produce the same verdict: unverifiable from this PR, needs human decision.

The automated review has reached its limit on these findings. A human reviewer should:

1. Verify the parent PRs in the stack contain fixes for DEREM-5 (RBAC), DEREM-6 (title key resolution), DEREM-7 (modelConfig.Enabled), and DEREM-8 (happy-path test).
2. Confirm merge order ensures parent fixes land before this PR.

All other findings are closed.

🤖 This review was automatically generated with Coder Agents.

Round 11. New head+base SHAs but all 5 remaining findings are unchanged in the code. No new author responses.

This is the 3rd consecutive round with identical state. The automated review cannot make further progress. The remaining findings (DEREM-5 P0, DEREM-6/7/8 P2, DEREM-15 P3) have been raised (R4), re-evaluated (R8), and verified (R9, R10, R11). The author's "fixed in the stack" defense has been noted. A human reviewer must verify the stack.

🤖 This review was automatically generated with Coder Agents.

Round 12. The author provided the missing stack context:

• Parent PR: #25413
• Stack order: 25413 → 25414 (parent merges first)
• DEREM-5 (P0): covered by PR #25413, commit ff67953be3
• DEREM-6, 7, 8, 15: covered in parent commits within the stacked diff

The author agrees this needs human reviewer confirmation because the automated review evaluates the isolated PR diff.

The automated review concurs. These findings are real in this PR's isolated diff but are expected to be resolved by the stack merge order. A human reviewer should verify PR #25413 contains the RBAC fix (DEREM-5) and the other fixes before approving.

All other findings (13 of 18) are closed.

🤖 This review was automatically generated with Coder Agents.

Round 13. New head+base SHAs but all 5 findings remain unchanged in the code. No new author responses.

This review has been in steady state since R9 (5 rounds). The remaining findings (DEREM-5 P0, DEREM-6/7/8 P2, DEREM-15 P3) are all "fixed in parent PR #25413" per the author. The automated review cannot verify cross-PR fixes and the author agrees human confirmation is needed.

A human reviewer should check PR #25413 and confirm merge order before approving. No further automated rounds will produce new information unless the code in this PR changes or the author provides new responses.

🤖 This review was automatically generated with Coder Agents.

Round 14. Steady state (6th round, since R9). All 5 findings remain in the code, no new author responses.

The automated review has no new information to add. The remaining findings (DEREM-5 P0, DEREM-6/7/8 P2, DEREM-15 P3) are claimed fixed in parent PR #25413. A human reviewer must verify the stack before approving.

🤖 This review was automatically generated with Coder Agents.

Round 15. The author states PR #25413 passed review with zero findings and its fixes are in this PR's base.

However, the orchestrator verified that the 5 remaining findings are introduced by this PR's own diff, not inherited from the base:

• uuid.Nil calls at chatd.go:2941 and 3005 appear as + lines in the diff (DEREM-6)
• The AiProviderID.Valid branch without modelConfig.Enabled check appears as + lines (DEREM-7)
• subjectChatd in dbauthz.go has no ResourceAIProvider entry (DEREM-5)

The parent PR (#25413) cannot fix code that this PR introduces. These findings require changes in this PR or a follow-up PR in the stack.

This is round 15, with the same conclusion since R9. The automated review is complete. A human reviewer should evaluate whether these 5 findings need to be addressed in this PR, a follow-up, or are acceptable as-is.

🤖 This review was automatically generated with Coder Agents.

Round 16. New head SHA with code changes.

DEREM-5 (P0) is verified fixed. rbac.ResourceAIProvider.Type: {policy.ActionRead} is now in the chatd subject's site permissions. The showstopper is resolved.

The remaining 4 findings are still present in the code:

• DEREM-6 (P2): Title gen closure (title_override.go:69) still discards the AI provider ID and returns pre-resolved legacy keys.
• DEREM-7 (P2): resolveModelConfigAndNormalizedProvider AI branch still has no modelConfig.Enabled check.
• DEREM-8 (P2): Only TestResolveChatModel_AIProviderDisabled exists; no happy-path test.
• DEREM-15 (P3): subagent.go:524 still returns bare error.

With the P0 resolved, the critical production blocker is gone. The remaining P2s are correctness issues that will manifest after key rotation (DEREM-6) or when admins disable model configs (DEREM-7). A human reviewer should decide whether these are acceptable for this PR or need follow-up.

🤖 This review was automatically generated with Coder Agents.

Round 17. Same head SHA, no new code or author responses.

4 findings remain (DEREM-6/7/8 P2, DEREM-15 P3). The P0 is fixed. These findings have been stable since R4 (raised), evaluated by the panel twice (R4, R8), and verified by the orchestrator across 8 rounds. The automated review has no new information to add.

A human reviewer should decide whether DEREM-6 (title gen stale keys after rotation), DEREM-7 (disabled model config ignored), DEREM-8 (no happy-path test), and DEREM-15 (bare error) are acceptable for this PR or need follow-up.

🤖 This review was automatically generated with Coder Agents.

Re-review panel (5 reviewers). Thank you for responding to all findings.

DEREM-6 (P2), DEREM-8 (P2), and DEREM-15 (P3) are closed. The panel unanimously accepts the author's resolutions.

DEREM-7 (P2) is re-raised. The panel unanimously rejects the partial fix (5/5). The !dbConfig.Enabled guard in resolveChatModel (chatd.go:8027) covers the main chat path, but the three override paths through resolveModelConfigAndNormalizedProvider (subagent overrides, personal overrides, title gen overrides) remain unguarded. Post-migration, ai_provider_id is set on all model configs, so the legacy branch with its modelConfig.Enabled check is unreachable for migrated data.

Fix: add if !modelConfig.Enabled { return ..., sql.ErrNoRows } before the AiProviderID.Valid branch in resolveModelConfigAndNormalizedProvider (subagent.go:521). This is a one-line addition.

17 of 18 findings closed. DEREM-7 is the sole remaining item.

"The migration sets ai_provider_id on all rows, so the legacy branch (which had the check) is now unreachable for all migrated data. Makes the gap worse." (Pariston)

🤖 This review was automatically generated with Coder Agents.

All 18 findings closed.

DEREM-7 (P2, the last open finding) is verified fixed in commit 08c5726e: !modelConfig.Enabled guard added before the AiProviderID.Valid branch in resolveModelConfigAndNormalizedProvider. This is the exact one-line fix the panel requested.

Review summary across 19 rounds:

• 18 findings raised (1 P0, 7 P2, 6 P3, 2 Nit, 2 Note)
• 7 fixed by author, 2 contested and panel-accepted, 9 acknowledged and panel-accepted
• Key catches: RBAC permission gap (P0), title generation stale keys (P2), missing modelConfig.Enabled check (P2), migration policy change (P2)

🤖 This review was automatically generated with Coder Agents.

Round 20. Same head SHA, CI green, all 18 findings closed. No open items remain.

This review is complete. No further automated rounds are needed unless new code is pushed.

🤖 This review was automatically generated with Coder Agents.