feat: add per-turn reasoning effort selection to chat API - #26975

P3 [CRF-1] No test exercises the edit-message reasoning effort path. The validation logic here (lines 3338-3347) and the ReasoningEffort passthrough to EditMessageOptions (line 3354) are untested. The create and send paths both have validation and persistence tests. The edit path's override/preservation behavior in transitions.go:573-576 (use original effort when override is invalid, use override when valid) has zero direct coverage. (Netero)

🤖

Fixed in 847f0e7.

Generated with Coder Agents.

Verified fixed in 847f0e72a6. TestPatchChatMessage/ReasoningEffort (with PreservesByDefault and Overrides subtests) and TestEditMessageReasoningEffort now cover the edit-message reasoning effort path at both the HTTP and chatd layers.

🤖

P3 [CRF-3] No test verifies that reasoning effort survives the queued-message roundtrip. TestSendMessageQueuesEffectiveModelConfigID proves model config ID is preserved when a message is queued and later promoted. The PR adds ReasoningEffort to insertQueuedMessage and messageFromQueuedRow (transitions.go:231, 244), but no test queues a message carrying a reasoning effort and verifies the promoted message still carries it.

A bug in either the insert or the promotion would silently drop the user's effort selection. Sketch: clone TestSendMessageQueuesEffectiveModelConfigID, add ReasoningEffort: ptr.Ref("high") to the request, assert the queued message carries it, drain the queue, and assert the promoted chat message does too. (Bisky)

🤖

Fixed in 847f0e7.

Generated with Coder Agents.

Verified fixed in 847f0e72a6. TestSendMessageQueuesReasoningEffort verifies the queued row carries the effort and the chat's LastReasoningEffort stays unset. TestPromoteQueuedPreservesReasoningEffort completes the loop: after promotion, both the promoted message and the chat carry the effort.

🤖

P3 [CRF-4] The three validation sites (lines 1241, 3176, 3344) return "Invalid reasoning_effort value." without including the rejected value. An API consumer debugging a 400 sees their value was wrong but not what value was rejected, forcing correlation with their own request logs. A message like fmt.Sprintf("Invalid reasoning_effort value: %q.", *effort) names the cause.

This follows the existing pattern (plan_mode, busy_behavior do the same), so it's consistent. But the class is wrong: every validation error that rejects user-supplied input should echo it back. (Leorio)

🤖

Not changed. Treating this as an optional nit for this round.

Generated with Coder Agents.

Panel closed (4/4 accept). Mafuuu, Pariston, Meruem, and Razor independently verified the convention is consistent across sibling validators (plan_mode, busy_behavior). The consequence (client must check own request body) is acceptable for a small documented enum.

🤖

I think CRF-4 is valid and we should avoid introducing new bad practices into the code-base.

Updated the validation response helper so the detail includes the rejected reasoning_effort value.

Generated with Coder Agents.

Nit [CRF-6] reasoningEffortRank("") always returns (-1, false) since "" is not on the scale. Both rank and ok are unconditionally overwritten before use (either by the effective != nil branch or by the fallback to config.Default). The call is a roundabout variable declaration. Clearer:

effective := requested
var rank int
var ok bool
if effective != nil {
    rank, ok = reasoningEffortRank(*effective)
}

(Hisoka, Mafuuu, Gon, Chopper, Meruem, Melody, Kite, Knov, Razor, Pariston)

🤖

Fixed in 847f0e7.

Generated with Coder Agents.

Verified fixed in 847f0e72a6. ResolveReasoningEffort rewritten with explicit var rank int; var ok bool declarations. Dead initialization eliminated.

🤖

Nit [CRF-7] "Must be one of none, minimal, low, medium, high, xhigh, max." is hardcoded in three places (lines 1242, 3177, 3345). The canonical scale lives in chatprovider.reasoningEffortOrder (unexported). If the scale changes, IsValidReasoningEffort would correctly reject old values but these error messages would list a stale set. Exporting the scale or adding a helper like chatprovider.ReasoningEffortListString() would keep message and validation in sync. (Leorio, Chopper, Razor)

🤖

Not changed. Treating this as an optional nit for this round.

