[ARC/DinD] Emit chroot.binariesSourcePath and chroot.identity in AWF stdin-config#38911
[ARC/DinD] Emit chroot.binariesSourcePath and chroot.identity in AWF stdin-config#38911Copilot wants to merge 13 commits into
Conversation
…OCKER_HOST condition Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…test Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. Test quality analysis complete |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR updates gh-aw’s AWF (gh-aw-firewall) integration to support ARC/DinD split-filesystem runners by emitting AWF v0.27.1+ chroot.binariesSourcePath and chroot.identity.* configuration, injected at runtime when DOCKER_HOST indicates a TCP DinD topology.
Changes:
- Added an AWF version gate for chroot config support (
AWFChrootConfigMinVersion = v0.27.1) and a helperawfSupportsChrootConfig. - Extended the AWF config schema structs to include a
chrootsection (populated via runtime injection, not compile-time JSON generation). - Embedded a compact Python heredoc into the existing ARC/DinD
DOCKER_HOSTprobe to patch the AWF config file in place; added tests + regenerated wasm golden fixtures.
Show a summary per file
| File | Description |
|---|---|
| pkg/constants/version_constants.go | Adds the minimum AWF version constant for chroot config support. |
| pkg/workflow/awf_config.go | Adds chroot config structs/field for schema completeness and documentation. |
| pkg/workflow/awf_helpers.go | Injects runtime Python patching of AWF config under ARC/DinD TCP DOCKER_HOST, plus new version gate helper. |
| pkg/workflow/awf_helpers_test.go | Adds version-gate and runtime-injection tests for the new chroot patching behavior. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Updates compiled golden to include the injected Python heredoc. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Updates compiled golden to include the injected Python heredoc. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 3
| return fmt.Sprintf(` python - <<'PY' | ||
| import json,os,subprocess as sp | ||
| from pathlib import Path | ||
| p=Path(os.environ["RUNNER_TEMP"])/"gh-aw"/"awf-config.json" | ||
| c=json.loads(p.read_text()) | ||
| c["chroot"]={"binariesSourcePath":"%s","identity":{"user":sp.check_output(["id","-un"],text=True).strip(),"uid":int(sp.check_output(["id","-u"],text=True)),"gid":int(sp.check_output(["id","-g"],text=True)),"home":"%s"}} | ||
| p.write_text(json.dumps(c,separators=(",",":"),ensure_ascii=False)+"\n") | ||
| PY`, awfArcDindChrootBinariesSourcePath, awfArcDindChrootIdentityHome) |
There was a problem hiding this comment.
Both issues were addressed in an earlier commit: the Python patch now writes to both ${RUNNER_TEMP}/gh-aw/awf-config.json and /tmp/gh-aw/awf-config.json, and the body is wrapped in try/except Exception as e: raise SystemExit(...) for actionable errors.
| // The Python is intentionally written as a dense single line to minimise script size | ||
| // and stay within GitHub Actions' 21 KB per-step expression limit. |
There was a problem hiding this comment.
Fixed in an earlier commit — the comment now reads "intentionally kept compact" rather than "dense single line".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (388 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. The choice to patch config at runtime via an embedded Python heredoc (rather than at compile time, or via a bootstrap action) is exactly the kind of trade-off future contributors will want explained. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
ADRs are stored in 🔒 This PR cannot merge until an ADR is linked in the PR body.
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the behavioral contract — the promises the system makes to its users and collaborators. References: §27444242896
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 3 new tests are design tests (behavioral contracts) with error/edge case coverage. Note: test inflation was flagged mechanically (ratio ≈ 3.7×) but is justified given the end-to-end bash+Python script execution in TestArcDindChrootConfigInjection.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two correctness issues and a test coverage gap.
📋 Key Themes & Highlights
Key Themes
pythonvspython3: The generated script callspython -but the test skip guard checks forpython3. On Ubuntu 22.04+ (no barepython), the test runs but the bash script fails — a confusing failure instead of a clean skip. This is the most actionable item.- Missing error handling in the new Python: The existing companion script (~line 149) has comprehensive
try/except SystemExitblocks forRUNNER_TEMP, file-not-found, and JSON decode errors. The new dense Python omits all of this, producing cryptic tracebacks on failure. - Test coverage gap:
TestArcDindChrootConfigInjectionverifieschrootwas added but never asserts that pre-existing config fields (apiProxy) survived the patch.
Positive Highlights
- ✅ Excellent version-gate pattern (
AWFChrootConfigMinVersion) with clear boundary tests atv0.27.0andv0.27.1 - ✅ Smart design choice to merge the Python patch body inside the existing DOCKER_HOST guard — single
DOCKER_HOSTcheck in the generated script is the right invariant - ✅
buildArcDindChrootConfigInjectScript()as a test-only wrapper overbuildArcDindChrootConfigPatchBody()cleanly separates the testable unit from the embedded production form - ✅ 9 golden files all updated consistently — easy to verify the injection is present across all engine variants
- ✅ The PR description is unusually complete, explaining the 21 KB per-step size constraint that motivates the dense Python
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 403.1 AIC · ⌖ 22.8 AIC · ⊞ 27.8K
| // section is added with the expected static paths and runtime identity values. | ||
| func TestArcDindChrootConfigInjection(t *testing.T) { | ||
| if _, err := exec.LookPath("python3"); err != nil { | ||
| t.Skip("python3 not available") |
There was a problem hiding this comment.
[/tdd] Skip condition checks for python3 but the generated script calls python -. On Ubuntu 22.04+ runners where only python3 is in PATH, the test will not be skipped (python3 is found) but the bash script will fail at python - <<'PY', surfacing as a confusing require.NoError failure rather than a skip.
💡 Fix: align the check with the command the script actually runs
Change
if _, err := exec.LookPath("python3"); err != nil {
t.Skip("python3 not available")
}to check for "python" — or, better, change buildArcDindChrootConfigPatchBody to emit python3 - and keep the check as python3. The rest of the codebase (mcp_scripts_generator.go, copilot_engine_execution.go, etc.) consistently uses python3.
There was a problem hiding this comment.
Done in 037e1f0 — both scripts now use python3 - and the test skip guard is exec.LookPath("python3") to match.
| p=Path(os.environ["RUNNER_TEMP"])/"gh-aw"/"awf-config.json" | ||
| c=json.loads(p.read_text()) | ||
| c["chroot"]={"binariesSourcePath":"%s","identity":{"user":sp.check_output(["id","-un"],text=True).strip(),"uid":int(sp.check_output(["id","-u"],text=True)),"gid":int(sp.check_output(["id","-g"],text=True)),"home":"%s"}} | ||
| p.write_text(json.dumps(c,separators=(",",":"),ensure_ascii=False)+"\n") |
There was a problem hiding this comment.
[/grill-with-docs] The dense Python has no error handling, which diverges noticeably from the established pattern in this same file (see the buildArcDindSetupScript-equivalent at ~line 149, which wraps every I/O call in try/except SystemExit). If RUNNER_TEMP is unset, the user gets a raw KeyError; if awf-config.json is missing, a bare FileNotFoundError. Both are fixable without much size cost.
💡 Suggested minimal guard
The 21 KB limit is a real constraint, but adding a single environment check and a try/except around the read costs fewer than 100 bytes:
import json,os,subprocess as sp
from pathlib import Path
rt=os.environ.get("RUNNER_TEMP") or (lambda:(_ for _ in ()).throw(SystemExit("RUNNER_TEMP not set")))()
p=Path(rt)/"gh-aw"/"awf-config.json"
try: c=json.loads(p.read_text())
except OSError as e: raise SystemExit(f"cannot read {p}: {e}") from e
c["chroot"]={...}
p.write_text(json.dumps(c,separators=(",",":"),ensure_ascii=False)+"\n")Or at minimum mirror the existing pattern: if not rt: raise SystemExit("RUNNER_TEMP is not set").
There was a problem hiding this comment.
Error handling was added in an earlier commit — the body is wrapped in try/except Exception as e: raise SystemExit(f"chroot config patch failed: {e}") from e.
| require.Contains(t, string(out), `"chroot"`, | ||
| "chroot section should be injected for DOCKER_HOST=%s", tt.dockerHost) | ||
|
|
||
| chrootRaw, ok := result["chroot"].(map[string]any) |
There was a problem hiding this comment.
[/tdd] The test verifies the chroot section was added, but never checks that the pre-existing config fields (e.g., apiProxy) survived the patch. If the Python accidentally replaced the whole config instead of merging into it, this test would still pass.
💡 Add a preservation assertion
// After the chroot assertions, verify the original fields are intact.
apiProxy, ok := result["apiProxy"].(map[string]any)
require.True(t, ok, "apiProxy section must be preserved after chroot injection")
assert.Equal(t, true, apiProxy["enabled"], "apiProxy.enabled should be unchanged")This turns the test into a true round-trip spec: inject chroot config → original fields are intact.
There was a problem hiding this comment.
Added in 037e1f0 — after the chroot assertions, the test now verifies apiProxy.enabled is still true, ensuring the patch merged rather than replaced the config.
| // The Python is intentionally written as a dense single line to minimise script size | ||
| // and stay within GitHub Actions' 21 KB per-step expression limit. | ||
| func buildArcDindChrootConfigPatchBody() string { | ||
| return fmt.Sprintf(` python - <<'PY' |
There was a problem hiding this comment.
[/grill-with-docs] python - is inconsistent with the rest of the codebase which exclusively uses python3 (mcp_scripts_generator.go, copilot_engine_execution.go, copilot_engine_installation.go). The companion script at line ~149 of this same file also uses python -, so this is a pre-existing divergence, but this PR is a good opportunity to standardise on python3 - to avoid ambiguity on environments where python resolves to Python 2 or is absent.
💡 One-line fix
Change python - <<PY → python3 - <<PY here and in the corresponding script at line ~149, and update the test skip guard to exec.LookPath("python3") (already using that — so the test already reflects the desired state).
There was a problem hiding this comment.
Done in 037e1f0 — both scripts (buildWorkflowCallNetworkAllowedUpdateScript and buildArcDindChrootConfigPatchBody) now use python3 -, consistent with the rest of the codebase.
| assert.Contains(t, command, awfArcDindChrootIdentityHome, | ||
| "command should include the expected identity.home constant") | ||
| assert.Contains(t, command, awfArcDindDockerHostRegex, | ||
| "chroot inject script should reuse the DinD Docker host regex") |
There was a problem hiding this comment.
[/tdd] TestBuildAWFCommand_IncludesChrootInjectScript uses string-contains checks that only verify the chroot body exists somewhere in the command, not that it is nested inside the DOCKER_HOST if-block. If the injection were accidentally emitted at the top level, these assertions would still pass.
💡 Stronger structural assertion
Use an index comparison to confirm the injection appears after the DOCKER_HOST guard:
dockerhostIdx := strings.Index(command, awfArcDindDockerHostRegex)
binariesIdx := strings.Index(command, "binariesSourcePath")
require.Greater(t, binariesIdx, dockerhostIdx,
"chroot injection must appear after the DOCKER_HOST=(redacted) guard")This spec-tests the key structural invariant the PR description calls out.
There was a problem hiding this comment.
Added in 037e1f0 — the test now uses strings.Index to confirm binariesSourcePath appears at a higher offset than awfArcDindDockerHostRegex in the generated command, verifying the chroot injection is nested inside the if-block.
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…dling, fix test python check Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Done in commit Changes made:
|
|
@copilot merge main |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · 54.8 AIC · ⌖ 25.5 AIC · ⊞ 8.1K
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "gh-aw": patch | |||
There was a problem hiding this comment.
Smoke test: changeset frontmatter looks correct — patch bump is appropriate here.
| "gh-aw": patch | ||
| --- | ||
|
|
||
| Emit `chroot.binariesSourcePath` and `chroot.identity` in AWF stdin config for ARC/DinD runners. |
There was a problem hiding this comment.
Smoke test: clear, descriptive changeset summary. Nice work.
|
[ARC/DinD] Emit chroot.binariesSourcePath and chroot.identity in AWF stdin-config Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
@copilo run pr-finisher skill |
|
@copilot run pr-finisher skill |
|
Smoke Test: 1✅,2✅,3✅,4✅,5✅,6✅,7✅,8✅,9❌,10✅,11❌,12✅,13❌,14✅,15✅. Overall PASS. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — see summary below. |
|
@copilot review all comments and address the unresolved review feedback, especially the python vs python3 mismatch and the missing Python error handling.
|
|
@copilot please summarize the remaining blockers and rerun checks after the review feedback is addressed.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
Please re-trigger CI and summarize any remaining blockers.
|
|
@copilot please refresh the branch, rerun checks, and summarize the remaining blockers.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
Please re-run checks after the branch update and summarize any remaining blockers.
|
|
Re-run the workflow on a fresh failure issue and confirm the new token-consumption section renders correctly.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
Please refresh the branch and rerun checks after addressing the feedback.
|
ARC/DinD users currently need a bootstrap action to copy the copilot binary into the DinD daemon's filesystem and pre-seed
/etc/passwdwith runner identity. AWF v0.27.1+ supportschroot.binariesSourcePathandchroot.identity.*natively, but gh-aw wasn't emitting these fields.Changes
Version gate (
pkg/constants/version_constants.go):AWFChrootConfigMinVersion = "v0.27.1"Config structs (
pkg/workflow/awf_config.go): AddedAWFChrootConfig/AWFChrootIdentityConfigand aChroot *AWFChrootConfigfield toAWFConfigFilefor schema completeness; fields are populated at runtime by the injected script, not byBuildAWFConfigJSON.Runtime injection (
pkg/workflow/awf_helpers.go): When AWF ≥ v0.27.1, a compact Python heredoc is merged inside the existingarcDindPrefixProbeif [[ "${DOCKER_HOST:-}" =~ ^tcp:// ]]block. On DinD runners it patches the AWF config file in-place before AWF reads it:Merged into the existing
ifblock (rather than a separate guard) so only oneDOCKER_HOSTcheck appears in the generated script.python3is used consistently with the rest of the codebase. The script writes the patched config to both${RUNNER_TEMP}/gh-aw/awf-config.json(read by AWF) and/tmp/gh-aw/awf-config.json(unified agent artifact upload), and wraps all I/O intry/except SystemExitfor actionable errors.Tests:
TestAWFSupportsChrootConfig(version gate),TestArcDindChrootConfigInjection(6 subtests covering tcp/unix/empty DOCKER_HOST viabuildArcDindChrootConfigInjectScript, including round-trip verification that original config fields survive the patch),TestBuildAWFCommand_IncludesChrootInjectScript(present/absent based on version, with a structural index assertion confirming the injection is nested inside theDOCKER_HOSTguard). Golden files regenerated.Changeset
chroot.binariesSourcePathandchroot.identityin AWF stdin config for ARC/DinD runners.pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/27467068427