feat: add configurable reasoning effort to Coder agents - #26974

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

The reasoning effort refactor is well structured: clean separation of the new resolution logic into its own module, thorough migration with fixtures, and good test coverage (~50% density). The concern decomposition tracks a single feature across the stack with tight coupling, so Law says no split.

Severity count: 1 P2, 2 P3, 1 Note.

The P2 is a documentation/implementation mismatch worth fixing before the panel reviews. The panel will review after these findings are addressed.

"The comment makes a factual claim about behavior that does not exist. Either implement mirroring or fix the comment." (Netero)

🤖 This review was automatically generated with Coder Agents.

Panel review (round 2, 19 reviewers). R1 findings (CRF-1 through CRF-3) all verified fixed in d39150f. CRF-4 acknowledged as follow-up scaffolding.

The reasoning effort refactor is well-structured. The resolution pipeline (validate at API boundary, resolve with clamping at generation time, provider-specific injection at the last mile) is cleanly layered with single responsibilities. The migration is solid: nullable columns avoid rewrites, lower(trim(...)) normalization with NULLIF and IN-clause whitelisting handles edge cases, and the test fixture seeds real-world variations. Test density is 53.4%, covering validation, resolution, and the full generation chain.

Severity count: 1 P2, 5 P3, 1 P4, 3 Nit, 1 Note.

The P2 is a widened perimeter: the old per-provider validation was removed, so admins can now save global-scale effort values that individual providers reject at runtime, breaking all chats using that model. Seven reviewers flagged this independently.

"The old code had per-provider normalization... This PR removes that layer without adding a replacement." (Mafuuu)

"The config API advertises all seven global-scale values, the resolution accepts them, and the provider API rejects them. The gap between 'the API accepts it' and 'the runtime uses it' is the finding." (Razor)

🤖 This review was automatically generated with Coder Agents.

Panel review (round 3, 9 reviewers). R2 fixes (CRF-6, 7, 9, 10, 11, 13, 14, 15) all verified correct and complete.

Contested findings resolved by panel vote:

• CRF-5 (P2, validation gap): closed 4/5. The design trades upfront strictness for flexibility. Runtime validation is the authoritative source, and per-provider sets would require manual maintenance that could drift.
• CRF-8 (P4, Google missing): closed 5/5. Not a regression; Google uses a different mechanism.
• CRF-12 (P3, "Xhigh" label): closed 3/5. Deferred to the downstream slider PR.

Severity count (new): 1 P2, 1 P3, 2 Nit, 1 Note.

The P2 is a missed call site: the advisor model setup builds provider options but never applies reasoning effort, unlike the generation and title-generation paths.

Note: the PR description still contains two inaccurate claims: (1) "values must be in the provider's runtime-supported set" (validation checks global scale only, as CRF-5 confirmed), and (2) "a single value mirrors into the other" (validation requires both, as CRF-1 established). Please update the description to match the actual behavior.

"Two of the three call sites were updated; this one was missed." (Knov)

coderd/x/chatd/chatd.go:401

P2 [CRF-16] Advisor model setup drops reasoning effort after the refactoring.

ProviderOptionsFromChatModelConfig builds provider options for the advisor model here, but ApplyReasoningEffort is never called. The generation path (generation_preparer.go:534-537) and title generation path (quickgen.go:359-360 via titleGenerationProviderOptions) were both updated to call ApplyReasoningEffort after ProviderOptionsFromChatModelConfig. This call site was missed.

