fix(agent-session): π propagate reasoning_content_constrained to all provider types - #160
Conversation
β¦provider types Previously, the `reasoning_content_constrained` flag was only applied to openai-compatible providers, causing HTTP 400 errors when using providers like DeepSeek V4 Pro via ZenMux that require `reasoning_content` on every assistant message when thinking is enabled. Now the flag is applied to all provider types, ensuring compatibility with non-openai-compatible providers that enforce this constraint.
AI Code Review SummaryPR: #160 (fix(agent-session): π propagate reasoning_content_constrained to all provider types) Overall AssessmentDetected 3 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
| // reasoning_content when thinking is enabled, and the API rejects | ||
| // requests that omit it with HTTP 400. | ||
| if let Some(true) = role.reasoning_content_constrained { | ||
| let mut compat = maybe_compat.unwrap_or_default(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // reasoning_content when thinking is enabled, and the API rejects | ||
| // requests that omit it with HTTP 400. | ||
| if let Some(true) = role.reasoning_content_constrained { | ||
| let mut compat = maybe_compat.unwrap_or_default(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
β¦atible providers
| let db_path = temp_dir.path().join("test.db"); | ||
| let pool = init_database(&db_path).await.expect("database"); | ||
|
|
||
| // Use a provider type that is NOT "openai-compatible" β this is the |
There was a problem hiding this comment.
[MEDIUM] Test only covers provider type with no default compat; misses case where default exists
The new test only covers the case where default_openai_compatible_compat returns None. There is no test confirming behavior when maybe_compat is Some(compat) and constrained is true β the current code will discard those defaults, but this edge case is not verified.
Suggestion: Add a test for a provider type that triggers default_openai_compatible_compat to return Some(compat) with non-constrained settings, then set constrained=true and assert that the resulting compat retains only reasoning_content_constrained (if that is intended behavior).
Risk: Undetected future mistake if default_openai_compatible_compat changes for a non-openai-compatible provider and the constrained path loses needed defaults.
Confidence: 0.85
|
|
||
| // For Some(false) or None, no compat should be applied at all | ||
| // (unlike openai-compatible providers that always get compat). | ||
| assert!( |
There was a problem hiding this comment.
[LOW] New test lacks a check that the compat struct is not applied for non-constrained non-openai-compatible roles
The test could explicitly assert that the resolved model struct has no compat object when constraints are unset or false, beyond the is_none() checks.
Suggestion: Add a direct assertion like assert_eq!(unconstrained.model.compat, None) immediately after the is_none() assertions for clarity.
Risk: Low risk β current assertions are sufficient but explicit checks would improve test clarity.
Confidence: 0.85
Summary
reasoning_content_constrainedflag only being applied to openai-compatible providersreasoning_contentwhen thinking is enabled, otherwise API returns HTTP 400Changes
reasoning_content_constrainedis applied to all provider types, not just those with an explicitOpenAICompatibleCompatTest Plan
reasoning_contentis included in requests for constrained providersπ€ Generated with TiyCode