fix: default api_type for specific github copilot models#2376
fix: default api_type for specific github copilot models#2376joshbarrington wants to merge 1 commit into
Conversation
|
@liketosweep can you also test this? I've actually ran out of Github Copliot credit 😆 . |
|
Hey @joshbarrington, just saw @dgageot gave it the green light! I was getting ready to pull it down but ran out of time on my end tonight before logging off. Thrilled it got approved and is ready to merge though great fix. |
|
@dgageot I just realised it makes sense to call the function earlier inside I'll push that change now |
|
@dgageot who else can we get a quick review from so we can get this fix in? |
|
@dgageot please re-review |
joshbarrington
changed the title
joshbarrington
force-pushed
the
fix/githubcopilot-model-defaults
branch
from
93090f9 to
52a34cd
Compare
|
@dgageot i have rebased this if you can reapprove for the CI jobs :) |
|
@dgageot friendly bump |
|
/review |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Good fix — the logic is sound and the ordering within applyProviderDefaults → applyModelDefaults is correct. CI is green. A few non-blocking suggestions below, mostly around test coverage and style consistency.
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-scoped fix. The logic is correct and handles the ordering concern well (overriding openai_chatcompletions that may have been set by the generic fallback). CI is all green.
All comments are non-blocking — this is mergeable as-is. The suggestions are just for style consistency and better test coverage.
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
2 findings — 1 LIKELY, 1 CONFIRMED (both medium severity)
| Severity | File | Issue |
|---|---|---|
| 🟡 MEDIUM | defaults.go:242 |
User-set api_type: openai_chatcompletions silently overridden |
| 🟡 MEDIUM | model_defaults_test.go:297 |
Missing test assertions for gpt-5.4-mini and gpt-5.4-nano |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix. Logic is correct, tests cover the main cases, CI is green. A few non-blocking suggestions below.
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
A few non-blocking suggestions inline. Nice fix overall — clean logic, good test coverage of the happy path, and CI is green.
aheritier
added
kind/fix
|
@aheritier thanks for the review, I've added the suggested improvements 👍 |
joshbarrington
force-pushed
the
fix/githubcopilot-model-defaults
branch
from
2983f35 to
d8cc381
Compare
aheritier
added
effort:small
joshbarrington
force-pushed
the
fix/githubcopilot-model-defaults
branch
from
70b2649 to
e3c7b09
Compare
|
@aheritier @dgageot have rebased and cleaned this up |
|
/review |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes correctly default api_type to openai_responses for GitHub Copilot models that require the /responses endpoint. The implementation:
- Guards against nil maps before writing
- Preserves any user-supplied custom
api_typevalues - Only overrides when the value is unset, empty, or was
openai_chatcompletions - Is wired as the first step in
applyModelDefaults, ensuring consistent behavior
No bugs found in the changed code.
joshbarrington
force-pushed
the
fix/githubcopilot-model-defaults
branch
from
e3c7b09 to
75fc27c
Compare
|
@aheritier I've rebased this, believe it should fix the tests. |
aheritier
removed
effort:small
6 participants
closes: #2303
Some Github Copilot models require the
/responsesendpoint as opposed to/chat/completions.These are not set by default, and so certain models will fail unless a custom agent config is set with
api_type: openai_responses. This sets the default for these models asopenai_responses.@liketosweep