Thread context.Context through 8 public remote_fetch.go APIs to fix Ctrl-C cancellation#43128
Conversation
…lers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…h call chain Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates gh-aw’s remote GitHub fetch pathways to propagate context.Context end-to-end, so CLI invocations can pass cmd.Context() and enable cancellation (e.g., Ctrl-C) through remote workflow/resource/include/package resolution code paths.
Changes:
- Add
ctx context.Contextas the first parameter to 8 publicpkg/parser/remote_fetch.goAPIs and thread it through helper call chains, replacing priorcontext.Background()usage in those chains. - Update CLI call sites to pass
cmd.Context()(and tests to passt.Context()/ update injected function types) across includes/resources/dispatch/package-manifest and list-workflows flows. - Adjust tests and dependency-injection hooks (mock function signatures) to account for the new context parameter.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/remote_fetch.go | Adds context.Context to exported remote fetch APIs and propagates it through helper paths/fallbacks. |
| pkg/parser/remote_fetch_integration_test.go | Updates integration tests to pass t.Context() into parser fetch helpers. |
| pkg/cli/update_manifest.go | Threads ctx into repository package resolution during manifest updates. |
| pkg/cli/update_manifest_test.go | Updates mocked remote functions to accept context.Context. |
| pkg/cli/resources.go | Threads ctx into default-branch lookup and remote resource downloads. |
| pkg/cli/remote_workflow_test.go | Updates injected SHA resolver and include fetching calls to accept/pass context.Context. |
| pkg/cli/list_workflows_command.go | Passes cmd.Context() into RunListWorkflows and remote listing helper. |
| pkg/cli/list_workflows_command_test.go | Updates tests to pass t.Context() into RunListWorkflows. |
| pkg/cli/includes.go | Adds ctx to include/frontmatter import/include fetching and propagates it through recursion. |
| pkg/cli/fetch.go | Passes ctx into remote download + ref-to-SHA resolution calls. |
| pkg/cli/dispatch.go | Extends injected downloader signature to accept ctx, and threads it into dispatch workflow fetches. |
| pkg/cli/add_workflow_resolution.go | Threads ctx into repository package resolution during workflow resolution. |
| pkg/cli/add_package_manifest.go | Threads ctx through repository package manifest loading and scanning helpers/wrappers. |
| pkg/cli/add_package_manifest_test.go | Updates package resolution tests/mocks to accept context.Context and pass t.Context(). |
| pkg/cli/add_command.go | Threads ctx into parsed-file dispatch workflow discovery call. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Low
| // Parameters that remain constant across all recursion levels (in opts): | ||
| // - ctx: context for cancellation and deadline propagation | ||
| // - owner, repo, ref: source repository coordinates | ||
| // - originalBaseDir: directory of the top-level workflow (used to map remote paths → local paths) | ||
| // - targetDir: the `.github/workflows` directory in the user's repo | ||
| // - seen: shared visited set (keyed by fully-resolved remote path) — prevents cycles & duplicates |
| func downloadFileFromGitHubWithDepth(ctx context.Context, owner, repo, path, ref string, symlinkDepth int, host string) ([]byte, error) { | ||
| client, err := createRESTClientForHost(host) | ||
| if err != nil { | ||
| if gitutil.IsAuthError(err.Error()) { | ||
| remoteLog.Printf("REST client creation failed due to auth error, attempting git fallback for %s/%s/%s@%s: %v", owner, repo, path, ref, err) | ||
| content, gitErr := downloadFileViaGit(context.Background(), owner, repo, path, ref, host) | ||
| content, gitErr := downloadFileViaGit(ctx, owner, repo, path, ref, host) |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ 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. |
…remote_fetch APIs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (318 new lines across 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (28 modified tests, 0 new tests)
Changes SummaryAll test modifications are purely mechanical refactors to adapt to the new
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100 — Excellent. 0% implementation tests (threshold: 30%). All 28 modified tests are behavioral design tests. Zero guideline violations. Changes are purely mechanical context-threading refactors with no coverage regression.
🤖 PR Triage
Score Breakdown: Impact 40 + Urgency 22 + Quality 16 Rationale: Threads Recommended Action: Expedited human review — broad signature change needs careful validation of all call sites.
|
There was a problem hiding this comment.
REQUEST_CHANGES — ctx propagation is incomplete on the primary execution paths
The PR threads context.Context through 8 public API signatures and all their callers, which is the right direction. However, ctx does not reach the code that actually performs blocking I/O on the common paths, so Ctrl-C cancellation — the stated goal — is not achieved in practice.
Blocking issues (must fix before merge)
HIGH: listDirAllFilesRecursivelyForHost — ctx is a dead parameter (see inline comment at line 1217)
ctx is accepted but none of the three execution paths inside the function forward it to any callee. This is the only listing function with no public-API fallback, so ctx is provably unreachable in every branch. Recursive skill-directory listing is fully uncancellable.
HIGH: fetchRemoteFileContent has no ctx — primary file download path uncancellable (see inline comment at line 926)
The hot path — every downloadFileFromGitHubWithDepth call on an authenticated repo — calls fetchRemoteFileContent(client, ...) which has no ctx parameter and calls client.Get directly. The fallback branches are all ctx-aware, which creates a misleading impression of correctness.
MEDIUM: resolveRefToSHA and *ViaGitForHost helpers drop ctx (see inline comments at lines 528, 1051)
Four places use exec.Command instead of exec.CommandContext, and gh.Exec has no context support. These are secondary paths, but they represent the same category of incompleteness.
The caller-side threading work (all the pkg/cli/ changes) is solid — the issue is entirely inside pkg/parser/remote_fetch.go.
🔎 Code quality review by PR Code Quality Reviewer · 224.9 AIC · ⌖ 5.92 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/parser/remote_fetch.go:1217
Dead ctx parameter: cancellation has zero effect in listDirAllFilesRecursivelyForHost.
<details>
<summary>Details and fix</summary>
ctx is accepted at the signature but never forwarded to any callee inside the function body. All three execution paths skip it:
- Early-return path (REST client creation failure): calls
listDirAllFilesRecursivelyViaGitForHost(owner, repo, ref, dirPath, host)— noctx - Primary REST path: calls `listContentsRecursively(client, owner, repo, ref, dirP…
pkg/parser/remote_fetch.go:926
ctx does not reach client.Get — the primary download path remains uncancellable.
<details>
<summary>Details and fix</summary>
downloadFileFromGitHubWithDepth accepts ctx and threads it to resolveRefToSHA and listWorkflowFilesForHost, but the actual file-content download on line 926 calls:
fetchRemoteFileContent(client, owner, repo, path, ref, &fileContent)fetchRemoteFileContent has no ctx parameter and calls client.Get(...) directly — a synchronous HTTP call…
pkg/parser/remote_fetch.go:528
resolveRefToSHA primary path ignores ctx — gh.Exec and the git subprocess cannot be cancelled.
<details>
<summary>Details and fix</summary>
resolveRefToSHA accepts ctx but on the primary branch (authenticated, non-public) the only ctx-aware callee is resolveRefToSHAViaPublicAPI — a last-resort fallback reached only after both gh.Exec and resolveRefToSHAViaGit have already failed:
if err := gh.Exec(args...); err != nil { // no ctx
if shaBuf, gitErr…
</details>
<details><summary>pkg/parser/remote_fetch.go:1051</summary>
**`ctx` silently dropped before git fallback in `listWorkflowFilesForHost` (and similarly in `listDirAllFilesForHost` and `listDirSubdirsForHost`).**
<details>
<summary>Details and fix</summary>
This is a recurring pattern across three listing functions:
| Caller | Git-fallback callee | Line (approx) |
|---|---|---|
| `listWorkflowFilesForHost` | `listWorkflowFilesViaGitForHost(owner, repo, ref, path, host)` | ~1051 |
| `listDirAllFilesForHost` | `listDirAllFilesViaGitForHost(owner, repo, re…
</details>There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 158.2 AIC · ⌖ 5.94 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/parser/remote_fetch.go:1226
Non-blocking suggestion: listContentsRecursively (and listContentsRecursivelyWithDepth) do not accept a context.Context, so the ctx threaded into listDirAllFilesRecursivelyForHost is silently dropped before the recursive Contents API calls.
While api.RESTClient.Get() does not yet support context, adding ctx to these helpers now keeps the propagation chain consistent and prepares the code for when go-gh adds context support:
func listContentsRecursively(ctx context.Cont…
</details>
<details><summary>pkg/parser/remote_fetch.go:690</summary>
**Non-blocking suggestion:** `downloadFileViaGitClone` does not accept a `context.Context`, so the git-clone subprocess it spawns cannot be interrupted on Ctrl-C. `downloadFileViaGit` (which has ctx) calls it after `git archive` fails, silently dropping cancellation support for the clone fallback path.
Consider adding `ctx` and using `exec.CommandContext` for the clone/checkout commands:
```go
func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host string) ([]byte, erro…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with observations and improvement suggestions (no blocking issues found).
📋 Key Themes & Highlights
Key Themes
-
Test coverage gap — the fix is not verified end-to-end. All 15 changed files correctly thread
ctxthrough the call chains, but no test asserts that a cancelled context actually cancels an in-flight operation. The tests updated in this PR only swapcontext.Background()fort.Context()—t.Context()is never cancelled in any of those tests. See inline comments onadd_package_manifest_test.go,list_workflows_command.go, andremote_fetch.go. -
Remaining
context.Background()gap is acknowledged but untracked.downloadIncludeFromWorkflowSpec/resolveWorkflowSpecSHAForCachestill hard-codecontext.Background(). The comment is helpful, but without a linked issue or skipped specification test, this gap could persist indefinitely. See inline comment onremote_fetch.go:352. -
spec.Versionside-mutation on cancel.fetchAndSaveRemoteFrontmatterImportswrites back tospec.Versionbefore the download is attempted. This was pre-existing, but threading ctx made the pattern more visible. See inline comment onincludes.go:140.
Positive Highlights
- ✅ Excellent mechanical discipline: every private helper also receives
ctx— no partial propagation. - ✅ The
fileDownloadFntype signature update is consistent with the new contract. - ✅ Tests use
t.Context()throughout, which is the correct modern Go idiom. - ✅ The scope note in the PR description is clear and honest about the remaining gap.
- ✅ Comment on
downloadIncludeFromWorkflowSpecexplains thecontext.Background()usage — good documentation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 147.1 AIC · ⌖ 5.86 AIC · ⊞ 6.6K
Comment /matt to run again
| // NOTE: This function is called from ResolveIncludePath which has no context.Context | ||
| // parameter. Threading ctx through ResolveIncludePath and its 6+ callers across multiple | ||
| // packages is tracked as a follow-up task; context.Background() is used in the interim. | ||
| func downloadIncludeFromWorkflowSpec(spec string, cache *ImportCache) (string, error) { |
There was a problem hiding this comment.
[/tdd] downloadIncludeFromWorkflowSpec / resolveWorkflowSpecSHAForCache still hard-code context.Background(), so Ctrl-C cannot cancel compile-time include resolution — the exact symptom this PR fixes elsewhere.
💡 Suggestion
The follow-up is acknowledged in the comment, but linking a tracking issue here prevents the gap from going stale. It would also help to add a skipped specification test now that documents the expected behaviour once the follow-up lands:
// TODO(follow-up): unskip and fix when ResolveIncludePath gains ctx
func TestDownloadIncludeFromWorkflowSpec_CancellationNotYetRespected(t *testing.T) {
t.Skip("tracked: ctx not threaded through ResolveIncludePath yet")
ctx, cancel := context.WithCancel(context.Background())
cancel()
_, err := downloadIncludeFromWorkflowSpec("owner/repo/path.md@main", nil)
require.ErrorIs(t, err, context.Canceled)
}This makes the remaining gap visible in the test suite rather than only in a comment.
@copilot please address this.
| // It is used for dependency injection in fetchAndSaveRemoteDispatchWorkflows to allow tests | ||
| // to provide a fast-failing mock instead of making real network calls. | ||
| type fileDownloadFn func(owner, repo, path, ref string) ([]byte, error) | ||
| type fileDownloadFn func(ctx context.Context, owner, repo, path, ref string) ([]byte, error) |
There was a problem hiding this comment.
[/codebase-design] The fileDownloadFn type now requires ctx as its first parameter, which is correct. However, the default assignment on line 82 (downloader := fileDownloadFn(parser.DownloadFileFromGitHub)) silently compiles only because parser.DownloadFileFromGitHub now matches the new signature. Any future caller that passes the old signature (without ctx) will get a compile error with a confusing message about the function type.
💡 Suggestion
Consider adding a compile-time assertion in a _test.go or doc.go so that the expected function type is validated independently of the default assignment:
var _ fileDownloadFn = parser.DownloadFileFromGitHub // compile-time interface checkThis makes the contract explicit and improves navigability for future contributors who need to supply a custom downloader.
@copilot please address this.
| @@ -42,15 +42,15 @@ func TestResolveRepositoryPackage(t *testing.T) { | |||
| getRepositoryPackageLatestRelease = func(repoSlug, host string) (string, error) { | |||
There was a problem hiding this comment.
[/tdd] The tests are updated to use t.Context() (great), but they do not include any test that verifies the context is actually forwarded to the download stubs — only that the stub receives the right path/ref/host values. This means a regression where ctx is silently dropped before being forwarded would not be caught.
💡 Suggestion
Add a cancellation sub-test to at least one scenario in TestResolveRepositoryPackage to verify that a cancelled context causes the function to return an error. This can be done cheaply by making the stub check the context:
t.Run("respects cancelled context", func(t *testing.T) {
downloadPackageFileFromGitHubForHost = func(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) {
if err := ctx.Err(); err != nil {
return nil, err
}
return nil, createRepositoryPackageNotFoundError(path)
}
ctx, cancel := context.WithCancel(t.Context())
cancel()
_, err := resolveRepositoryPackage(ctx, &RepoSpec{RepoSlug: "owner/repo"}, "")
require.ErrorIs(t, err, context.Canceled)
})@copilot please address this.
|
|
||
| // RunListWorkflows lists workflows without checking GitHub status | ||
| func RunListWorkflows(repo, path, pattern string, verbose bool, jsonOutput bool, labelFilter string) error { | ||
| func RunListWorkflows(ctx context.Context, repo, path, pattern string, verbose bool, jsonOutput bool, labelFilter string) error { |
There was a problem hiding this comment.
[/tdd] RunListWorkflows is now a public API that accepts ctx, but none of the unit tests in list_workflows_command_test.go assert that a cancelled context is respected (they all pass t.Context() which is never cancelled). The fix enables cancellation; no test verifies it.
💡 Suggestion
Add a test that cancels the context before any network call and asserts the function returns a context-related error:
func TestRunListWorkflows_ContextCancelled(t *testing.T) {
ctx, cancel := context.WithCancel(t.Context())
cancel()
err := RunListWorkflows(ctx, "owner/repo", ".github/workflows", "", false, false, "")
require.Error(t, err)
require.ErrorIs(t, err, context.Canceled)
}\n```
This provides a regression guard for the core behaviour introduced by this PR.
</details>
@copilot please address this.| // If the includePath is a relative path, it's resolved relative to the baseSpec. | ||
| // Returns: (content, section, error) where section is the #fragment from the path (e.g., "#section-name"). | ||
| func FetchIncludeFromSource(includePath string, baseSpec *WorkflowSpec, verbose bool) ([]byte, string, error) { | ||
| func FetchIncludeFromSource(ctx context.Context, includePath string, baseSpec *WorkflowSpec, verbose bool) ([]byte, string, error) { |
There was a problem hiding this comment.
[/codebase-design] FetchIncludeFromSource is a public API — its signature change is a breaking change for any external caller. The PR doesn't mention whether this package has external consumers; if it does, a CHANGELOG entry or deprecation note is warranted.
💡 Suggestion
If the cli package is intended only for internal use (unexported via a build tag or documentation), a brief // Package cli is internal. comment in the package doc would clarify that. If it is externally consumed, consider a CHANGELOG or migration note. Either way, making the intent explicit helps future contributors know whether these signatures are stable contracts.
@copilot please address this.
| // Returns the file content as bytes or an error if the file cannot be retrieved. | ||
| func DownloadFileFromGitHub(owner, repo, path, ref string) ([]byte, error) { | ||
| return downloadFileFromGitHubWithDepth(owner, repo, path, ref, 0, "") | ||
| func DownloadFileFromGitHub(ctx context.Context, owner, repo, path, ref string) ([]byte, error) { |
There was a problem hiding this comment.
[/codebase-design] The DownloadFileFromGitHub and DownloadFileFromGitHubForHost public functions now correctly accept ctx, but there is no test in the non-integration test suite that exercises these public wrappers with a cancelled context. The integration tests exercise real network paths; unit tests for the cancellation contract would be faster and more reliable in CI.
💡 Suggestion
A unit test using an HTTP test server (httptest.NewServer) that blocks until the context is cancelled would confirm the contract without real network access:
func TestDownloadFileFromGitHub_ContextCancelled(t *testing.T) {
// Set up a server that blocks indefinitely
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
<-r.Context().Done()
}))
defer srv.Close()
// ... inject the server URL and cancel the context
ctx, cancel := context.WithCancel(t.Context())
cancel()
_, err := DownloadFileFromGitHub(ctx, "owner", "repo", "path", "main")
require.ErrorIs(t, err, context.Canceled)
}\n```
This directly validates the key behaviour the PR enables.
</details>
@copilot please address this.| if ref == "" { | ||
| // Resolve the actual default branch of the source repo rather than assuming "main" | ||
| defaultBranch, err := getRepoDefaultBranch(context.Background(), spec.RepoSlug) | ||
| defaultBranch, err := getRepoDefaultBranch(ctx, spec.RepoSlug) |
There was a problem hiding this comment.
[/codebase-design] fetchAndSaveRemoteFrontmatterImports now correctly propagates ctx to getRepoDefaultBranch, but the flow has a subtle asymmetry: the resolved default branch is written back into spec.Version (a caller-owned struct). This side-mutation could cause surprising behaviour if the same *WorkflowSpec is reused after cancellation — the Version field would be set even if the subsequent download was never completed.
💡 Suggestion
Consider using a local variable for the resolved ref instead of mutating spec.Version in this function. The mutation was already there before this PR, but threading ctx surfaced it as a potential race in a cancelled scenario. If spec mutation is intentional (caching for later callers), a short comment explaining the design choice would aid future readers.
@copilot please address this.
|
@copilot run pr-finished skill |
… comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Two issues were addressed in commit
|
pkg/parser/remote_fetch.go's 8 public APIs had nocontext.Contextparameter; their private call chains hard-codedcontext.Background(), so Ctrl-C could not cancel in-flight HTTP requests from any CLI command.Core changes
pkg/parser/remote_fetch.go— addsctx context.Contextas first param to all 8 public APIs (DownloadFileFromGitHub,DownloadFileFromGitHubForHost,ResolveRefToSHAForHost,ListWorkflowFiles,ListWorkflowFilesForHost,ListDirAllFilesForHost,ListDirAllFilesRecursivelyForHost,ListDirSubdirsForHost), propagates through every private helper, replaces all 7 hard-codedcontext.Background()sitesCLI callers updated to pass
cmd.Context()dispatch.go—fileDownloadFntype gainsctx;fetchAndSaveDispatchWorkflowsFromParsedFilegainsctxincludes.go—FetchIncludeFromSource,fetchAndSaveRemoteFrontmatterImports,fetchFrontmatterImportsRecursive,fetchAndSaveRemoteIncludesadd_package_manifest.go—resolveRepositoryPackage+ all sub-functions (loadRepositoryPackageManifestFile,resolvePackageSkillFiles,resolvePackageAgentFiles,scanPackageSkillDirs,scanRepositoryPackageInstallablePaths,resolveRepositoryPackageDocsPath) + 5 wrapperslist_workflows_command.go—RunListWorkflows/getRemoteWorkflowFiles; cobraRunEpassescmd.Context()resources.go,fetch.go,add_workflow_resolution.go,update_manifest.go— call sites updatedScope note
Two
context.Background()calls remain indownloadIncludeFromWorkflowSpec/resolveWorkflowSpecSHAForCache— these sit behindResolveIncludePath, whose call chain spans 6+ files across 4 packages with no existing ctx. Threading ctx through that compile-time include-resolution path is a follow-up.