refactor: split agent-service.ts into focused modules by lpcox · Pull Request #2635 · github/gh-aw-firewall

lpcox · 2026-05-06T23:13:44Z

Summary

Splits src/services/agent-service.ts (1,103 lines) into three focused modules:

Module	Responsibility	Lines
`agent-environment.ts`	`buildAgentEnvironment()` — env vars, proxy config, secrets	433
`agent-volumes.ts`	`buildAgentVolumes()` — bind mounts, home whitelisting, SSL	409
`agent-service.ts`	`buildAgentService()` + `buildIptablesInitService()` + re-exports	245

Also splits the matching test file (agent-service.test.ts, 1,998 lines) into three focused test modules that mirror the production code structure.

Backwards Compatibility

agent-service.ts re-exports all public symbols from the new modules, so existing imports continue to work unchanged.

Verification

✅ Build passes (npm run build)
✅ All 127 agent-service tests pass
✅ Lint passes (warnings only, no errors)

Closes #2622
Closes #2626

Extract buildAgentEnvironment (414 lines) into agent-environment.ts and buildAgentVolumes (402 lines) into agent-volumes.ts. Agent-service.ts retains buildAgentService + buildIptablesInitService with re-exports for backwards compatibility. Split matching test file into agent-environment.test.ts (1,025 lines), agent-volumes.test.ts (617 lines), and agent-service.test.ts (411 lines). All 127 tests pass. Build passes. Closes #2622 Closes #2626 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-05-06T23:14:59Z

✅ Coverage Check Passed

Overall Coverage

Metric	Base	PR	Delta
Lines	87.51%	87.63%	📈 +0.12%
Statements	87.46%	87.58%	📈 +0.12%
Functions	82.35%	82.47%	📈 +0.12%
Branches	79.73%	79.81%	📈 +0.08%

📁 Per-file Coverage Changes (2 files)

File	Lines (Before → After)	Statements (Before → After)
`src/container-lifecycle.ts`	87.1% → 88.2% (+1.14%)	87.5% → 88.6% (+1.11%)
`src/services/agent-service.ts`	95.6% → 97.5% (+1.90%)	95.3% → 97.6% (+2.29%)

✨ New Files (2 files)

src/services/agent-environment.ts: 98.5% lines
src/services/agent-volumes.ts: 91.6% lines

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copilot

Pull request overview

This PR refactors the agent container configuration code by splitting the previously large src/services/agent-service.ts into two new focused modules (agent-environment.ts and agent-volumes.ts) while keeping agent-service.ts as the service-definition layer and re-export surface for backwards compatibility. It also mirrors this split in the Jest test suite to keep tests aligned with production responsibilities.

Changes:

Extracted environment-variable construction into src/services/agent-environment.ts and volume/bind-mount construction into src/services/agent-volumes.ts.
Updated src/services/agent-service.ts to re-export the moved symbols while keeping buildAgentService() / buildIptablesInitService() in place.
Split the original monolithic agent service test file into agent-environment.test.ts, agent-volumes.test.ts, and a reduced agent-service.test.ts.

Show a summary per file

File	Description
src/services/agent-environment.ts	New module containing `buildAgentEnvironment()` and related types.
src/services/agent-volumes.ts	New module containing `buildAgentVolumes()` and related types.
src/services/agent-service.ts	Now focuses on service definitions and re-exports env/volume builders for compatibility.
src/services/agent-environment.test.ts	New test file for environment-variable behavior.
src/services/agent-volumes.test.ts	New test file for volume/mount behavior.
src/services/agent-service.test.ts	Reduced to service-definition-focused tests + remaining integration-style tests.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 6/6 changed files
Comments generated: 3

+    // System paths (read-only) - required for binaries and libraries
+    agentVolumes.push(
+      '/usr:/host/usr:ro',
+      '/bin:/host/bin:ro',
+      '/sbin:/host/sbin:ro',
+    );
+
+    // Handle /lib and /lib64 - may be symlinks on some systems
+    // Always mount them to ensure library resolution works
+    agentVolumes.push('/lib:/host/lib:ro');
+    agentVolumes.push('/lib64:/host/lib64:ro');
+
+    // Tool cache - language runtimes from GitHub runners (read-only)
+    // /opt/hostedtoolcache contains Python, Node, Ruby, Go, Java, etc.
+    agentVolumes.push('/opt:/host/opt:ro');
+
+    // Special filesystem mounts for chroot (needed for devices and runtime introspection)
+    // NOTE: /proc is NOT bind-mounted here. Instead, a fresh container-scoped procfs is
+    // mounted at /host/proc in entrypoint.sh via 'mount -t proc'. This provides:
+    //   - Dynamic /proc/self/exe (required by .NET CLR and other runtimes)
+    //   - /proc/cpuinfo, /proc/meminfo (required by JVM, .NET GC)
+    //   - Container-scoped only (does not expose host process info)
+    // The mount requires SYS_ADMIN capability, which is dropped before user code runs.


