fix(api-proxy): derive api.subdomain.ghe.com for GHEC domains#1072
fix(api-proxy): derive api.subdomain.ghe.com for GHEC domains#1072
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
lpcox
changed the title
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
|
@claude[agent] can you add a description for this pr? |
Added a comprehensive PR description explaining the automatic Copilot API endpoint derivation for GHEC domains. (commit 085cf29) Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Claude
AI
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds GitHub Enterprise Cloud data residency (*.ghe.com) support for Copilot API routing in the API proxy sidecar, along with documentation and tests.
Changes:
- Update
deriveCopilotApiTarget()to auto-deriveapi.<subdomain>.ghe.comwhenGITHUB_SERVER_URLends with.ghe.com. - Document
*.ghe.comconfiguration and the related environment variables/flags. - Add Jest tests intended to cover the Copilot API target derivation logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/api-proxy-sidecar.md | Documents *.ghe.com auto-derivation behavior and adds COPILOT_API_TARGET / GITHUB_SERVER_URL to the env var table. |
| containers/api-proxy/server.test.js | Adds tests for Copilot API target derivation (currently via a standalone duplicate implementation). |
| containers/api-proxy/server.js | Implements .ghe.com → api.<subdomain>.ghe.com derivation logic in deriveCopilotApiTarget(). |
Comments suppressed due to low confidence (1)
containers/api-proxy/server.test.js:33
- The jest.mock() calls and require cache deletion in getDerivedTarget() are currently dead code: nothing requires ./server.js afterward, and the return value doesn’t depend on process.env. Once the test is updated to import the real deriveCopilotApiTarget(), simplify this helper to only set/restore env (or call the function with explicit inputs) to keep the test deterministic and easier to maintain.
function getDerivedTarget(env = {}) {
// Set environment variables
Object.keys(env).forEach(key => {
process.env[key] = env[key];
});
// Clear module cache to force re-evaluation
delete require.cache[require.resolve('./server.js')];
// Mock the required modules that have side effects
jest.mock('http', () => ({
createServer: jest.fn(() => ({
listen: jest.fn(),
})),
}));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/api-proxy-sidecar.md
Outdated
| | `OPENAI_API_KEY` | Real API key | `--enable-api-proxy` and env set | OpenAI API key (injected into requests) | | ||
| | `ANTHROPIC_API_KEY` | Real API key | `--enable-api-proxy` and env set | Anthropic API key (injected into requests) | | ||
| | `COPILOT_GITHUB_TOKEN` | Real token | `--enable-api-proxy` and env set | GitHub Copilot token (injected into requests) | | ||
| | `COPILOT_API_TARGET` | Target hostname | `--copilot-api-target` flag | Override Copilot API endpoint (default: auto-derived) | |
There was a problem hiding this comment.
In the environment variable table, COPILOT_API_TARGET can be set either via --copilot-api-target or via the host environment variable COPILOT_API_TARGET (see CLI behavior). The “When set” column currently implies it’s only set via the flag; please update this row to reflect both sources for accuracy/consistency with the other rows.
| | `COPILOT_API_TARGET` | Target hostname | `--copilot-api-target` flag | Override Copilot API endpoint (default: auto-derived) | | |
| | `COPILOT_API_TARGET` | Target hostname | `--copilot-api-target` flag or host env `COPILOT_API_TARGET` | Override Copilot API endpoint (default: auto-derived) | |
There was a problem hiding this comment.
Updated the environment variable table to show both sources. (commit 43f446f)
containers/api-proxy/server.test.js
Outdated
| // Helper function to reload the module and get the derived target | ||
| function getDerivedTarget(env = {}) { | ||
| // Set environment variables | ||
| Object.keys(env).forEach(key => { | ||
| process.env[key] = env[key]; | ||
| }); | ||
|
|
||
| // Clear module cache to force re-evaluation | ||
| delete require.cache[require.resolve('./server.js')]; | ||
|
|
||
| // Mock the required modules that have side effects | ||
| jest.mock('http', () => ({ | ||
| createServer: jest.fn(() => ({ | ||
| listen: jest.fn(), | ||
| })), | ||
| })); | ||
|
|
||
| jest.mock('https', () => ({ | ||
| request: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('./logging', () => ({ | ||
| generateRequestId: jest.fn(() => 'test-id'), | ||
| sanitizeForLog: jest.fn(x => x), | ||
| logRequest: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('./metrics', () => ({ | ||
| increment: jest.fn(), | ||
| gaugeInc: jest.fn(), | ||
| gaugeDec: jest.fn(), | ||
| observe: jest.fn(), | ||
| statusClass: jest.fn(() => '2xx'), | ||
| getSummary: jest.fn(() => ({})), | ||
| getMetrics: jest.fn(() => ({})), | ||
| })); | ||
|
|
||
| jest.mock('./rate-limiter', () => ({ | ||
| create: jest.fn(() => ({ | ||
| check: jest.fn(() => ({ allowed: true })), | ||
| getAllStatus: jest.fn(() => ({})), | ||
| })), | ||
| })); | ||
|
|
||
| // We can't easily extract the function since it's not exported, | ||
| // but we can test via the startup logs which log COPILOT_API_TARGET | ||
| // For now, let's create a standalone version to test | ||
| return deriveCopilotApiTargetStandalone(env); | ||
| } | ||
|
|
||
| // Standalone version of the function for testing | ||
| function deriveCopilotApiTargetStandalone(env) { | ||
| const COPILOT_API_TARGET = env.COPILOT_API_TARGET; | ||
| const GITHUB_SERVER_URL = env.GITHUB_SERVER_URL; | ||
|
|
||
| if (COPILOT_API_TARGET) { | ||
| return COPILOT_API_TARGET; | ||
| } | ||
|
|
||
| if (GITHUB_SERVER_URL) { | ||
| try { | ||
| const hostname = new URL(GITHUB_SERVER_URL).hostname; | ||
| if (hostname !== 'github.com') { | ||
| // For GitHub Enterprise Cloud with data residency (*.ghe.com), | ||
| // derive the API endpoint as api.SUBDOMAIN.ghe.com | ||
| if (hostname.endsWith('.ghe.com')) { | ||
| const subdomain = hostname.replace('.ghe.com', ''); | ||
| return `api.${subdomain}.ghe.com`; | ||
| } | ||
| // For other enterprise hosts (GHES), use the generic enterprise endpoint | ||
| return 'api.enterprise.githubcopilot.com'; | ||
| } | ||
| } catch { | ||
| // Invalid URL — fall through to default | ||
| } | ||
| } | ||
| return 'api.githubcopilot.com'; | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests don’t exercise the production deriveCopilotApiTarget() in server.js. getDerivedTarget() clears require cache and sets up mocks, but never requires ./server.js, and ultimately returns deriveCopilotApiTargetStandalone() (a duplicated implementation). This can easily drift from server.js and give false confidence. Refactor so the real function is imported and tested (e.g., move deriveCopilotApiTarget into a small module with no side effects, or guard server startup with require.main === module and export the function), then delete the standalone copy and the unused mocks/cache handling.
This issue also appears on line 19 of the same file.
| // Helper function to reload the module and get the derived target | |
| function getDerivedTarget(env = {}) { | |
| // Set environment variables | |
| Object.keys(env).forEach(key => { | |
| process.env[key] = env[key]; | |
| }); | |
| // Clear module cache to force re-evaluation | |
| delete require.cache[require.resolve('./server.js')]; | |
| // Mock the required modules that have side effects | |
| jest.mock('http', () => ({ | |
| createServer: jest.fn(() => ({ | |
| listen: jest.fn(), | |
| })), | |
| })); | |
| jest.mock('https', () => ({ | |
| request: jest.fn(), | |
| })); | |
| jest.mock('./logging', () => ({ | |
| generateRequestId: jest.fn(() => 'test-id'), | |
| sanitizeForLog: jest.fn(x => x), | |
| logRequest: jest.fn(), | |
| })); | |
| jest.mock('./metrics', () => ({ | |
| increment: jest.fn(), | |
| gaugeInc: jest.fn(), | |
| gaugeDec: jest.fn(), | |
| observe: jest.fn(), | |
| statusClass: jest.fn(() => '2xx'), | |
| getSummary: jest.fn(() => ({})), | |
| getMetrics: jest.fn(() => ({})), | |
| })); | |
| jest.mock('./rate-limiter', () => ({ | |
| create: jest.fn(() => ({ | |
| check: jest.fn(() => ({ allowed: true })), | |
| getAllStatus: jest.fn(() => ({})), | |
| })), | |
| })); | |
| // We can't easily extract the function since it's not exported, | |
| // but we can test via the startup logs which log COPILOT_API_TARGET | |
| // For now, let's create a standalone version to test | |
| return deriveCopilotApiTargetStandalone(env); | |
| } | |
| // Standalone version of the function for testing | |
| function deriveCopilotApiTargetStandalone(env) { | |
| const COPILOT_API_TARGET = env.COPILOT_API_TARGET; | |
| const GITHUB_SERVER_URL = env.GITHUB_SERVER_URL; | |
| if (COPILOT_API_TARGET) { | |
| return COPILOT_API_TARGET; | |
| } | |
| if (GITHUB_SERVER_URL) { | |
| try { | |
| const hostname = new URL(GITHUB_SERVER_URL).hostname; | |
| if (hostname !== 'github.com') { | |
| // For GitHub Enterprise Cloud with data residency (*.ghe.com), | |
| // derive the API endpoint as api.SUBDOMAIN.ghe.com | |
| if (hostname.endsWith('.ghe.com')) { | |
| const subdomain = hostname.replace('.ghe.com', ''); | |
| return `api.${subdomain}.ghe.com`; | |
| } | |
| // For other enterprise hosts (GHES), use the generic enterprise endpoint | |
| return 'api.enterprise.githubcopilot.com'; | |
| } | |
| } catch { | |
| // Invalid URL — fall through to default | |
| } | |
| } | |
| return 'api.githubcopilot.com'; | |
| } | |
| // Helper function to get the derived target from the production implementation | |
| function getDerivedTarget(env = {}) { | |
| // Set environment variables | |
| Object.keys(env).forEach(key => { | |
| process.env[key] = env[key]; | |
| }); | |
| // Import and use the real deriveCopilotApiTarget from server.js | |
| const { deriveCopilotApiTarget } = require('./server.js'); | |
| return deriveCopilotApiTarget(); | |
| } |
There was a problem hiding this comment.
Refactored tests to export and use the real deriveCopilotApiTarget function. Removed the duplicate standalone implementation and unused mocks. (commit 43f446f)
| sudo -E awf --enable-api-proxy \ | ||
| --allow-domains api.mycompany.ghe.com,mycompany.ghe.com \ | ||
| -- npx @github/copilot --prompt "your prompt" |
There was a problem hiding this comment.
The docs example allowlist includes mycompany.ghe.com, but the api-proxy only makes outbound Copilot requests to the derived API host (e.g., api.mycompany.ghe.com). Listing extra domains encourages a broader-than-necessary egress policy. Consider narrowing the example to only the required Copilot API domain (and mention adding additional domains only if the user’s workflow needs them).
There was a problem hiding this comment.
Updated examples to use wildcard pattern *.mycompany.ghe.com which matches all subdomains including api.mycompany.ghe.com. Added explanation that additional domains should only be added if needed for the user's workflow. (commit 43f446f)
🦀 Rust Build Test Results
Overall: ✅ PASS
|
🦕 Deno Build Test Results
Overall: ✅ PASS
|
C++ Build Test Results
Overall: PASS ✅
|
|
Smoke test results (run #22465649023) ✅ GitHub MCP — Last 2 merged PRs: #1069 "fix(deps): resolve high-severity rollup vulnerability in docs-site", #1067 "fix(ci): recompile ci-doctor and add missing workflows" (both by Overall: PASS
|
Build Test: Node.js Results
Overall: ✅ PASS
|
Smoke Test Results
Overall: PASS
|
Build Test: Bun Results
Overall: ✅ PASS Bun version: 1.3.10
|
Go Build Test Results ✅
Overall: PASS
|
Claude
AI
changed the title
Deno Build Test Results
Overall: ✅ PASS Deno version: 2.7.1
|
🧪 Bun Build Test Results
Overall: PASS ✅ All tests passed using Bun v1.3.10.
|
Go Build Test Results ✅
Overall: PASS
|
🦀 Rust Build Test Results
Overall: ✅ PASS
|
Smoke Test ResultsRecent merged PRs:
Overall: PASS
|
|
Smoke Test Results (run #22468322836) ✅ GitHub MCP — Last 2 merged PRs: Overall: PASS
|
🟢 Build Test: Node.js — PASS
Overall: PASS
|
C++ Build Test Results
Overall: PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world: json-parse: {
"Name": "AWF Test",
"Version": 1,
"Success": true
}
Name: AWF Test, Success: True
|
Java Build Test Results
Overall: ✅ PASS
|
Chroot Version Comparison Results
Result:
|
…1078) * Initial plan * fix: add explicit execute directive to smoke-codex to prevent noop Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
@claude[agent] can you fix the pr title and summary? |
Deno Build Test Results
Overall: ✅ PASS
|
Build Test: Bun Results
Overall: PASS ✅ Bun version:
|
Smoke Test Results — Copilot Engine
Overall: PASS PR author:
|
|
Smoke Test Results ✅ GitHub MCP: #1078 fix: add explicit execute directive to smoke-codex to prevent noop / #1069 fix(deps): resolve high-severity rollup vulnerability in docs-site Overall: PASS
|
Node.js Build Test Results
Overall: ✅ PASS
|
Go Build Test Results
Overall: ✅ PASS
|
C++ Build Test Results
Overall: PASS 🎉 All C++ projects configured and built successfully.
|
Updated PR title to "fix(api-proxy): derive api.subdomain.ghe.com for GHEC domains" and added comprehensive description covering the automatic endpoint derivation feature, test refactoring, and documentation improvements. |
Rust Build Test Results
Overall: PASS ✅
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
Claude
AI
changed the title
Java Build Test Results
Overall: PASS ✅ All projects compiled and all tests passed successfully.
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Summary
This PR adds automatic Copilot API endpoint derivation for GitHub Enterprise Cloud (GHEC) with data residency domains (
*.ghe.com).Changes
API Proxy Enhancement: The api-proxy now automatically derives the correct Copilot API endpoint based on
GITHUB_SERVER_URL*.ghe.comdomains:mycompany.ghe.com→api.mycompany.ghe.comapi.enterprise.githubcopilot.comgithub.com: continues to useapi.githubcopilot.comTest Coverage: Refactored tests to use real implementation instead of duplicate
deriveCopilotApiTargetfunction from server.jsDocumentation: Updated
docs/api-proxy-sidecar.mdwith:*.mycompany.ghe.com)Why This Matters
GHEC customers with data residency requirements use custom domains like
mycompany.ghe.comand need traffic routed to region-specific API endpoints (api.mycompany.ghe.com) rather than the generic enterprise endpoint. This change makes that routing automatic whenGITHUB_SERVER_URLis set, eliminating the need for manual--copilot-api-targetconfiguration.Testing
COPILOT_API_TARGETenv var