omarluq/gpt-5.6#195
Conversation
📝 WalkthroughWalkthroughGPT-5.6 Sol, Terra, and Luna models were added to discovery and built-in provider catalogs, with updated costs, context limits, defaults, reasoning mappings, payload handling, terminal cycling, and test coverage. ChangesGPT-5.6 model support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
==========================================
+ Coverage 83.51% 83.59% +0.08%
==========================================
Files 287 287
Lines 23204 23363 +159
==========================================
+ Hits 19379 19531 +152
- Misses 2658 2662 +4
- Partials 1167 1170 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/session_settings_actions.go (1)
85-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate
ThinkingMaxin the thinking cycle.cycleThinking()always includesThinkingMax, but some models never map that level. On those models,reasoningEffort()falls back to the raw"max"string, so a user can end up sending an unsupportedreasoning_effortvalue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/session_settings_actions.go` around lines 85 - 94, The cycleThinking method must not always expose ThinkingMax because some models do not support it. Gate the ThinkingMax entry using the model’s supported thinking-level capability, ensuring unsupported models cycle only through valid levels and never produce a raw "max" reasoning_effort value.
🧹 Nitpick comments (3)
internal/provider/openai_chat_payload_internal_test.go (1)
30-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"max maps directly" doesn't test the direct/fallback path.
Both subtests call
setTestThinkingMap, soreasoningEffortalways takes the mapped branch (payload builder conditionally sets payload["reasoning_effort"] using reasoningEffort(request), which maps request thinking level through request.Model.ThinkingLevelMap (or falls back to the raw thinking level) when reasoning is enabled). SincemappedLevel == level == thinkingMaxfor the "max maps directly" case, this doesn't actually verify the no-map-entry fallback that most GPT-5.6 models will use for a level with no explicit map entry.♻️ Suggested addition to cover the fallback path
{ name: "max maps directly", level: thinkingMax, mappedLevel: thinkingMax, want: thinkingMax, }, + { + name: "max falls back without a map entry", + level: thinkingMax, + want: thinkingMax, + },Then skip
setTestThinkingMapwhenmappedLevel == ""in the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provider/openai_chat_payload_internal_test.go` around lines 30 - 66, Update TestOpenAIChatPayloadMapsReasoningEffort so the direct/fallback case omits the thinking-level map entry: only call setTestThinkingMap when mappedLevel is non-empty, and add a max case with an empty mappedLevel whose expected value remains thinkingMax. Keep the existing mapped xhigh case to verify explicit mapping.internal/model/providers_test.go (1)
139-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a struct-literal table instead of an 8-parameter constructor.
gpt56BuiltInTestCasetakes 8 positional parameters (5 consecutivestrings), which SonarCloud flags and which risks silent parameter-order mistakes in test data. Buildinggpt56BuiltInTest{...}literals directly with named fields ingpt56BuiltInTests()would remove the constructor and the ordering risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/providers_test.go` around lines 139 - 159, Replace the eight-parameter gpt56BuiltInTestCase constructor with direct gpt56BuiltInTest struct literals in gpt56BuiltInTests(), using named fields for every value. Remove the helper and update all call sites to eliminate positional string parameters and ordering risks.Source: Linters/SAST tools
internal/model/discovery.go (1)
297-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared constant for the repeated
"gpt-5.6"literal.
"gpt-5.6"is hardcoded three times (Line 307, Line 658, Line 662). SonarCloud flags this as a duplicated literal; a shared constant would also protect against typos across the three call sites.♻️ Proposed fix
+const gpt56Generic = "gpt-5.6" + func openAIUnsupportedDiscoveredModel(provider, fallbackModelID string, definition *discoveryModel) bool { if provider != providerOpenAI { return false } modelID := fallbackModelID if definition.ID != "" { modelID = definition.ID } - return modelID == "gpt-5.6" + return modelID == gpt56Generic }func openAISupportsXHigh(modelID string) bool { return strings.Contains(modelID, gpt52) || strings.Contains(modelID, "gpt-5.3") || strings.Contains(modelID, "gpt-5.4") || strings.Contains(modelID, gpt55) || - strings.Contains(modelID, "gpt-5.6") + strings.Contains(modelID, gpt56Generic) } func openAISupportsMax(modelID string) bool { - return strings.Contains(modelID, "gpt-5.6") + return strings.Contains(modelID, gpt56Generic) }Also applies to: 653-666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/discovery.go` around lines 297 - 309, Extract a shared constant for the repeated "gpt-5.6" model identifier, placing it near the other model constants in internal/model/discovery.go. Update openAIUnsupportedDiscoveredModel and the related call sites around the model handling logic to reference this constant instead of hardcoding the literal.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/terminal/session_settings_actions.go`:
- Around line 85-94: The cycleThinking method must not always expose ThinkingMax
because some models do not support it. Gate the ThinkingMax entry using the
model’s supported thinking-level capability, ensuring unsupported models cycle
only through valid levels and never produce a raw "max" reasoning_effort value.
---
Nitpick comments:
In `@internal/model/discovery.go`:
- Around line 297-309: Extract a shared constant for the repeated "gpt-5.6"
model identifier, placing it near the other model constants in
internal/model/discovery.go. Update openAIUnsupportedDiscoveredModel and the
related call sites around the model handling logic to reference this constant
instead of hardcoding the literal.
In `@internal/model/providers_test.go`:
- Around line 139-159: Replace the eight-parameter gpt56BuiltInTestCase
constructor with direct gpt56BuiltInTest struct literals in gpt56BuiltInTests(),
using named fields for every value. Remove the helper and update all call sites
to eliminate positional string parameters and ordering risks.
In `@internal/provider/openai_chat_payload_internal_test.go`:
- Around line 30-66: Update TestOpenAIChatPayloadMapsReasoningEffort so the
direct/fallback case omits the thinking-level map entry: only call
setTestThinkingMap when mappedLevel is non-empty, and add a max case with an
empty mappedLevel whose expected value remains thinkingMax. Keep the existing
mapped xhigh case to verify explicit mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73c28c0f-be1b-48b2-9ce6-e3a9e8e334e6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
config.example.yamlinternal/config/loader.gointernal/model/discovery.gointernal/model/discovery_behavior_internal_test.gointernal/model/discovery_internal_test.gointernal/model/providers.gointernal/model/providers_test.gointernal/model/types.gointernal/provider/anthropic_tools_internal_test.gointernal/provider/client.gointernal/provider/errors_internal_test.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses_internal_test.gointernal/provider/request_shape_internal_test.gointernal/provider/test_helpers_internal_test.gointernal/provider/tool_loop_internal_test.gointernal/terminal/async_events_internal_test.gointernal/terminal/compact_commands_internal_test.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render_composer.gointernal/terminal/session_setting_actions_internal_test.gointernal/terminal/session_settings_actions.go
|
1 participant
No description provided.