+    for (const domain of config.allowedDomains) {
+      // Skip patterns that aren't resolvable hostnames
+      if (domain.startsWith('*.') || domain.startsWith('.') || domain.includes('*')) continue;
+      // Skip if already in hosts file
+      if (hostsContent.includes(domain)) continue;


+  // Also hide credentials at /host paths (chroot mounts home at /host$HOME too)
+  logger.debug('Hiding credential files at /host paths');
+
+  // Note: In chroot mode, effectiveHome === getRealUserHome() (see line 433),


lpcox · 2026-05-06T23:32:25Z

@copilot address the review feedback

- Fix extra indentation on lines 43-262 (dedent by 2 spaces for readability) - Fix hostsContent.includes(domain) false positives with line/field-based matching - Fix stale 'see line 433' comment reference, replace with function/module name" Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/5a4df47e-0521-44a6-8306-a18f28f92ff2

Copilot · 2026-05-06T23:38:51Z

@copilot address the review feedback

Done in commit b12a1ee. Three fixes applied to src/services/agent-volumes.ts:

Indentation: Dedented lines 43–262 by 2 spaces so the chroot path mounts are at the correct function-body indent level, not erroneously nested.
False-positive domain check: Replaced hostsContent.includes(domain) with a line/field-based check that splits each non-comment hosts line on whitespace and looks for an exact hostname token match, preventing substrings like notgithub.com from matching github.com.
Stale comment: Replaced "see line 433" with a stable module reference ("resolved by the caller in compose-generator.ts").

lpcox · 2026-05-07T00:06:50Z

@copilot merge main

…service Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot · 2026-05-07T00:10:01Z

@copilot merge main

Done — merged origin/main into this branch in commit cdaca62. The only conflict was in src/services/agent-service.test.ts (the import path ../docker-manager → ../compose-generator); resolved in favour of main's updated path. All 127 tests pass.

github-actions · 2026-05-07T00:12:40Z

🔬 Smoke Test Results

Test	Status
GitHub MCP connectivity	✅ PR listed: "refactor: split agent-service.ts into focused modules"
GitHub.com HTTP	✅ HTTP 200
File write/read	⚠️ Pre-step template vars not substituted (skipped)

Overall: PASS (2/2 verifiable tests passed)

PR author: @lpcox · Reviewer: @Mossaka

📰 BREAKING: Report filed by Smoke Copilot

github-actions · 2026-05-07T00:12:42Z

Smoke Test Results

✅ GitHub MCP: Last 2 merged PRs fetched successfully
✅ Playwright: GitHub homepage loaded, title verified
✅ File Writing: Test file created in /tmp/gh-aw/agent/
✅ Bash Tool: File verified via cat

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude

github-actions · 2026-05-07T00:13:45Z

🔥 Smoke Test: Copilot BYOK (Offline) Mode

Test	Result
GitHub MCP (list PRs)	✅
GitHub.com connectivity	⚠️ (pre-step value not substituted)
File write/read	⚠️ (pre-step value not substituted)
BYOK inference (this response)	✅

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

PR by @lpcox · Reviewer: @Mossaka

Overall: PASS (core BYOK path validated; pre-step template vars were not substituted for tests 2–3)

🔑 BYOK report filed by Smoke Copilot BYOK

github-actions · 2026-05-07T00:14:55Z

🧪 Chroot Smoke Test Results

Runtime	Host Version	Chroot Version	Match?
Python	Python 3.12.13	Python 3.12.3	❌
Node.js	v24.14.1	v20.20.2	❌
Go	go1.22.12	go1.22.12	✅

Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environment.

Tested by Smoke Chroot

github-actions · 2026-05-07T00:14:55Z

🏗️ Build Test Suite Results

Ecosystem	Project	Build/Install	Tests	Status
Bun	elysia	✅	1/1 passed	✅ PASS
Bun	hono	✅	1/1 passed	✅ PASS
C++	fmt	✅	N/A	✅ PASS
C++	json	✅	N/A	✅ PASS
Deno	oak	N/A	1/1 passed	✅ PASS
Deno	std	N/A	1/1 passed	✅ PASS
.NET	hello-world	✅	N/A	✅ PASS
.NET	json-parse	✅	N/A	✅ PASS
Go	color	✅	1/1 passed	✅ PASS
Go	env	✅	1/1 passed	✅ PASS
Go	uuid	✅	1/1 passed	✅ PASS
Java	gson	✅	1/1 passed	✅ PASS
Java	caffeine	✅	1/1 passed	✅ PASS
Node.js	clsx	✅	all passed	✅ PASS
Node.js	execa	✅	all passed	✅ PASS
Node.js	p-limit	✅	all passed	✅ PASS
Rust	fd	✅	1/1 passed	✅ PASS
Rust	zoxide	✅	1/1 passed	✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #2635 · ● 650.7K · ◷

github-actions · 2026-05-07T00:15:38Z

Smoke Test

docs: add billing bug warning for retired releases v0.25.21–v0.25.39
refactor: split api-proxy server.test.js into focused modules
GitHub PR review: ✅
Safe Inputs GH CLI: ❌
Playwright title: ✅
Tavily search: ❌
File/Bash + Build: ✅
Discussion safe-input: ❌
Overall status: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

github-actions · 2026-05-07T00:16:02Z

Smoke Test Results

Redis PING: ❌ (timeout — no response from host.docker.internal:6379)
PostgreSQL pg_isready: ❌ (no response from host.docker.internal:5432)
PostgreSQL SELECT 1: ❌ (skipped — pg unreachable)

Overall: FAIL — service containers are not reachable from this runner environment.

🔌 Service connectivity validated by Smoke Services

Copilot AI review requested due to automatic review settings May 6, 2026 23:13

lpcox requested a review from Mossaka as a code owner May 6, 2026 23:13

Copilot started reviewing on behalf of lpcox May 6, 2026 23:14 View session