The advisorCallConfig carries the new ReasoningEffort field (from the fallback config or override config's migrated options). The effort is present in the config but never injected into provider options, so the advisor model operates without its configured reasoning effort.

Fix:

providerOptions := chatprovider.ProviderOptionsFromChatModelConfig(
    advisorModel,
    advisorCallConfig.ProviderOptions,
)
providerOptions = chatprovider.ApplyReasoningEffort(
    advisorModel,
    providerOptions,
    chatprovider.ResolveReasoningEffort(advisorCallConfig.ReasoningEffort),
)

(Knov)

🤖

🤖 This review was automatically generated with Coder Agents.

All findings resolved across 4 rounds (21 findings total: 16 fixed, 3 closed by panel vote, 1 acknowledged, 1 minor nit below).

R4 fixes verified: CRF-16 (advisor model now applies reasoning effort, with dedicated test), CRF-17 (OpenAI preserve tests added for both Responses and legacy API shapes), CRF-18 (fixture comment corrected), CRF-19 (doc comment added), CRF-20 (exported mutable var converted to function returning clone). PR description updated to match actual behavior.

Test density: 57.8%. Netero clean across all 8 categories. The pipeline (validate at API boundary, resolve with clamping, provider-specific injection) is cleanly layered. Migration handles edge cases well. The TestChatModelReasoningEffortConfigEnumTags test mechanically prevents enum drift.

🤖 This review was automatically generated with Coder Agents.

Writing critique of this PR's title, description, and code comments, produced with the writing-critique method: each unit checked against clear-writing guides, every claim verified against the diff.

Ten findings. Inline suggestions are attached for the three code-comment findings (CF1-CF3) and can be applied directly. The title and description findings (F1-F7) are below, because they are not part of the diff and cannot carry inline suggestions.

Three findings need a maintainer fact: F2, F7, and CF1.

Title and description

F1 - title contradicts the body on the old state. The title calls the old state "per-provider"; para 1 calls it a "fixed per-model setting stored at provider-specific paths." The value was one-per-model-config keyed by provider, not shared per provider, so "per-provider" is wider than the evidence. Proposed title:

feat: replace fixed reasoning effort with a per-model default and max

F2 - Validation bullet asserts a behavior not in this diff (needs a fact). "Updates fail when the config's provider has been soft-deleted" has no supporting change anywhere in the diff; the only validation added is validateChatModelReasoningEffortConfig. If it is not introduced here, drop the sentence:

-- **Validation**: `default` and `max` must both be present when reasoning effort is configured, each value must be in the global scale `none < minimal < low < medium < high < xhigh < max`, and `default <= max`. Updates fail when the config's provider has been soft-deleted.
+- **Validation**: `default` and `max` must both be present when reasoning effort is configured, each value must be in the global scale `none < minimal < low < medium < high < xhigh < max`, and `default <= max`.

If it is introduced or changed here, keep it but cite where.

F3 - "site" bullet contradicts itself and carries jargon. "compile fallout only" says nothing behavioral changed, then the same sentence reports new default <= max validation. It also uses the coined "compile fallout" / "known-model catalog" and UI jargon "selects".

-- **site**: compile fallout only; the admin form schema loses the old per-provider effort selects, known-model catalog defaults populate the new pair, and form logic validates `default <= max`. New admin UI placement and the chat slider land in follow-ups.
+- **site**: mostly mechanical changes forced by the removed fields. The admin form no longer shows the old per-provider effort dropdowns, and default values for recognized models fill the new `default` and `max` fields. The one behavior change is form validation that rejects a `default` greater than `max`. New admin UI placement and the chat slider land in follow-ups.

F4 - Stack line hides a branching tree behind linear arrows. Verified: #26975 and #26976 both branch off this PR; #26977 off #26975; #27061 off #26977. The -> / (on X) notation cannot express that.

-Stack: this PR → per-turn API (#26975) and models page UI (#26976, on this PR); chat slider (#26977, on #26975) → model override reasoning effort (#27061, on #26977).
+Stack (each PR is based on the one named):
+
+- this PR (on `main`)
+- #26975 per-turn API (on this PR)
+- #26976 models page UI (on this PR)
+- #26977 chat slider (on #26975)
+- #27061 model override reasoning effort (on #26977)

F5 + F7 - "behavior-preserving" is undefined-term jargon and omits its scope (F7 needs a fact). Para 2 relies on "global scale" before it is defined (it is defined only in the Validation bullet), and asserts "behavior-preserving" without qualification. The migration filters with AND legacy.effort IN ('none', ..., 'max') and so drops out-of-scale legacy values; the testdata's GPT-5.1 (Invalid Legacy Effort) = "extreme" becomes no effort, where the old path passed it straight through. Combined fix:

-This PR is behavior-preserving: generation resolves the configured default (clamped to max on the global scale), which matches the old fixed value after migration. Per-turn user selection lands in the follow-up PR.
+This PR is behavior-preserving for efforts that were already a recognized value: generation resolves the configured default, clamped to the configured max, which matches the old fixed value after migration. A stored value the new schema does not recognize (for example a manually set `"extreme"`) is dropped by the migration rather than carried over, so that config resolves to no effort. Per-turn user selection lands in the follow-up PR.

If out-of-scale rows cannot exist because the old write path enforced each provider's enum, the second sentence can shrink to a one-clause scope note.

F6 - "fantasy provider options" is an undefined library name. The chatd bullet says the value is injected into the "fantasy provider options"; the ARCHITECTURE.md section this same PR adds calls them the "provider-native options". Use the term the PR's own docs use.

-- **chatd**: new `chatprovider.ResolveReasoningEffort` clamps the configured default to the configured max on the global scale before `ApplyReasoningEffort` injects it into the fantasy provider options (azure routes via openai, bedrock via anthropic).
+- **chatd**: new `chatprovider.ResolveReasoningEffort` clamps the configured default to the configured max before `ApplyReasoningEffort` injects it into the provider-native options (azure routes via openai, bedrock via anthropic).

The three code-comment findings are attached inline as applicable suggestions.

🤖 This review was automatically generated with Coder Agents.