Generated with Coder Agents.

If moving to db enums, this becomes a simple join operation. In lieu of that, I think CRF-7 is still valid and should be fixed.

Replaced the hardcoded allowed-value detail strings with a canonical joined value list from codersdk.ChatModelReasoningEffortValues().

Generated with Coder Agents.

Nit [CRF-8] chatReasoningEffort uses a chat prefix that the sibling variable modelConfigID (line 1122) does not. postChatMessages (line 3173) and patchChatMessage (line 3341) use reasoningEffort and editReasoningEffort matching their modelConfigID / editModelConfigID siblings. Rename to reasoningEffort for consistency. (Gon)

🤖

Not changed. Treating this as an optional nit for this round.

Generated with Coder Agents.

I think consistency is worth it tbh.

Renamed chatReasoningEffort to reasoningEffort for consistency with the sibling modelConfigID naming.

Generated with Coder Agents.

Nit [CRF-9] This comment restates ResolveReasoningEffort's own doc comment ("The requested per-turn value wins over the config's default, and the result is clamped to the config's max on the global scale. Returns nil when the model config has no reasoning effort configured"). If the resolution logic changes, two places need updating. The code on lines 538-543 is straightforward: read chat.LastReasoningEffort, pass it to ResolveReasoningEffort. Delete the comment. (Gon)

🤖

Already resolved.

Generated with Coder Agents.

Confirmed still resolved. No code change needed for this thread.

Generated with Coder Agents.

P4 [CRF-10] No test row covers requested with a valid config that has a default but no max. The code path where config.Max == nil and a valid effective value exists (lines 77-85 skipped, line 86 returns effective) is only covered indirectly. A row like {name: "NoMaxReturnsRequested", requested: ptr.Ref("high"), config: effortConfig("low", ""), want: ptr.Ref("high")} would cover the unclamped return. (Bisky)

🤖

Fixed in 847f0e7.

Generated with Coder Agents.

Verified fixed in 847f0e72a6. RequestedWinsWithoutMax row added covering the unclamped return path.

🤖

P3 [CRF-11] The old suite had a DefaultClampedToMax row proving that when config.Default exceeds config.Max, the result is clamped to max. That row was removed when the requested parameter was added, and no new row covers the equivalent: requested is nil, default is above max, result should be max.

The existing RequestedClampedToMax tests clamping when the source is requested, and InvalidRequestedFallsBackToDefault tests fallback from invalid requested to default, but neither covers the composition: fallback to default, then clamp to max. The clamping logic operates on the same rank variable regardless of origin, so this is unlikely to break independently, but the guarantee existed before and was dropped.

Fix: add the row back.

{name: "DefaultClampedToMax", config: effortConfig("xhigh", "medium"), want: ptr.Ref("medium")},

(Bisky)

🤖

Restored the missing DefaultClampedToMax row in the amended PR2 commit.

Generated with Coder Agents.

Verified fixed in 7b4f4eefe6. DefaultClampedToMax row restored: {config: effortConfig("xhigh", "medium"), want: ptr.Ref("medium")} covers the fallback-to-default-then-clamp-to-max path.

🤖

Should reasoning_effort just be an enum instead?

Implemented as a Postgres enum (chat_reasoning_effort) and regenerated the database types/queries.

Generated with Coder Agents.

We can skip ptr.Ref and just use new("string") instead. Note, db enum as mentioned previously would let us avoid these strings.

Updated the resolver tests to use new(...) for typed reasoning effort constants, and used new(string(...)) where converting the database enum back to *string is needed.

Generated with Coder Agents.

Drive-by rename?

Already resolved, that drive-by rename was not part of this branch after the rebuild.

Generated with Coder Agents.

I think consistency is worth it tbh.

I think CRF-4 is valid and we should avoid introducing new bad practices into the code-base.

If moving to db enums, this becomes a simple join operation. In lieu of that, I think CRF-7 is still valid and should be fixed.

A bit of testing wording bleeding through here. This details is supposed to be helpful to humans hence the suggestion.

Coder Agents generated.

Updated the wording to Invalid value %q, must be one of %s and updated the tests to match.