feat(cli): auto-add api-target values to allowlist#1290
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
@claude[agent] fix the PR title and summary |
Updated PR title to "feat(cli): auto-add api-target values to allowlist" and added a comprehensive description including summary, changes, motivation, and testing details. |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.64% | 85.17% | 📉 -0.47% |
| Statements | 85.57% | 85.10% | 📉 -0.47% |
| Functions | 85.77% | 85.77% | ➡️ +0.00% |
| Branches | 78.48% | 77.64% | 📉 -0.84% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
55.7% → 53.9% (-1.79%) | 56.1% → 54.4% (-1.78%) |
src/docker-manager.ts |
87.4% → 87.9% (+0.50%) | 86.8% → 87.2% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
Adds CLI behavior to automatically allowlist API target hostnames (Copilot/OpenAI/Anthropic) when users specify them via flags or environment variables, and introduces integration coverage to prevent regressions in GitHub Agentic Workflows setups.
Changes:
- Auto-adds
--copilot-api-target,--openai-api-target,--anthropic-api-target(or corresponding env vars) into the computedallowedDomainslist before validation. - Extends the test runner to pass the new api-target flags through to the CLI.
- Adds integration tests intended to verify allowlisting, env-var handling, and duplicate prevention.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/cli.ts |
Auto-injects api-target hostnames into allowedDomains and logs when doing so. |
tests/fixtures/awf-runner.ts |
Adds support for passing api-target options into CLI invocations in tests. |
tests/integration/api-target-allowlist.test.ts |
New integration tests for the auto-allowlist behavior (currently brittle due to non-resolvable hostnames). |
Comments suppressed due to low confidence (3)
tests/integration/api-target-allowlist.test.ts:33
- These tests use hostnames like api.acme.ghe.com that are unlikely to resolve/reliably accept TCP 443 in CI. Since toAllowDomain only passes when Squid logs a successful TCP_TUNNEL/200, this can make the test fail due to DNS/connectivity rather than allowlist behavior. Use a stable resolvable host (e.g., example.com) for the curl target and api-target value.
test('should automatically add copilot-api-target to allowlist', async () => {
const result = await runner.runWithSudo(
'curl -s https://api.acme.ghe.com',
{
allowDomains: ['github.com'], // Note: NOT including api.acme.ghe.com
copilotApiTarget: 'api.acme.ghe.com',
enableApiProxy: true,
buildLocal: true,
tests/integration/api-target-allowlist.test.ts:55
- This test uses a .internal hostname (custom.openai-router.internal) which typically won’t resolve in CI, so Squid may never log a TCP_TUNNEL/200 even if the domain is allowlisted. Prefer a stable public domain (e.g., example.com) so the test outcome reflects the allowlist behavior.
test('should automatically add openai-api-target to allowlist', async () => {
const result = await runner.runWithSudo(
'curl -s https://custom.openai-router.internal',
{
allowDomains: ['github.com'], // Note: NOT including custom.openai-router.internal
openaiApiTarget: 'custom.openai-router.internal',
enableApiProxy: true,
buildLocal: true,
logLevel: 'debug',
timeout: 60000,
tests/integration/api-target-allowlist.test.ts:75
- Same issue here: custom.anthropic-router.internal is unlikely to resolve/reliably accept connections in CI, which makes this allowlist integration test brittle. Use a resolvable host for both the curl URL and anthropicApiTarget so Squid can record a TCP_TUNNEL/200 when allowlisted.
test('should automatically add anthropic-api-target to allowlist', async () => {
const result = await runner.runWithSudo(
'curl -s https://custom.anthropic-router.internal',
{
allowDomains: ['github.com'], // Note: NOT including custom.anthropic-router.internal
anthropicApiTarget: 'custom.anthropic-router.internal',
enableApiProxy: true,
buildLocal: true,
logLevel: 'debug',
timeout: 60000,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.64% | 85.17% | 📉 -0.47% |
| Statements | 85.57% | 85.10% | 📉 -0.47% |
| Functions | 85.77% | 85.77% | ➡️ +0.00% |
| Branches | 78.48% | 77.64% | 📉 -0.84% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
55.7% → 53.9% (-1.79%) | 56.1% → 54.4% (-1.78%) |
src/docker-manager.ts |
87.4% → 87.9% (+0.50%) | 86.8% → 87.2% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.64% | 85.17% | 📉 -0.47% |
| Statements | 85.57% | 85.10% | 📉 -0.47% |
| Functions | 85.77% | 85.77% | ➡️ +0.00% |
| Branches | 78.48% | 77.64% | 📉 -0.84% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
55.7% → 53.9% (-1.79%) | 56.1% → 54.4% (-1.78%) |
src/docker-manager.ts |
87.4% → 87.9% (+0.50%) | 86.8% → 87.2% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.64% | 84.96% | 📉 -0.68% |
| Statements | 85.57% | 84.87% | 📉 -0.70% |
| Functions | 85.77% | 85.40% | 📉 -0.37% |
| Branches | 78.48% | 77.30% | 📉 -1.18% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
55.7% → 53.3% (-2.39%) | 56.1% → 53.7% (-2.48%) |
src/docker-manager.ts |
87.4% → 87.9% (+0.50%) | 86.8% → 87.2% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.64% | 84.86% | 📉 -0.78% |
| Statements | 85.57% | 84.76% | 📉 -0.81% |
| Functions | 85.77% | 85.40% | 📉 -0.37% |
| Branches | 78.48% | 77.19% | 📉 -1.29% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
55.7% → 53.0% (-2.69%) | 56.1% → 53.4% (-2.77%) |
src/docker-manager.ts |
87.4% → 87.9% (+0.50%) | 86.8% → 87.2% (+0.48%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
Overall: FAIL
|
Smoke Test Results
Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all versions match — Python and Node.js versions differ between host and chroot environments.
|
Smoke Test Results
Overall: PASS
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
…1294) * Initial plan * fix: extract api-target logic into testable helper, add unit tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
Smoke Test Results
Overall: PASS
|
Smoke Test Results — PASS✅ GitHub MCP: Last 2 merged PRs: #1294 "fix: extract api-target logic into testable helper, add unit tests" ( Overall: PASS
|
|
Chroot Version Comparison Results
Overall: ❌ Not all runtimes match — Python and Node.js versions differ between host and chroot environments. Go versions match.
|
|
Smoke Test Results — PASS
|
Automatically adds API target values (
--copilot-api-target,--openai-api-target,--anthropic-api-target) to the firewall allowlist when specified, eliminating the need for users to manually include them in--allow-domains.Changes
src/cli.ts): When api-target flags or environment variables are set, they are automatically added to the allowlist before domain validationtests/integration/api-target-allowlist.test.ts): Comprehensive test coverage for CLI flags, environment variables, duplicate prevention, and multiple targetstests/fixtures/awf-runner.ts): Added support for api-target options in test runnerMotivation
In GitHub Agentic Workflows, users can specify
engine.api-target(e.g.,api.acme.ghe.comfor GHEC deployments). Previously, users had to manually add these values to--allow-domains, which was error-prone and confusing. This change makes the configuration seamless by automatically including api-targets in the allowlist.Testing