fix: harden unauthenticated GitHub API fallbacks with context and 30s timeout#42521
Conversation
…eout
- resolveRefToSHAViaPublicAPI: add ctx parameter, use NewRequestWithContext + timeout client
- fetchPublicGitHubContentsAPI: add ctx parameter, use NewRequestWithContext + timeout client
- Thread ctx through immediate wrappers: downloadFileViaPublicAPI, listDirAllFilesViaPublicAPI, listDirSubdirsViaPublicAPI, listWorkflowFilesViaPublicAPI
- fetchPublicGitHubAPI: replace http.DefaultClient with &http.Client{Timeout: DefaultHTTPClientTimeout}
All callers that lack a context pass context.Background(), consistent with
the existing pattern (e.g. downloadFileViaGit calls on line 901/921).
Fixes #42500
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…backs Avoid allocating a new http.Client on every call by introducing two package-level variables: - parser.publicAPIClient (remote_fetch.go) — shared by resolveRefToSHAViaPublicAPI and fetchPublicGitHubContentsAPI - cli.updatePublicAPIClient (update_workflows.go) — used by fetchPublicGitHubAPI Both carry DefaultHTTPClientTimeout (30s). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (30 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
This PR hardens unauthenticated api.github.com fallback requests in gh-aw to avoid indefinite hangs during workflow compilation/update resolution by ensuring all such HTTP calls are context-aware and use a shared client with a 30s timeout.
Changes:
- Introduces shared
http.Clientinstances withconstants.DefaultHTTPClientTimeoutfor public GitHub API fallbacks. - Threads
context.Contextinto the parser’s public-API fallback helpers and switches requests tohttp.NewRequestWithContext. - Updates the update-workflows unauthenticated API path to stop using
http.DefaultClientand instead use the timeout-configured client.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/remote_fetch.go | Adds a shared timeout HTTP client and propagates context.Context through unauthenticated public-API fallback calls. |
| pkg/cli/update_workflows.go | Replaces http.DefaultClient usage in unauthenticated fallback calls with a shared timeout HTTP client. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review summary
The core fix is correct and well-targeted. The two package-level HTTP clients (publicAPIClient in pkg/parser, updatePublicAPIClient in pkg/cli) both carry constants.DefaultHTTPClientTimeout (30 s), which is the right way to bound unauthenticated GitHub API calls that previously ran through http.DefaultClient with no timeout.
What the fix gets right:
- All three previously-unbounded paths (
resolveRefToSHAViaPublicAPI,fetchPublicGitHubContentsAPI,fetchPublicGitHubAPI) now have a hard 30 s timeout. fetchPublicGitHubAPIalready hadctxthreaded but usedhttp.DefaultClient— that oversight is now closed.- Switching to
http.NewRequestWithContextis the correct API for context-aware requests.
Non-blocking observation (see inline comment):
All five call sites in pkg/parser/remote_fetch.go pass context.Background() because the intermediate callers (resolveRefToSHA, downloadFileFromGitHubWithDepth, etc.) have no ctx parameter. The ctx argument to http.NewRequestWithContext is therefore a no-op; the client-level timeout is the real hang protection. This is consistent with the pre-existing downloadFileViaGit(context.Background(), ...) pattern in the same file, but propagating ctx through those intermediate functions would be a valuable follow-up.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 42.3 AIC · ⌖ 6.75 AIC · ⊞ 4.9K
| if host == "" || host == "github.com" { | ||
| remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(context.Background(), owner, repo, ref) |
There was a problem hiding this comment.
The five fallback call sites are all forced to use context.Background() because the intermediate callers (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, listDirAllFilesForHost, listDirSubdirsForHost) have no ctx parameter. The 30 s client-level timeout is the real hang protection here — the ctx passed to http.NewRequestWithContext carries no deadline or cancellation signal from the outer call stack. As a follow-up, propagating ctx through those intermediate functions would allow callers to cancel in-flight requests via their own context deadlines. @copilot please address this.
There was a problem hiding this comment.
The 30 s timeout is real — the context threading is not
The http.Client{Timeout: 30s} additions in both files are a genuine improvement and the right direction. The updatePublicAPIClient fix in update_workflows.go is clean and complete.
The blocking concern is in remote_fetch.go: the ctx parameters were added to the five inner ViaPublicAPI functions, but all five outer callers (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, listDirAllFilesForHost, listDirSubdirsForHost) are context-free and pass context.Background(). The context refactor is scaffolding with no current effect on cancellation — a Ctrl-C or a caller deadline still can't interrupt these HTTP calls. Thread ctx through the outer functions to complete the fix.
Two secondary concerns are noted inline: duplicate HTTP client vars across packages (future tuning gap) and mutable package-level var (test-pollution risk).
🔎 Code quality review by PR Code Quality Reviewer · 76.8 AIC · ⌖ 7.45 AIC · ⊞ 1.6K
Comment /review to run again
| if host == "" || host == "github.com" { | ||
| remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(context.Background(), owner, repo, ref) |
There was a problem hiding this comment.
Incomplete context threading: context.Background() silently drops all caller cancellation signals at every public-API fallback site.
💡 Details and suggested fix
Five call sites in this file (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, listDirAllFilesForHost, listDirSubdirsForHost) pass context.Background() because the outer functions have no ctx parameter. This means:
- Ctrl-C / OS signal → context cancelled at the CLI layer → the HTTP request in
publicAPIClient.Do(req)continues for up to 30 s regardless. - Any caller with a short deadline (e.g., a test with
t.Context()) has that deadline silently discarded. - The
Timeoutfield onpublicAPIClientis the only real protection; the newctxparameters are scaffolding with no current effect on cancellation.
Complete the threading by adding ctx to the outer functions:
// was: func resolveRefToSHA(owner, repo, ref, host string) (string, error)
func resolveRefToSHA(ctx context.Context, owner, repo, ref, host string) (string, error) {
// ...
return resolveRefToSHAViaPublicAPI(ctx, owner, repo, ref) // real ctx, not context.Background()
}Same pattern for the other four outer wrappers. Without this, the PR description overstates the change — context is not actually threaded, only a 30 s client-level timeout is added.
| // updatePublicAPIClient is a shared HTTP client used for unauthenticated GitHub | ||
| // API fallback calls in the update workflow path. It carries a timeout to | ||
| // prevent indefinite hangs on slow or unresponsive hosts. | ||
| var updatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
Duplicate HTTP client declarations across two packages create a silent maintenance trap.
💡 Details
updatePublicAPIClient (here) and publicAPIClient (pkg/parser/remote_fetch.go:36) are structurally identical:
&http.Client{Timeout: constants.DefaultHTTPClientTimeout}When the unauthenticated API path eventually needs tuning — custom Transport for proxy support, different timeout per environment, retry middleware — the change will be applied to one and silently missed in the other. A shared constructor (or a single exported client) in a common internal/httpclient or constants package would close this gap:
// e.g., in pkg/constants or pkg/internal/httpclient
func NewPublicAPIClient() *http.Client {
return &http.Client{Timeout: DefaultHTTPClientTimeout}
}Both call sites then call httpclient.NewPublicAPIClient(), giving a single place to evolve the configuration.
| // publicAPIClient is a shared HTTP client used for unauthenticated GitHub API | ||
| // fallback calls. It carries a timeout to prevent indefinite hangs on slow or | ||
| // unresponsive hosts. | ||
| var publicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
Mutable package-level var for the HTTP client enables test-state pollution.
💡 Details
publicAPIClient is a plain var, which means any in-package test can overwrite it:
// In a test file
func TestSomething(t *testing.T) {
publicAPIClient = &http.Client{Transport: mockTransport}
// if this test panics before cleanup, next test picks up mockTransport
}If a test fails before restoring the original value (or uses t.Parallel()), subsequent tests silently use the wrong transport. The standard Go mitigation is to save and restore via t.Cleanup, but that requires discipline across every future test that needs to inject a mock. A safer pattern is a function-level or injected client:
// Unexported sentinel; set once at init, replaced in tests via a setter
var publicAPIClient = newPublicAPIClient()
func newPublicAPIClient() *http.Client {
return &http.Client{Timeout: constants.DefaultHTTPClientTimeout}
}Tests swap via the constructor rather than mutating the global, which makes the scope of the change explicit and t.Cleanup-friendly.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes primarily for the missing regression test.
📋 Key Themes & Highlights
Key Themes
- Missing regression test (blocking): The goroutine-hang fix has no test. If a future change reverts to
http.DefaultClient, the bug silently returns. A singlehttptest.NewServer-based test would lock this in permanently. context.Background()at call sites: All 5 new call sites passcontext.Background()because parent functions lack actxparameter. Correct for the current scope, but worth tracking for future context-propagation work.- Duplicate HTTP client config:
publicAPIClientandupdatePublicAPIClientare identical — low risk now, but could diverge silently.
Positive Highlights
- ✅ Surgical fix: minimal diff, clear intent, all relevant fallback paths covered consistently
- ✅
http.NewRequestWithContextused correctly in every new request - ✅ Comments on the new package-level vars clearly explain the timeout rationale
- ✅ Call sites that lack ambient context correctly pass
context.Background(), matching the existing pattern in the file - ✅ Centralising the client in a package-level var (rather than per-call) is the right call for a shared timeout
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 54.9 AIC · ⌖ 9.13 AIC · ⊞ 6.5K
Comment /matt to run again
| // publicAPIClient is a shared HTTP client used for unauthenticated GitHub API | ||
| // fallback calls. It carries a timeout to prevent indefinite hangs on slow or | ||
| // unresponsive hosts. | ||
| var publicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
[/diagnosing-bugs + /tdd] No regression test guards this fix — if a future change silently reverts to http.DefaultClient, the hang will come back undetected.
💡 Suggested test skeleton
A test using httptest.NewServer with a slow handler and a short-timeout client would confirm the fix:
func TestPublicAPIClientTimeout(t *testing.T) {
slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
time.Sleep(2 * time.Second)
}))
defer slow.Close()
orig := publicAPIClient
publicAPIClient = &http.Client{Timeout: 100 * time.Millisecond}
t.Cleanup(func() { publicAPIClient = orig })
_, err := fetchPublicGitHubContentsAPI(context.Background(), "owner", "repo", "path", "ref")
if err == nil {
t.Fatal("expected timeout error, got nil")
}
}This acts as a living specification: any regression that swaps back to http.DefaultClient fails immediately.
@copilot please address this.
| if host == "" || host == "github.com" { | ||
| remoteLog.Printf("Git fallback also failed, attempting unauthenticated API for %s/%s@%s", owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(owner, repo, ref) | ||
| return resolveRefToSHAViaPublicAPI(context.Background(), owner, repo, ref) |
There was a problem hiding this comment.
[/diagnosing-bugs] All five new context.Background() call sites forfeit context cancellation — if a caller later gains a deadline (e.g. a per-request context from a CLI command), the HTTP call will not respect it; the 30 s timeout is the only guard.
💡 Why this matters and what to consider
The parent functions (resolveRefToSHA, downloadFileFromGitHubWithDepth, listWorkflowFilesForHost, etc.) currently have no context.Context parameter, so context.Background() is the only viable option right now. This PR is correct as-is, but leaves a TODO in the door:
- Short term: add a code comment at each call site, e.g.
// TODO(context): pass caller ctx once parent accepts one - Longer term: thread
ctxthrough the parent functions so cancellation signals from the CLI propagate end-to-end
Without this, a user cancelling a long-running gh aw compile will cancel everything except the last-resort public API fallbacks, which must wait for the full 30 s.
@copilot please address this.
| // updatePublicAPIClient is a shared HTTP client used for unauthenticated GitHub | ||
| // API fallback calls in the update workflow path. It carries a timeout to | ||
| // prevent indefinite hangs on slow or unresponsive hosts. | ||
| var updatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
[/codebase-design] updatePublicAPIClient is a second package-level HTTP client with the same configuration as publicAPIClient in pkg/parser/remote_fetch.go. If the timeout value ever needs to change (e.g. for a shorter default), both declarations must be updated independently.
💡 Consider consolidating
Both clients are: &http.Client{Timeout: constants.DefaultHTTPClientTimeout}. The duplication is currently low-risk because constants.DefaultHTTPClientTimeout is the single source of truth for the value. However, the two clients could diverge in future (e.g. one gets headers, retries, or a transport).
A shared constructor or a single pkg/httputil package-level var would make the intent clearer and reduce the divergence risk. For now, a comment linking the two declarations would also help:
// See also publicAPIClient in pkg/parser/remote_fetch.go — same config.
var updatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}@copilot please address this.
|
@copilot run pr-finisher skill |
🤖 PR Triage
Score breakdown: Impact 25/50 · Urgency 12/30 · Quality 3/20 Rationale: Fixes goroutine hang in unauthenticated GitHub API paths by adding 30s timeout. Currently blocked by
|
Ran the pr-finisher pass.
Actions taken: executed triage + local validation and confirmed no additional code changes were required for this requested pass. |
|
🎉 This pull request is included in a new release. Release: |
Three unauthenticated
api.github.comfallback paths dispatched viahttp.DefaultClient(no timeout), allowing a slow GitHub response to hang a goroutine indefinitely during workflow compilation.fetchPublicGitHubAPIalready threadedcontext.Contextbut still usedhttp.DefaultClient.Changes
pkg/parser/remote_fetch.gopublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}(30s) shared across all public-API fallback calls.resolveRefToSHAViaPublicAPI— addsctx context.Contextparameter; switches tohttp.NewRequestWithContext+publicAPIClient.fetchPublicGitHubContentsAPI— same fix; threadsctxthrough all four immediate callers:downloadFileViaPublicAPI,listDirAllFilesViaPublicAPI,listDirSubdirsViaPublicAPI,listWorkflowFilesViaPublicAPI.context.Background(), matching the existing pattern already used in this file.pkg/cli/update_workflows.goupdatePublicAPIClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}.fetchPublicGitHubAPI— replaces the lingeringhttp.DefaultClient.Do(req)withupdatePublicAPIClient.Do(req).