diff --git a/docs/adr/0022-copilot-cli-egress-allowlist-uses-wildcard-domains.md b/docs/adr/0022-copilot-cli-egress-allowlist-uses-wildcard-domains.md new file mode 100644 index 0000000..7bb9951 --- /dev/null +++ b/docs/adr/0022-copilot-cli-egress-allowlist-uses-wildcard-domains.md @@ -0,0 +1,53 @@ +# 0022. The copilot-cli egress allowlist uses dotted-wildcard domains only + +- **Status:** Accepted +- **Date:** 2026-07-06 +- **Deciders:** project owner, Copilot + +## Context + +The local `copilot-cli` Pier installed agent (`src/copilot_experiments/pier_agents/copilot_cli.py`) +declares a `NetworkAllowlist` so air-gapped DeepSWE tasks (`allow_internet = false`) can still reach +the small set of GitHub hosts the Copilot CLI needs to install and run. + +Pier enforces that allowlist with an **egress proxy that is a Squid instance**. Squid renders the +allowlist into a `dstdomain` ACL, one entry per domain. Squid's `dstdomain` treats a bare domain and +its leading-dot wildcard as an **overlapping, fatal** pair: if both `github.com` and `.github.com` +appear, Squid aborts at startup with + +``` +ERROR: '.github.com' is a subdomain of 'github.com' +FATAL: Bungled ... acl allowed_domains dstdomain +``` + +The proxy container exits (1). The agent (`main`) container depends on the proxy via +`condition: service_healthy`, so every trial fails to start its environment. Pier then surfaces a +confusing downstream symptom — `Could not find the file /logs/artifacts/model.patch` — that hides the +real cause. The original allowlist listed three domains both bare and dotted +(`github.com`/`.github.com`, `githubcopilot.com`/`.githubcopilot.com`, +`githubusercontent.com`/`.githubusercontent.com`), so this crashed 100% of DeepSWE trials. + +## Decision + +We will list **only** the leading-dot wildcard form for any domain whose subdomains we need. In +Squid a `.example.com` entry already matches the apex `example.com` *and* every subdomain, so the +bare entries are both redundant and fatal. The allowlist is: + +```python +[".github.com", ".githubcopilot.com", ".githubusercontent.com", "gh.io"] +``` + +`gh.io` stays bare: it is a single redirector host with no subdomains we call, and its +`/copilot-install` redirect targets `*.githubusercontent.com`, already covered by the wildcard. + +A regression test asserts the wildcard domains are present and that no domain appears both bare and +as a `.domain` wildcard, so this exact Squid conflict cannot be reintroduced silently. + +## Consequences + +- DeepSWE (and any other air-gapped) tasks run through the egress proxy again; the proxy container + starts healthy instead of exiting (1). +- Anyone extending the allowlist must add the **dotted** form (e.g. `.example.com`), not the bare + domain, whenever subdomains are needed — the test enforces this and a code comment explains why. +- Bare domains remain acceptable only for hosts with no subdomain we contact (like `gh.io`); if such + a host ever needs subdomains, switch it to the dotted form rather than listing both. diff --git a/docs/adr/README.md b/docs/adr/README.md index 8791c59..6be3d64 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -37,3 +37,5 @@ We follow the lightweight format popularized by | [0018](0018-adopt-pytest-cov-for-local-coverage-analysis.md) | Adopt pytest-cov for local coverage analysis | Accepted | | [0019](0019-use-nested-pier-run-directories.md) | Use nested Pier run directories | Accepted | | [0020](0020-remove-legacy-native-harness.md) | Remove the legacy native harness | Accepted | +| [0021](0021-interactive-html-dashboards.md) | Interactive self-contained HTML result dashboards | Accepted | +| [0022](0022-copilot-cli-egress-allowlist-uses-wildcard-domains.md) | The copilot-cli egress allowlist uses dotted-wildcard domains only | Accepted | diff --git a/src/copilot_experiments/pier_agents/copilot_cli.py b/src/copilot_experiments/pier_agents/copilot_cli.py index 02ced35..a8bb33c 100644 --- a/src/copilot_experiments/pier_agents/copilot_cli.py +++ b/src/copilot_experiments/pier_agents/copilot_cli.py @@ -98,14 +98,16 @@ def parse_version(self, stdout: str) -> str: return match.group(1) if match else text def network_allowlist(self) -> NetworkAllowlist: + # Pier's egress proxy is Squid, whose dstdomain ACL treats a bare domain + # and its ".domain" wildcard as a fatal conflict ("'.github.com' is a + # subdomain of 'github.com'"). Listing both crashes the proxy container + # (exit 1) and fails every trial. A leading-dot entry already matches the + # apex domain *and* all subdomains, so we list only the wildcard forms + # (plus the bare redirector host gh.io, which has no subdomains). return NetworkAllowlist( domains=[ - "api.github.com", - "github.com", ".github.com", - "githubcopilot.com", ".githubcopilot.com", - "githubusercontent.com", ".githubusercontent.com", "gh.io", ] diff --git a/src/copilot_experiments/report.py b/src/copilot_experiments/report.py index 578455f..6f7c254 100644 --- a/src/copilot_experiments/report.py +++ b/src/copilot_experiments/report.py @@ -115,7 +115,7 @@ def summary_markdown(summary: dict, description: str = "") -> str: for agent in summary["agents"]: for task in agent.get("tasks", []): lines.append( - f"| {agent['name']} | {task['task_slug']} | {task['n_trials']} | " + f"| {agent['name']} | {task['task']} | {task['n_trials']} | " f"{_pct(task.get('success_rate'))} | {_pct(task.get('resolved_rate'))} | " f"{_aiu(task.get('avg_aiu'))} |" ) diff --git a/tests/test_pier_agent.py b/tests/test_pier_agent.py index 39b92d6..ca417bd 100644 --- a/tests/test_pier_agent.py +++ b/tests/test_pier_agent.py @@ -40,7 +40,14 @@ def test_copilot_cli_install_spec_and_allowlist(tmp_path: Path): assert install.version == "1.0.64" assert any("https://gh.io/copilot-install" in step.run for step in install.steps) assert install.verification_command and "copilot --version" in install.verification_command - assert {"github.com", "api.github.com", "githubcopilot.com", "gh.io"} <= domains + # Leading-dot wildcards cover the apex domain and all subdomains. + assert {".github.com", ".githubcopilot.com", ".githubusercontent.com", "gh.io"} <= domains + # Squid's dstdomain ACL fatals when a domain is listed both bare and as + # ".domain"; guard against reintroducing such a conflict. + bare = {d for d in domains if not d.startswith(".")} + assert not any(f".{d}" in domains for d in bare), ( + "allowlist must not contain a domain both bare and as a .domain wildcard" + ) def test_copilot_cli_version_parser(tmp_path: Path): diff --git a/tests/test_pier_results.py b/tests/test_pier_results.py index 3c99b4a..0551fbd 100644 --- a/tests/test_pier_results.py +++ b/tests/test_pier_results.py @@ -448,6 +448,52 @@ def test_write_pier_summary(tmp_path: Path): assert "Agent" in (job_dir / "summary.md").read_text(encoding="utf-8") +def _make_multi_task_pier_job(job_dir: Path) -> Path: + _write_json(job_dir / "config.json", {"job_name": "demo-job"}) + _write_json( + job_dir / "result.json", + { + "started_at": "2026-01-01T00:00:00Z", + "finished_at": "2026-01-01T00:00:05Z", + "stats": {"n_errored_trials": 0}, + }, + ) + for task, reward in (("alpha", 1), ("beta", 0)): + trial = job_dir / f"copilot-cli__{task}__1" + _write_json( + trial / "result.json", + { + "trial_name": f"copilot-cli__{task}__1", + "task_name": task, + "started_at": "2026-01-01T00:00:00Z", + "finished_at": "2026-01-01T00:00:05Z", + "agent_info": { + "name": "copilot-cli", + "model_info": {"name": "gpt-5-mini"}, + }, + "config": {"agent": {"kwargs": {"reasoning_effort": "low"}}}, + "verifier_result": {"rewards": {"reward": reward}}, + }, + ) + return job_dir + + +def test_write_pier_summary_renders_multi_task_table(tmp_path: Path): + # Regression: the per-task "Task suite coverage" table read task["task_slug"], + # but _aggregate_task stores the slug under "task", so multi-task summaries + # raised KeyError: 'task_slug' when rendering summary.md. + job_dir = _make_multi_task_pier_job(tmp_path / "jobs" / "demo-job" / "20260620-153000") + + summary = write_pier_summary(job_dir) + + assert summary["n_tasks"] == 2 + markdown = (job_dir / "summary.md").read_text(encoding="utf-8") + assert "## Task suite coverage" in markdown + assert "| Agent | Task | Trials | Mean success | Resolved@k | Avg AIU |" in markdown + assert "| alpha |" in markdown + assert "| beta |" in markdown + + def _trial(success: bool | None, aiu: float | None, tokens: float | None, task: str = "t") -> dict: return { "success": success,