Harden Copilot CLI release installation against transient CDN failures - #53112
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #53112 diff for over-engineering (ponytail-review). Changes are minimal and targeted: bounded curl retries, dev-build compatibility branch in jq, and a small regex tweak, plus one smoke test. No speculative abstractions, dead code, or reinventable stdlib patterns found. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 19 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Hardens Copilot CLI installation and enables compatibility resolution for development builds.
Changes:
- Adds bounded retries to release downloads.
- Maps
devbuilds to the open compatibility row. - Adds regression coverage for compatibility and retry flags.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/install_copilot_cli.sh |
Updates compatibility resolution and download retries. |
pkg/cli/install_copilot_cli_test.go |
Tests development compatibility and retry configuration. |
Review details
Suppressed comments (1)
actions/setup/sh/install_copilot_cli.sh:575
- The binary download has the same gap:
--retryalone will not retry curl exit 35, so a TLS/SSL connection reset can still fail installation immediately. Include--retry-all-errorshere as well and update the corresponding test assertion.
curl -fsSL --retry 5 --retry-delay 2 --retry-max-time 60 -o "${TEMP_DIR}/${TARBALL_NAME}" "${TARBALL_URL}"
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (semver_cmp($compiled; $min_aw) >= 0) and | ||
| (($max_aw == "*") or (semver_cmp($compiled; $max_aw) <= 0)) then | ||
| # Use the open row for development builds, which do not have a semver release tag. | ||
| if (($compiled_version == "dev") and ($row.open == true)) or |
| # Download checksums | ||
| echo "Downloading checksums from ${CHECKSUMS_URL}..." | ||
| curl -fsSL --retry 3 --retry-delay 5 -o "${TEMP_DIR}/SHA256SUMS.txt" "${CHECKSUMS_URL}" | ||
| curl -fsSL --retry 5 --retry-delay 2 --retry-max-time 60 -o "${TEMP_DIR}/SHA256SUMS.txt" "${CHECKSUMS_URL}" |
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking issues found in the changed lines.
Why this stays non-blocking
I checked the new dev compatibility-matrix path and the bounded curl retry changes against the affected shell logic and tests. The shell change keeps release-tag semver comparisons on the existing path, gates dev builds onto open: true rows only, and the added test coverage exercises both the dev toolcache path and the exact retry flags for release downloads.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.2 AIC · ⌖ 5.57 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
The changes look correct and well-tested.
- curl retry hardening: Bumping
--retryfrom 3 to 5 and adding--retry-max-time 60is a good improvement. The shorter--retry-delay 2(vs 5) means faster recovery on short blips; the total-time cap prevents runaway retries during extended outages. - dev build support: The
open: truecompat row path is a clean way to handle development builds that have no semver tag. The condition logic inresolve_compat_with_jqand the guard inresolve_version_from_compatare both correct. - Tests: The new string-match test for curl args is a lightweight but effective regression guard. Updating the existing test to use
devas the compiled version exercises the new code path end-to-end.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20 AIC · ⌖ 7.06 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — changes are well-scoped and the root causes (hard CDN failure and dev-build version skip) are correctly addressed. Approving with two minor observations.
📋 Key Themes & Highlights
Key Themes
- Retry strategy: The move from
--retry 3 --retry-delay 5to--retry 5 --retry-delay 2 --retry-max-time 60is a good improvement. The 60-second budget is the right primary control; the fixed 2-second gap is a minor trade-off (noted inline). - Dev-build compat resolution: The
open: truerow lookup is a clean, explicit special-case rather than a semver hack. The guard inresolve_version_from_compatcorrectly allowsdevthrough without touching the regex path. - Test type for retry: The new test is a source-level assertion, not a live execution test (noted inline) — acceptable as a quick guard but worth a comment about its scope.
Positive Highlights
- ✅ Surgical changes — only the two affected code paths are touched
- ✅ Dev-build path is tested end-to-end via the existing integration harness (toolcache scenario)
- ✅ Both download sites (checksums + tarball) consistently hardened — no asymmetry
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.2 AIC · ⌖ 8.6 AIC · ⊞ 7.7K
Comment /matt to run again
| assert.Equal(t, 1, compatFetches, "compat.json should be fetched exactly once (no double fallback)") | ||
| } | ||
|
|
||
| func TestInstallCopilotCLIScriptUsesBoundedRetriesForReleaseDownloads(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] This test is a static string-scan of the script rather than a live execution — it validates that the flags are present in the source, but not that curl actually receives them at runtime. If the variable substitution or quoting around the flags ever changes, the substring check could pass while the real retry behaviour silently regressed.
💡 Suggestion
Consider adding a comment noting the test's scope:
// Static check: confirms the bounded-retry flags are present in the script source.
// For runtime confidence, consider an integration test with a curl stub returning 503s.An integration test using a curl stub returning configurable HTTP errors (like the existing fake-curl pattern) would give much stronger assurance that the retry budget is actually exercised end-to-end.
@copilot please address this.
| # Download checksums | ||
| echo "Downloading checksums from ${CHECKSUMS_URL}..." | ||
| curl -fsSL --retry 3 --retry-delay 5 -o "${TEMP_DIR}/SHA256SUMS.txt" "${CHECKSUMS_URL}" | ||
| curl -fsSL --retry 5 --retry-delay 2 --retry-max-time 60 -o "${TEMP_DIR}/SHA256SUMS.txt" "${CHECKSUMS_URL}" |
There was a problem hiding this comment.
[/diagnosing-bugs] --retry-delay 2 sets a fixed inter-attempt delay with no exponential back-off. Against a CDN that is shedding load under pressure, a flat 2-second gap between five rapid retries may not give the origin enough breathing room and all attempts could still hit the same congestion window.
💡 Suggestion
curl supports exponential backoff via --retry-delay 0 (lets curl pick a delay) or the newer --retry-all-errors flag, but true exponential backoff requires a wrapper. If the 60-second budget is the primary guard, consider documenting this trade-off in a comment:
# --retry-max-time 60 caps total wall-clock spend; --retry-delay 2 is a flat gap.
# For true exponential backoff a shell loop with sleep 2^n would be needed.
curl -fsSL --retry 5 --retry-delay 2 --retry-max-time 60 ...This is a minor concern given the 60-second budget, but worth noting for future readers.
@copilot please address this.
|
@copilot PR #53112 still has a failing check to address. Quick triage
Next step
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Copilot CLI installation could fail on a single GitHub Releases connection reset. Development builds also skipped compatibility resolution, unnecessarily bypassing compatible toolcache entries.
Bounded release retries
Development-build compatibility resolution
GH_AW_COMPILED_VERSION=devthrough the compatibility matrix’s open Copilot row.Coverage
devcompatibility resolution with a cached compatible CLI.Run: https://github.com/github/gh-aw/actions/runs/31950301208> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.5 AIC · ⌖ 5.77 AIC · ⊞ 8.7K · ◷