Handle unversioned OpenAI /responses paths in API proxy sidecar#2080
Handle unversioned OpenAI /responses paths in API proxy sidecar#2080
/responses paths in API proxy sidecar#2080Conversation
/responses paths in API proxy sidecar
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Updates the api-proxy sidecar’s path handling to keep Codex/OpenAI clients working when they send unversioned Responses API paths (e.g. /responses) to OPENAI_BASE_URL, by normalizing to OpenAI’s versioned /v1/* surface when appropriate.
Changes:
- Add OpenAI-specific default
/v1prefixing for unversioned paths when targetingapi.openai.comand no explicit base path is configured. - Harden OpenAI host detection by normalizing the target host before comparison (e.g. handling
api.openai.com:443). - Add Jest regression tests covering
/responsesnormalization and non-OpenAI passthrough behavior.
Show a summary per file
| File | Description |
|---|---|
| containers/api-proxy/server.js | Adds OpenAI host-based /v1 default prefixing and duplicate-prefix avoidance in buildUpstreamPath(). |
| containers/api-proxy/server.test.js | Adds regression tests validating /responses → /v1/responses on OpenAI and no rewrite for non-OpenAI targets. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| function buildUpstreamPath(reqUrl, targetHost, basePath) { | ||
| const targetUrl = new URL(reqUrl, `https://${targetHost}`); | ||
| const prefix = basePath === '/' ? '' : basePath; | ||
| return prefix + targetUrl.pathname + targetUrl.search; | ||
| const pathname = targetUrl.pathname; |
There was a problem hiding this comment.
buildUpstreamPath() currently accepts protocol-relative paths (e.g. //host/path). Because new URL() treats //... as a host override, this can drop the first path segment and produce surprising upstream paths. Consider rejecting reqUrl values that start with // (or normalizing them) before calling new URL() to ensure only origin-form paths are forwarded.
See below for a potential fix:
* @param {string} reqUrl - The incoming request URL (must start with '/' and not '//')
* @param {string} targetHost - The upstream hostname (used only to parse the URL)
* @param {string} basePath - Normalized base path prefix (e.g. '/serving-endpoints' or '')
* @returns {string} Full upstream path including query string
*/
function buildUpstreamPath(reqUrl, targetHost, basePath) {
if (reqUrl.startsWith('//')) {
throw new Error('Protocol-relative request URLs are not supported');
}
| if (!prefix) { | ||
| let normalizedTargetHost = targetHost; | ||
| try { | ||
| normalizedTargetHost = new URL(`https://${targetHost}`).hostname; | ||
| } catch { | ||
| // Fall back to the raw host value if parsing fails. | ||
| } | ||
| if (normalizedTargetHost === 'api.openai.com') { | ||
| prefix = '/v1'; | ||
| } |
There was a problem hiding this comment.
buildUpstreamPath() parses targetHost a second time to normalize the hostname for the OpenAI check. Since you already constructed targetUrl with new URL(reqUrl, ...), you can use targetUrl.hostname (or normalize once upstream) to avoid redundant parsing/try-catch and keep the logic simpler.
| if (!prefix) { | |
| let normalizedTargetHost = targetHost; | |
| try { | |
| normalizedTargetHost = new URL(`https://${targetHost}`).hostname; | |
| } catch { | |
| // Fall back to the raw host value if parsing fails. | |
| } | |
| if (normalizedTargetHost === 'api.openai.com') { | |
| prefix = '/v1'; | |
| } | |
| if (!prefix && targetUrl.hostname === 'api.openai.com') { | |
| prefix = '/v1'; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address this review feedback #2080 (review) |
Addressed in commit d4ab433. I implemented both review items: |
🔥 Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( Overall: PASS · PR by
|
|
Smoke Test Results ✅ PASS Recent Merged PRs:
Test Status:
|
This comment has been minimized.
This comment has been minimized.
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
Chroot Version Comparison Results
Overall: FAILED — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
🔥 Smoke Test Results — Copilot Engine
Overall: PASS Author:
|
Codex CLI now targets OpenAI Responses API paths like
/responsesagainstOPENAI_BASE_URL. The sidecar forwarded paths verbatim, which broke against OpenAI’s versioned API surface and caused 404s in Smoke Codex.OpenAI path normalization in catch-all forwarding
buildUpstreamPath()incontainers/api-proxy/server.jsto map unversioned OpenAI paths to/v1/*when:api.openai.comOPENAI_API_BASE_PATHis configuredHost normalization hardening
api.openai.com:443is treated consistently as OpenAI.Focused regression coverage
containers/api-proxy/server.test.jsfor:/responses -> /v1/responseson OpenAI/v1/responses/v1on non-OpenAI custom targets