Prefer Actions toolcache Copilot CLI before release download#35992
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add more logging |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35992 does not have the 'implementation' label and has 64 new lines of code in business logic directories (≤100 threshold). No custom config present. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR updates the Copilot CLI installer to prefer an already-cached copilot-cli from the GitHub Actions toolcache (matching arch/version, and choosing the highest version for latest) before falling back to the existing verified release download + SHA256 validation flow.
Changes:
- Add toolcache discovery/activation logic to
actions/setup/sh/install_copilot_cli.shprior to any network download. - Keep the existing release tarball download + checksum verification as the cache-miss fallback.
- Add a Go unit test to validate the cache-hit path skips
curland exports the cached bin directory viaGITHUB_PATH.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/sh/install_copilot_cli.sh | Adds toolcache lookup/activation before the existing verified download fallback. |
| pkg/cli/install_copilot_cli_test.go | Adds regression coverage ensuring cache hits avoid downloads and export the cached path. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 6
| VERSION="${1:-}" | ||
| COPILOT_REPO="github/copilot-cli" | ||
| INSTALL_DIR="/usr/local/bin" | ||
| COPILOT_DIR="/home/runner/.copilot" | ||
| COPILOT_TOOLCACHE_MAX_DEPTH=4 |
| candidate_version="$(basename "$(dirname "$(dirname "$candidate_dir")")")" | ||
| candidate_version_normalized="$(normalize_version "$candidate_version")" | ||
|
|
| echo "Verifying cached Copilot CLI installation..." | ||
| if command -v copilot >/dev/null 2>&1; then | ||
| copilot --version | ||
| echo "✓ Copilot CLI installation complete" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "ERROR: Cached Copilot CLI activation failed - command not found" | ||
| exit 1 |
| VERSION="${1:-}" | ||
| COPILOT_REPO="github/copilot-cli" | ||
| INSTALL_DIR="/usr/local/bin" | ||
| COPILOT_DIR="/home/runner/.copilot" | ||
| COPILOT_TOOLCACHE_MAX_DEPTH=4 |
| candidate_version="$(basename "$(dirname "$(dirname "$candidate_dir")")")" | ||
| candidate_version_normalized="$(normalize_version "$candidate_version")" | ||
|
|
| echo "Verifying cached Copilot CLI installation..." | ||
| if command -v copilot >/dev/null 2>&1; then | ||
| copilot --version | ||
| echo "✓ Copilot CLI installation complete" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "ERROR: Cached Copilot CLI activation failed - command not found" | ||
| exit 1 |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
The single new test is an excellent end-to-end behavioral contract test. It exercises the real 📖 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 system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on correctness and test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Correctness risk: Activation failure exits
1instead of falling through to the download path, violating the PR's own "preserve existing behavior" goal. - Test coverage gaps: 4 of the 5 new code paths are untested (
latestresolution, arch filtering, cache miss, wrapper-install branch). - Build-tag mismatch: The test file spawns real shell processes but carries
!integration, which misclassifies it as a unit test. version_is_greateredge cases: Pre-release suffixes (1.2.3-beta) will cause bash10#arithmetic to fail; this is unverified by tests.
Positive Highlights
- ✅ Clean separation into focused helper functions (
find_cached_copilot_bin,activate_cached_copilot_bin, etc.) - ✅ Portable version comparison without
sort -V— good cross-platform awareness - ✅ Existing verified download + SHA256 path is left structurally unchanged
- ✅ Good use of
RUNNER_TOOL_CACHEwith sensible fallback roots
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M
|
|
||
| cached_copilot_dir="$(dirname "$cached_copilot_bin")" | ||
| export PATH="${cached_copilot_dir}:$PATH" | ||
|
|
There was a problem hiding this comment.
[/tdd] Cache-activation failure exits 1 instead of falling through to the download path — a corrupt or missing toolcache entry becomes a hard build failure rather than a graceful fallback.
💡 Suggestion
Change the hard exit 1 to a warn-and-fallthrough so the existing verified download path remains the safety net:
if CACHED_COPILOT_BIN="$(find_cached_copilot_bin "$REQUESTED_VERSION")"; then
echo "Using cached GitHub Copilot CLI from ${CACHED_COPILOT_BIN}"
activate_cached_copilot_bin "$CACHED_COPILOT_BIN"
if command -v copilot >/dev/null 2>&1; then
copilot --version
echo "✓ Copilot CLI installation complete"
exit 0
fi
echo "WARNING: Cached Copilot CLI activation failed — falling back to release download"
# fall through
fiThe PR description says "preserve existing behavior" — a hard exit 1 on activation failure violates that intent.
| @@ -0,0 +1,64 @@ | |||
| //go:build !integration | |||
There was a problem hiding this comment.
[/tdd] Build tag is !integration but the test spawns a real bash process and performs real filesystem I/O — these are integration-test characteristics by this repo's conventions.
💡 Suggestion
Change the build tag to //go:build integration so it runs with make test / -tags integration and does not inflate make test-unit runtime:
(go/redacted):build integrationIf you intentionally want it in the unit-test suite, add a brief comment explaining why.
| wd, err := os.Getwd() | ||
| require.NoError(t, err, "Failed to get working directory") | ||
|
|
||
| projectRoot := filepath.Join(wd, "..", "..") |
There was a problem hiding this comment.
[/tdd] Only the exact-version cache-hit path (with GITHUB_PATH set) is tested. Three important new behaviors have no coverage: latest picks the highest version, arch mismatch is skipped, and a cache miss falls through to the download.
💡 Suggested additional test cases
latestpicks highest version — create two cached entries (1.2.3and1.10.0), call the script with no version arg, assert1.10.0is activated.- Arch mismatch is filtered — place a binary under
arm64/bin/copilotwhile the runner resolves tox64; assertcurlis invoked (download path) and not the cache-hit message. - Cache miss falls through — empty toolcache dir; assert the script reaches the download step (e.g.
curlis invoked, or the script exits non-zero without emitting the cache-hit message).
| # Compare dotted numeric versions without relying on GNU-specific sort -V. | ||
| # Returns success only when the left version is strictly greater than the right version. | ||
| version_is_greater() { | ||
| local left="${1:-0}" |
There was a problem hiding this comment.
[/tdd] version_is_greater has no tests for edge inputs: single-component versions (1 vs 1.0), pre-release suffixes (1.2.3-beta), and leading-zero segments (08) where the 10# base coercion is load-bearing.
💡 Cases to verify
# Should be true
version_is_greater "1.10.0" "1.9.0" && echo ok || echo FAIL
# Should be false (equal)
version_is_greater "1.2.3" "1.2.3" && echo FAIL || echo ok
# Should be false (less)
version_is_greater "1.2.0" "1.2.3" && echo FAIL || echo ok
# Edge: pre-release suffix — 10# will fail: "10#3-beta" is a syntax error
version_is_greater "1.2.3-beta" "1.2.3"Pre-release suffixes cause a bash arithmetic error with 10#. Consider stripping the suffix with ${part%%-*} or documenting that pre-release versions are unsupported in the toolcache path.
| done | ||
|
|
||
| if [ -n "$best_candidate" ]; then | ||
| printf '%s\n' "$best_candidate" |
There was a problem hiding this comment.
[/tdd] The activate_cached_copilot_bin wrapper-install branch (when GITHUB_PATH is unset) calls sudo install but has zero test coverage — a regression here would go undetected.
💡 Suggestion
Add a test scenario that omits GITHUB_PATH from the environment, provides a writable INSTALL_DIR override, and asserts the wrapper file was created at the expected path. This is the only code path exercising the sudo install line.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs and a test coverage gap need addressing before this merges.
### Blocking issues (2)
-
Hard
exit 1on cache-activation failure (line 216) — a corrupted or non-executable cached binary causes a permanent CI failure with no self-healing. The download+verify path is never reached. Fix: fall through with a warning instead of exiting. -
findskips executable-permission check (line 164) — a non-executable toolcache entry passes thefindfilter, reachescopilot --version, fails withPermission denied, and hits the same hardexit 1. Fix: add-perm /111to thefindcall.
### Non-blocking (1)
- Test coverage covers only the GITHUB_PATH happy path; the
exit 1branch, the local-wrapper path, and the cache-miss fallthrough are all unexercised.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2M
| fi | ||
|
|
||
| echo "ERROR: Cached Copilot CLI activation failed - command not found" | ||
| exit 1 |
There was a problem hiding this comment.
Hard exit 1 on cache-activation failure breaks CI with no recovery: if command -v copilot fails after a cache hit (e.g., the cached binary is not executable), the script dies permanently — the download-and-verify path is never attempted.
💡 Suggested fix
Replace the hard exit 1 with a warning that falls through to the download path:
echo "WARNING: Cached Copilot CLI activation failed — falling back to download"
fi
# download path continues here...Any corrupted or permission-stripped toolcache entry currently causes a permanent CI failure on every run until the cache is manually purged. The download path already performs SHA256 verification, so there is no safety tradeoff.
| best_candidate="$candidate" | ||
| best_version="$candidate_version_normalized" | ||
| fi | ||
| done < <(find "${tool_cache_root}/copilot-cli" -maxdepth "${COPILOT_TOOLCACHE_MAX_DEPTH}" -type f -path '*/bin/copilot' 2>/dev/null) |
There was a problem hiding this comment.
find does not check execute permission — a non-executable cached binary causes hard failure at line 210: copilot --version will produce a confusing Permission denied error and the script will exit 1 via the activation-failure path rather than falling through to a fresh download.
💡 Suggested fix
Add -perm /111 to the find invocation:
done < <(find "${tool_cache_root}/copilot-cli" \
-maxdepth "${COPILOT_TOOLCACHE_MAX_DEPTH}" \
-type f \
-perm /111 \
-path '*/bin/copilot' 2>/dev/null)This is especially relevant on self-hosted runners where toolcache entries may have been extracted without preserving execute bits.
|
|
||
| githubPathContent, err := os.ReadFile(githubPath) | ||
| require.NoError(t, err, "Expected the script to append the cached bin dir to GITHUB_PATH") | ||
| assert.Contains(t, string(githubPathContent), toolcacheBin, "cached Copilot bin directory should be exported for later steps") |
There was a problem hiding this comment.
Test only covers the happy-path GITHUB_PATH branch; the cache-activation-failure path is untested: the test sets GITHUB_PATH, so activate_cached_copilot_bin always returns after writing to it. The exit 1 branch (line 216 in the script) is never exercised, and neither is the local-wrapper path used when GITHUB_PATH is absent.
💡 Missing test cases
- Activation failure → hard exit: call the script with
RUNNER_TOOL_CACHEpointing at a toolcache entry whosecopilotbinary is not executable (mode0644); assert the script exits non-zero. - Wrapper path (no GITHUB_PATH): call the script without
GITHUB_PATHset and verify a wrapper is installed toINSTALL_DIR. - Cache miss → falls through to download: a toolcache with no
copilotentry should attemptcurl(assertcurl.logexists).
Without case 1 in particular, the hard-exit behavior at line 216 is both untested and — as raised in another comment — likely wrong.
There was a problem hiding this comment.
Me agree. Wrapper-path and failure-path coverage still missing here.
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.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 12.3M
There was a problem hiding this comment.
Me agree. Wrapper-path and failure-path coverage still missing here.
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.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 12.3M
|
@copilot review all comments and reviews from copilot and GitHub-actions actor. Resolve. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added logging in two commits:
|
|
@copilot merge main and recompile |
…ions-toolcache-lookup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
💫 TO BE CONTINUED... Smoke Claude failed to deliver outputs! Our hero faces unexpected challenges... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test Results:
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Commit pushed:
|
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.
|
💥 Smoke Test: Claude — Run 26698359909Core #1-13: 1✅ 2❌(mcpscripts-gh unknown→alt✅) 3✅ 4✅ 5✅ 6✅ 7✅ 8✅ 9✅ 10✅ 11✅ 12✅ 13✅ Overall: PARTIAL 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.
💥 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 · opus48 887.5K
There was a problem hiding this comment.
Smoke review done. Build good. Two small risk spots on executable filtering and wrapper-path coverage.
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.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 12.3M
| best_candidate="$candidate" | ||
| best_version="$candidate_version_normalized" | ||
| fi | ||
| done < <(find "${tool_cache_root}/copilot-cli" -maxdepth "${COPILOT_TOOLCACHE_MAX_DEPTH}" -type f -path '*/bin/copilot' 2>/dev/null) |
There was a problem hiding this comment.
Small trap here. find may pick non-executable cached binary. Add -perm /111 so bad cache falls through clean.
| cmd := exec.Command("bash", installScript, "1.2.3") | ||
| cmd.Env = append(os.Environ(), | ||
| "RUNNER_TOOL_CACHE="+filepath.Join(tempDir, "toolcache"), | ||
| "GITHUB_PATH="+githubPath, |
There was a problem hiding this comment.
Test walks only GITHUB_PATH road. Missing wrapper path when GITHUB_PATH is absent, so sudo install branch can break unseen.
| best_candidate="$candidate" | ||
| best_version="$candidate_version_normalized" | ||
| fi | ||
| done < <(find "${tool_cache_root}/copilot-cli" -maxdepth "${COPILOT_TOOLCACHE_MAX_DEPTH}" -type f -path '*/bin/copilot' 2>/dev/null) |
There was a problem hiding this comment.
Small trap here. find may pick non-executable cached binary. Add -perm /111 so bad cache falls through clean.
| cmd := exec.Command("bash", installScript, "1.2.3") | ||
| cmd.Env = append(os.Environ(), | ||
| "RUNNER_TOOL_CACHE="+filepath.Join(tempDir, "toolcache"), | ||
| "GITHUB_PATH="+githubPath, |
There was a problem hiding this comment.
Test walks only GITHUB_PATH road. Missing wrapper path when GITHUB_PATH is absent, so sudo install branch can break unseen.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Run: https://github.com/github/gh-aw/actions/runs/26698549505
|
|
Run: https://github.com/github/gh-aw/actions/runs/26698549505
|
This change makes the Copilot installer use a compatible
copilot-clialready present in the Actions toolcache before falling back to the current release-download path. The goal is to avoid unnecessary network fetches while preserving the existing verified install flow when no cache entry is available.Installer behavior
actions/setup/sh/install_copilot_cli.shbefore any download attempt.latest, select the highest compatible cached version.Cache activation
bindirectory toPATH.GITHUB_PATHfor downstream steps.copilotresolves consistently without copying the cached binary.Fallback semantics
Regression coverage
curland exposes the cached binary correctly.Changeset
✨ PR Review Safe Output Test - Run 26698359909
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.