Require RUNNER_TOOL_CACHE for tool-cache discovery#40157
Conversation
Co-authored-by: zarenner <13670625+zarenner@users.noreply.github.com>
|
@copilot recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🌑 The shadows whisper... Smoke Codex failed to deliver outputs. The oracle requires further meditation... |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40157 does not have the 'implementation' label (has_implementation_label=false) and has only 46 new lines of code in business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). No custom config present. Neither Condition A nor Condition B is met. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
There was a problem hiding this comment.
Pull request overview
This pull request removes gh-aw’s fallback guessing for runner tool-cache locations and instead requires RUNNER_TOOL_CACHE for (1) discovering tool-cache bin directories used for Node resolution inside AWF, and (2) deciding whether a read-only bind mount is needed for non-/opt tool caches.
Changes:
- Require
RUNNER_TOOL_CACHEin the generated PATH setup (GetNpmBinPathSetup) and stop searching hard-coded fallback roots (/opt/hostedtoolcache,/home/runner/work/_tool). - Require
RUNNER_TOOL_CACHEin AWF tool-cache mount probing and stop probing the legacy_toolpath. - Update unit tests plus wasm golden outputs / compiled
.lock.ymlworkflows to match the new required-env behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/nodejs.go | Makes GetNpmBinPathSetup() require RUNNER_TOOL_CACHE and only search that root for bin dirs. |
| pkg/workflow/awf_helpers.go | Updates AWF tool-cache mount probing to require RUNNER_TOOL_CACHE and drop legacy _tool probing. |
| pkg/workflow/copilot_engine_execution.go | Updates comments/intent around PATH setup to reflect RUNNER_TOOL_CACHE-only behavior. |
| pkg/workflow/engine_helpers_test.go | Updates assertions and a shell-based test to cover required RUNNER_TOOL_CACHE behavior and removal of fallback paths. |
| pkg/workflow/awf_config_test.go | Updates expectations for the AWF command’s tool-cache mount probe and asserts fallback paths are not present. |
| pkg/workflow/copilot_home_expansion_test.go | Adjusts commentary to remove now-stale mention of _tool fallback in unrelated HOME-resolution tests. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Updates wasm golden output to require RUNNER_TOOL_CACHE and remove fallback tool-cache searches. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Same as above for the “with-imports” fixture output. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Same as above for Playwright CLI mode fixture output. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Same as above for smoke Copilot fixture output. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Updates all-engines Copilot golden output to require RUNNER_TOOL_CACHE and stop probing fallback roots. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Same as above for Claude golden output. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Same as above for Codex golden output. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Same as above for Gemini golden output. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Same as above for Pi golden output. |
| .github/workflows/workflow-normalizer.lock.yml | Regenerates compiled workflow to require RUNNER_TOOL_CACHE and remove fallback tool-cache probing/search paths. |
| .github/workflows/video-analyzer.lock.yml | Same compiled-workflow update. |
| .github/workflows/update-astro.lock.yml | Same compiled-workflow update. |
| .github/workflows/test-workflow.lock.yml | Same compiled-workflow update. |
| .github/workflows/test-project-url-default.lock.yml | Same compiled-workflow update. |
| .github/workflows/test-dispatcher.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-test-tools.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-temporary-id.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-pi.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-opencode.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-gemini.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-crush.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-ci.lock.yml | Same compiled-workflow update. |
| .github/workflows/smoke-antigravity.lock.yml | Same compiled-workflow update. |
| .github/workflows/security-review.lock.yml | Same compiled-workflow update. |
| .github/workflows/research.lock.yml | Same compiled-workflow update. |
| .github/workflows/repo-tree-map.lock.yml | Same compiled-workflow update. |
| .github/workflows/release.lock.yml | Same compiled-workflow update. |
| .github/workflows/pdf-summary.lock.yml | Same compiled-workflow update. |
| .github/workflows/notion-issue-summary.lock.yml | Same compiled-workflow update. |
| .github/workflows/issue-triage-agent.lock.yml | Same compiled-workflow update. |
| .github/workflows/hippo-embed.lock.yml | Same compiled-workflow update. |
| .github/workflows/functional-pragmatist.lock.yml | Same compiled-workflow update. |
| .github/workflows/firewall.lock.yml | Same compiled-workflow update. |
| .github/workflows/example-permissions-warning.lock.yml | Same compiled-workflow update. |
| .github/workflows/example-failure-category-filter.lock.yml | Same compiled-workflow update. |
| .github/workflows/dependabot-repair.lock.yml | Same compiled-workflow update. |
| .github/workflows/dependabot-go-checker.lock.yml | Same compiled-workflow update. |
| .github/workflows/dependabot-burner.lock.yml | Same compiled-workflow update. |
| .github/workflows/daily-team-status.lock.yml | Same compiled-workflow update. |
| .github/workflows/daily-malicious-code-scan.lock.yml | Same compiled-workflow update. |
| .github/workflows/daily-credit-limit-test.lock.yml | Same compiled-workflow update. |
| .github/workflows/codex-github-remote-mcp-test.lock.yml | Same compiled-workflow update. |
| .github/workflows/changeset.lock.yml | Same compiled-workflow update. |
| .github/workflows/brave.lock.yml | Same compiled-workflow update. |
| .github/workflows/bot-detection.lock.yml | Same compiled-workflow update. |
| .github/workflows/ai-moderator.lock.yml | Same compiled-workflow update. |
| .github/workflows/agentic-token-optimizer.lock.yml | Same compiled-workflow update. |
| .github/workflows/ace-editor.lock.yml | Same compiled-workflow update. |
| .changeset/patch-mount-non-opt-tool-cache-in-awf.md | Updates changeset text to reflect removal of guessed fallback paths and reliance on RUNNER_TOOL_CACHE. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 116/266 changed files
- Comments generated: 0
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
Smoke Test Results\n- GitHub MCP Testing: ✅\n- Web Fetch Testing: ✅\n- File Writing Testing: ✅\n- Bash Tool Testing: ✅\n- Build gh-aw: ❌\n\nOverall status: FAILWarning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 · 72.5 AIC · ⌖ 8.47 AIC · ⊞ 8.5K
| fi | ||
| GH_AW_TOOL_CACHE_MOUNT="" | ||
| GH_AW_TOOL_CACHE="${RUNNER_TOOL_CACHE:-/opt/hostedtoolcache}" | ||
| GH_AW_TOOL_CACHE="${RUNNER_TOOL_CACHE:?RUNNER_TOOL_CACHE must be set}" |
There was a problem hiding this comment.
Good change — requiring RUNNER_TOOL_CACHE via :? makes the failure explicit instead of silently mounting a fallback path. 💥
| fi | ||
| GH_AW_TOOL_CACHE_MOUNT="" | ||
| GH_AW_TOOL_CACHE="${RUNNER_TOOL_CACHE:-/opt/hostedtoolcache}" | ||
| GH_AW_TOOL_CACHE="${RUNNER_TOOL_CACHE:?RUNNER_TOOL_CACHE must be set}" |
There was a problem hiding this comment.
Consistent with the first hunk — nice that both agent and harness paths now use the same required-variable pattern.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. 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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving; the core fix is correct with a couple of minor test hardening suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Fail-fast by design: Replacing
:-/opt/hostedtoolcachewith:?RUNNER_TOOL_CACHE must be setconverts a silent misconfiguration into an actionable error — exactly the/diagnosepattern of fixing root cause, not symptom. - ✅ Single-source truth: The template change in
pkg/workflow/nodejs.goandpkg/workflow/awf_helpers.gomechanically regenerates all 266 lock files — consistent and auditable. - ✅ Test update quality:
TestGetNpmBinPathSetup_NoGorootDoesNotBreakChainnow correctly suppliesRUNNER_TOOL_CACHEviat.TempDir()so the shell execution test actually reflects the new contract. - ✅ findIdx guard: The new
if findIdx < 0check inTestGetNpmBinPathSetupprevents a false-pass if thefind "$GH_AW_TOOL_CACHE"pattern is ever absent — good defensive testing.
Improvement Opportunities
/tdd — Missing behavioral test for unset RUNNER_TOOL_CACHE (pkg/workflow/engine_helpers_test.go)
TestGetNpmBinPathSetup checks string content only. The fail-fast contract is the heart of this change; a shell-execution regression test that verifies the command actually exits non-zero when RUNNER_TOOL_CACHE is unset would close the loop:
func TestGetNpmBinPathSetup_FailsWhenRunnerToolCacheUnset(t *testing.T) {
if runtime.GOOS != "linux" {
t.Skip("Skipping shell-based test on non-Linux platform")
}
cmd := exec.Command("bash", "-c",
fmt.Sprintf("unset RUNNER_TOOL_CACHE; %s", GetNpmBinPathSetup()))
output, err := cmd.CombinedOutput()
if err == nil {
t.Errorf("expected non-zero exit when RUNNER_TOOL_CACHE is unset, got: %s", output)
}
if !strings.Contains(string(output), "RUNNER_TOOL_CACHE must be set") {
t.Errorf("expected error message in output, got: %s", output)
}
}/tdd — Tighten the :? assertion (pkg/workflow/engine_helpers_test.go, TestGetNpmBinPathSetup)
strings.Contains(pathSetup, "RUNNER_TOOL_CACHE must be set") passes as long as the message appears anywhere. Asserting RUNNER_TOOL_CACHE:?RUNNER_TOOL_CACHE must be set (the full operator form) would catch a regression where the enforcement operator is accidentally softened to :-.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 112.3 AIC · ⌖ 8.39 AIC · ⊞ 6.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 3 ( 🔍 Analysis Notes
i️ Pre-existing note (not a PR violation): Verdict
|
|
Smoke test results: ❌ Overall FAIL\nTests: ✅1, ✅2, ❌3, ✅4, ✅5, ✅6, ✅8 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.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 5 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Smoke tests passed. ✅ Overall PASS.
|
There was a problem hiding this comment.
Smoke review submitted with inline notes.
Warning
Firewall blocked 5 domains
The following domains were blocked by the firewall during workflow execution:
accounts.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"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · 244.9 AIC · ⌖ 26.9 AIC · ⊞ 19.1K
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 317.7 AIC · ⌖ 7.55 AIC · ⊞ 5.1K
Comments that could not be inline-anchored
pkg/workflow/engine_helpers_test.go:337
TestGetNpmBinPathSetup_GorootOrdering tests a hardcoded shell command, not the actual GetNpmBinPathSetup() function — a GOROOT-ordering regression in the function itself will not be caught by this test.
<details>
<summary>💡 Suggested fix</summary>
TestGetNpmBinPathSetup_NoGorootDoesNotBreakChain (line 366) shows the correct pattern: it inlines GetNpmBinPathSetup() as %s so the actual function is exercised. The GOROOT ordering test should mirror that:
shellCmd := fmt.Spri…
</details>
<details><summary>pkg/workflow/awf_helpers.go:274</summary>
**The `/opt/*` exclusion has no comment explaining why those paths skip explicit bind-mounting**, making it look like an arbitrary condition that could be "cleaned up" away.
<details>
<summary>💡 Suggested fix</summary>
The reason is that AWF is always invoked with `--enable-host-access` (line 615), and in chroot mode that makes the host filesystem — including `/opt/hostedtoolcache` — accessible inside the container without an explicit `--mount` bind. Paths outside `/opt/` (e.g. a self-hosted…
</details>
<details><summary>pkg/workflow/nodejs.go:193</summary>
**`find` now searches only `$RUNNER_TOOL_CACHE`, silently breaking self-hosted runners that have tools pre-cached at `/opt/hostedtoolcache` but set `RUNNER_TOOL_CACHE` to a different path.**
<details>
<summary>💡 Details</summary>
Before this PR, `GetNpmBinPathSetup()` searched three paths inside AWF:
```bash
find "$GH_AW_TOOL_CACHE" /opt/hostedtoolcache /home/runner/work/_tool ...After this PR, only one path is searched:
find "$GH_AW_TOOL_CACHE" ...On GitHub-hosted runner…
pkg/workflow/awf_config_test.go:1307
The /opt/* guard is never exercised by this test — removing or inverting the condition would leave the test suite green while GitHub-hosted runners silently receive a redundant bind-mount.
<details>
<summary>💡 Suggested assertion</summary>
The test sets RUNNER_TOOL_CACHE to a custom non-/opt/ path to verify the mount is emitted. Add a complementary subtest (or a second NotContains assertion here) that exercises the /opt/* branch where GH_AW_TOOL_CACHE_MOUNT must remain empty:…
gh-aw was guessing tool-cache locations when
RUNNER_TOOL_CACHEwas absent, including a hard-coded self-hosted runner path that assumes/home/runner/work/_tool. GitHub Actions already providesRUNNER_TOOL_CACHE, so generated commands should use that value rather than infer runner layout.RUNNER_TOOL_CACHEbefore searching for tool-cachebindirectories./opt/hostedtoolcacheand/home/runner/work/_tool.AWF mount probing
RUNNER_TOOL_CACHEto decide whether a non-/opttool cache needs a read-only bind mount._toolpath.Tests and generated expectations
✨ PR Review Safe Output Test - Run 27798181236
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.comSee Network Configuration for more information.