Fix GitHub Copilot gpt-5.4 endpoint routing#453
Fix GitHub Copilot gpt-5.4 endpoint routing#453HeCHieh wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019d14cd-bc90-70ce-b1ae-87bc97332650 Co-authored-by: Amp <amp@ampcode.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where GitHub Copilot's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes GitHub Copilot request routing for models (specifically gpt-5.4) that do not support the /chat/completions endpoint, ensuring requests are sent to /responses when required.
Changes:
- Update GitHub Copilot endpoint selection to prefer
/responseswhen model metadata indicates/chat/completionsis unsupported. - Add a static GitHub Copilot model definition for
gpt-5.4as a/responses-only model. - Add a regression test to cover
gpt-5.4routing behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/runtime/executor/github_copilot_executor.go | Adds endpoint selection logic based on SupportedEndpoints and a helper to check endpoint membership. |
| internal/runtime/executor/github_copilot_executor_test.go | Adds a regression test asserting gpt-5.4 routes via /responses. |
| internal/registry/model_definitions.go | Adds a static gpt-5.4 GitHub Copilot model entry marked as /responses-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if info := registry.GetGlobalRegistry().GetModelInfo(baseModel, ""); info != nil { | ||
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The registry lookup uses GetModelInfo(baseModel, "") which returns the last-registered/global model info for that ID, not the GitHub Copilot provider-specific info. Since ModelRegistry keys are shared across providers, this can select endpoints from an unrelated provider (depending on registration order) and mis-route Copilot traffic. Consider querying provider-specific info (e.g. provider "github-copilot") or otherwise scoping the lookup to Copilot models only.
| if info := registry.GetGlobalRegistry().GetModelInfo(baseModel, ""); info != nil { | |
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | |
| return true | |
| } | |
| } |
| for _, info := range registry.GetGitHubCopilotModels() { | ||
| if info == nil || !strings.EqualFold(info.ID, baseModel) { | ||
| continue | ||
| } | ||
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | ||
| return true | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
useGitHubCopilotResponsesEndpoint calls registry.GetGitHubCopilotModels() (which constructs the full static model list) on every request and then linearly scans it. This adds avoidable allocations/CPU in a hot path; consider caching a map/set of responses-only Copilot models (or consulting the dynamic registry first and only falling back to static definitions on miss).
| func TestUseGitHubCopilotResponsesEndpoint_RegistryResponsesOnlyModel(t *testing.T) { | ||
| t.Parallel() | ||
| if !useGitHubCopilotResponsesEndpoint(sdktranslator.FromString("openai"), "gpt-5.4") { | ||
| t.Fatal("expected responses-only registry model to use /responses") |
There was a problem hiding this comment.
This test name suggests it exercises the dynamic ModelRegistry path, but it only relies on the static GitHub Copilot model list (no registry setup). Either rename it to reflect the static-definition behavior, or extend it to register a temporary model in the global registry with SupportedEndpoints={"/responses"} and assert the registry-based routing logic is honored.
| func TestUseGitHubCopilotResponsesEndpoint_RegistryResponsesOnlyModel(t *testing.T) { | |
| t.Parallel() | |
| if !useGitHubCopilotResponsesEndpoint(sdktranslator.FromString("openai"), "gpt-5.4") { | |
| t.Fatal("expected responses-only registry model to use /responses") | |
| func TestUseGitHubCopilotResponsesEndpoint_StaticResponsesOnlyModel(t *testing.T) { | |
| t.Parallel() | |
| if !useGitHubCopilotResponsesEndpoint(sdktranslator.FromString("openai"), "gpt-5.4") { | |
| t.Fatal("expected responses-only model to use /responses") |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the routing issue for the gpt-5.4 model by adding a static definition and updating the endpoint selection logic. The changes are logical and include a necessary regression test. I have two suggestions for improvement: one to enhance consistency in the new model definition, and another to correct a potential logic flaw in the endpoint selection that could cause issues with model definitions in the dynamic registry.
| if info := registry.GetGlobalRegistry().GetModelInfo(baseModel, ""); info != nil { | ||
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | ||
| return true | ||
| } | ||
| } | ||
| for _, info := range registry.GetGitHubCopilotModels() { | ||
| if info == nil || !strings.EqualFold(info.ID, baseModel) { | ||
| continue | ||
| } | ||
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | ||
| return true | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
The current logic for determining which endpoint to use has a potential flaw in how it prioritizes model definitions. It first checks the dynamic registry, and if the model is found but isn't a /responses-only model, it falls through to check the static list of GitHub Copilot models. This can lead to incorrect behavior if a model has different definitions in the dynamic and static registries, as the dynamic definition should be considered authoritative.
For instance, if a model is dynamically registered with SupportedEndpoints: ["/chat/completions", "/responses"], the first check fails. If a stale static definition for the same model exists with SupportedEndpoints: ["/responses"], the function will incorrectly return true.
The logic should be revised to ensure that if a model is found in the dynamic registry, that definition is used exclusively for the decision, and the static list is only consulted as a fallback.
| if info := registry.GetGlobalRegistry().GetModelInfo(baseModel, ""); info != nil { | |
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | |
| return true | |
| } | |
| } | |
| for _, info := range registry.GetGitHubCopilotModels() { | |
| if info == nil || !strings.EqualFold(info.ID, baseModel) { | |
| continue | |
| } | |
| if len(info.SupportedEndpoints) > 0 && !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) { | |
| return true | |
| } | |
| break | |
| } | |
| if info := registry.GetGlobalRegistry().GetModelInfo(baseModel, ""); info != nil { | |
| // If model is in dynamic registry, its definition is authoritative. | |
| if len(info.SupportedEndpoints) > 0 { | |
| return !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) | |
| } | |
| // No endpoint info, so fall through to the codex check. | |
| } else { | |
| // If not in dynamic registry, check static definitions. | |
| for _, info := range registry.GetGitHubCopilotModels() { | |
| if info != nil && strings.EqualFold(info.ID, baseModel) { | |
| if len(info.SupportedEndpoints) > 0 { | |
| return !containsEndpoint(info.SupportedEndpoints, githubCopilotChatPath) && containsEndpoint(info.SupportedEndpoints, githubCopilotResponsesPath) | |
| } | |
| // Found model, but no endpoint info, so fall through to codex check. | |
| break | |
| } | |
| } | |
| } |
| ContextLength: 200000, | ||
| MaxCompletionTokens: 32768, | ||
| SupportedEndpoints: []string{"/responses"}, | ||
| Thinking: &ThinkingSupport{Levels: []string{"low", "medium", "high", "xhigh"}}, |
There was a problem hiding this comment.
For consistency with other /responses-only models like gpt-5.3-codex, consider adding "none" to the Thinking.Levels for gpt-5.4. This would allow users to explicitly disable the thinking feature for this model, which is a common option for similar models.
| Thinking: &ThinkingSupport{Levels: []string{"low", "medium", "high", "xhigh"}}, | |
| Thinking: &ThinkingSupport{Levels: []string{"none", "low", "medium", "high", "xhigh"}}, |
Amp-Thread-ID: https://ampcode.com/threads/T-019d14cd-bc90-70ce-b1ae-87bc97332650 Co-authored-by: Amp <amp@ampcode.com>
Summary
Problem
GitHub Copilot gpt-5.4 currently fails with:
{"error":{"message":"model \"gpt-5.4\" is not accessible via the /chat/completions endpoint","code":"unsupported_api_for_model"}}The root cause is that upstream path selection only checked whether the model name contains
codex. That works for codex models, but not forgpt-5.4, which also needs the/responsesendpoint.Testing
go test ./internal/runtime/executor ./internal/registry