-
Notifications
You must be signed in to change notification settings - Fork 17
fix: route Copilot /models to COPILOT_API_TARGET, not GitHub REST API #1952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -888,6 +888,31 @@ if (require.main === module) { | |||||||||||||||||||||||||||||||||||||||
| const contentLength = parseInt(req.headers['content-length'], 10) || 0; | ||||||||||||||||||||||||||||||||||||||||
| if (checkRateLimit(req, res, 'copilot', contentLength)) return; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Copilot CLI 1.0.21+ calls GET /models at startup (to list or validate models). | ||||||||||||||||||||||||||||||||||||||||
| // The /models endpoint lives on the Copilot inference API (COPILOT_API_TARGET), | ||||||||||||||||||||||||||||||||||||||||
| // NOT on the GitHub REST API. Explicitly use COPILOT_GITHUB_TOKEN for this | ||||||||||||||||||||||||||||||||||||||||
| // request so the GitHub OAuth token is used even when both COPILOT_GITHUB_TOKEN | ||||||||||||||||||||||||||||||||||||||||
| // and COPILOT_API_KEY are configured (COPILOT_API_KEY alone is not accepted by | ||||||||||||||||||||||||||||||||||||||||
| // the /models endpoint). | ||||||||||||||||||||||||||||||||||||||||
| let reqPathname; | ||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||
| reqPathname = new URL(req.url, 'http://localhost').pathname; | ||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||
| logRequest('warn', 'copilot_proxy_malformed_url', { | ||||||||||||||||||||||||||||||||||||||||
| message: 'Malformed request URL in Copilot proxy — rejecting with 400', | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| res.writeHead(400, { 'Content-Type': 'application/json' }); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+900
to
+904
|
||||||||||||||||||||||||||||||||||||||||
| } catch { | |
| logRequest('warn', 'copilot_proxy_malformed_url', { | |
| message: 'Malformed request URL in Copilot proxy — rejecting with 400', | |
| }); | |
| res.writeHead(400, { 'Content-Type': 'application/json' }); | |
| } catch (err) { | |
| const requestId = req.headers['x-request-id'] || generateRequestId(); | |
| const sanitizedUrl = sanitizeForLog(typeof req.url === 'string' ? req.url : ''); | |
| logRequest('warn', 'copilot_proxy_malformed_url', { | |
| request_id: requestId, | |
| message: 'Malformed request URL in Copilot proxy — rejecting with 400', | |
| method: req.method, | |
| url: sanitizedUrl, | |
| error: err && err.message ? sanitizeForLog(err.message) : 'Unknown URL parse error', | |
| }); | |
| res.writeHead(400, { | |
| 'Content-Type': 'application/json', | |
| 'X-Request-ID': requestId, | |
| }); |
Copilot
AI
Apr 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces new behavior for Copilot request routing (/models path detection + token selection + malformed URL handling) but there are no unit tests covering it. Since this module already has tests (server.test.js), add coverage to assert that GET /models uses COPILOT_GITHUB_TOKEN when set and that malformed req.url returns 400.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1535,6 +1535,9 @@ export function generateDockerCompose( | |||||||
| ...(config.geminiApiBasePath && { GEMINI_API_BASE_PATH: config.geminiApiBasePath }), | ||||||||
| // Forward GITHUB_SERVER_URL so api-proxy can auto-derive enterprise endpoints | ||||||||
| ...(process.env.GITHUB_SERVER_URL && { GITHUB_SERVER_URL: process.env.GITHUB_SERVER_URL }), | ||||||||
| // Forward GITHUB_API_URL so api-proxy can use the correct GitHub REST API hostname | ||||||||
| // on GHES/GHEC (e.g. https://ghes.example.com/api/v3 or api.mycompany.ghe.com) | ||||||||
| ...(process.env.GITHUB_API_URL && { GITHUB_API_URL: process.env.GITHUB_API_URL }), | ||||||||
|
Comment on lines
+1538
to
+1540
|
||||||||
| // Forward GITHUB_API_URL so api-proxy can use the correct GitHub REST API hostname | |
| // on GHES/GHEC (e.g. https://ghes.example.com/api/v3 or api.mycompany.ghe.com) | |
| ...(process.env.GITHUB_API_URL && { GITHUB_API_URL: process.env.GITHUB_API_URL }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new comment implies COPILOT_AUTH_TOKEN might resolve to COPILOT_API_KEY when both COPILOT_GITHUB_TOKEN and COPILOT_API_KEY are set, but resolveCopilotAuthToken() currently always prefers COPILOT_GITHUB_TOKEN. Either adjust the comment to reflect the actual precedence, or (if the intent is to use BYOK for most calls and OAuth only for /models) revisit resolveCopilotAuthToken/token selection logic accordingly.