refactor(parser): split remote_fetch.go (1553 lines) into focused modules#43834
Conversation
Split pkg/parser/remote_fetch.go (1553 lines) into 5 focused files: - remote_client.go: shared HTTP client utilities (publicAPIClient, createRESTClientForHost, buildContentsAPIPath, fetchRemoteFileContent, fetchPublicGitHubContentsAPI) - remote_resolve_path.go: include-path resolution & security validation (ResolveIncludePath and helpers) - remote_workflow_spec.go: WorkflowSpec parsing & caching (IsWorkflowSpec, downloadIncludeFromWorkflowSpec and helpers) - remote_resolve_sha.go: ref→SHA resolution chain with fallbacks (resolveRefToSHA, ResolveRefToSHAForHost and helpers) - remote_download_file.go: single-file download with auth/git/public fallbacks (DownloadFileFromGitHub, DownloadFileFromGitHubForHost and helpers) - remote_list_files.go: directory listing operations (ListWorkflowFiles, ListDirAllFilesForHost, etc.) All public APIs preserved, all tests pass, no logic changes. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request refactors the parser’s remote-fetch functionality by splitting the former monolithic pkg/parser/remote_fetch.go into smaller, domain-focused modules, aiming to improve navigability and future testability while preserving existing behavior.
Changes:
- Split remote fetch logic into separate files for client utilities, include-path resolution, workflow-spec parsing, ref→SHA resolution, single-file download, and directory listing.
- Centralized shared REST/public-API client helpers and the shared
remoteLog. - Removed the original 1553-line
remote_fetch.goafter distributing its contents across the new modules.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/remote_client.go | Introduces shared REST client helpers, public API fallback helper, and shared logger/client state. |
| pkg/parser/remote_resolve_path.go | Holds local include-path resolution and security validation logic. |
| pkg/parser/remote_workflow_spec.go | Contains workflow-spec detection/parsing and workflowspec include download + caching. |
| pkg/parser/remote_resolve_sha.go | Contains ref→SHA resolution with gh/auth/git/public-API fallback chain. |
| pkg/parser/remote_download_file.go | Contains remote single-file download logic including symlink resolution and fallback paths. |
| pkg/parser/remote_list_files.go | Contains directory listing APIs (workflow files, dir files, recursive, subdirs) plus git clone caching for fallbacks. |
| pkg/parser/remote_fetch.go | Deleted in favor of the new focused modules. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Low
| // Try to resolve the ref using git ls-remote | ||
| // Format: git ls-remote <repo> <ref> | ||
| cmd := exec.Command("git", "ls-remote", repoURL, ref) | ||
| output, err := cmd.Output() | ||
| if err != nil { |
| // downloadFileViaGitClone downloads a file by shallow cloning the repository | ||
| // This is used as a fallback when git archive doesn't work | ||
| func downloadFileViaGitClone(owner, repo, path, ref, host string) ([]byte, error) { | ||
| remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref) | ||
|
|
| // Additional validation: check if it looks like a valid owner/repo format | ||
| // GitHub identifiers can't start with numbers, must be alphanumeric with hyphens/underscores |
| cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir) | ||
| cloneOutput, err := cloneCmd.CombinedOutput() | ||
| if err != nil { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This is a pure refactoring PR (splitting remote_fetch.go into focused modules). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
Captures the architectural decision to split the 1553-line pkg/parser/remote_fetch.go into six focused single-domain modules, preserving all exported symbols with no logic changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,624 new lines in 📄 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
|
![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 /codebase-design and /improve-codebase-architecture — requesting changes on a small set of consistency and correctness issues introduced during the split.
The decomposition itself is well-executed: clear domain boundaries, no logic changes, all exports preserved, file sizes now manageable. The four issues flagged are opportunistic improvements that are easier to fix now while the files are being touched.
📋 Key Themes
Issues flagged
rawClientper-call allocation (remote_download_file.go:327) — allocates a newhttp.Clienton everydownloadFileViaRawURLcall;publicAPIClientinremote_client.gois the established pattern and should cover this too.exec.Commandwithout context (remote_resolve_sha.go:35) —resolveRefToSHAViaGitcan't be cancelled;exec.CommandContextshould be used, consistent withdownloadFileViaGit.isWorkflowSpecone-liner wrapper (remote_workflow_spec.go:64) — a no-op indirection overIsWorkflowSpec; callers within the package can use the exported name directly.listWorkflowFilesViaGitForHostbypasses shared clone cache (remote_list_files.go:177) — clones fresh instead of usinggetOrCreateListRepoClone, inconsistent with the other two git-fallback listing functions.
Positive highlights
- ✅ Clean domain boundaries: each file has a clear single responsibility
- ✅ Exported API surface fully preserved — no caller breakage
- ✅
fetchPublicGitHubContentsAPIcorrectly lives inremote_client.goas a shared utility - ✅ Security path-validation logic correctly isolated in
remote_resolve_path.go - ✅ Three-tier fallback pattern (REST → git → public API) applied consistently across all listing functions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 94.9 AIC · ⌖ 5.84 AIC · ⊞ 6.7K
Comment /matt to run again
| remoteLog.Printf("Attempting raw URL download: %s", rawURL) | ||
|
|
||
| // Use a client with a timeout to prevent indefinite hangs on slow/unresponsive hosts. | ||
| rawClient := &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
[/codebase-design] rawClient is allocated fresh on every call while publicAPIClient in remote_client.go is a shared package-level client for the same purpose. This inconsistency signals that the raw-URL client should also be a package-level var.
💡 Suggested fix
In remote_client.go, add alongside publicAPIClient:
var rawURLClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}Then in downloadFileViaRawURL, replace the local allocation:
// Before
rawClient := &http.Client{Timeout: constants.DefaultHTTPClientTimeout}
resp, err := rawClient.Do(req)
// After
resp, err := rawURLClient.Do(req)http.Client is safe for concurrent use; a package-level client avoids repeated allocations and is consistent with the pattern already established for publicAPIClient.
@copilot please address this.
|
|
||
| // Try to resolve the ref using git ls-remote | ||
| // Format: git ls-remote <repo> <ref> | ||
| cmd := exec.Command("git", "ls-remote", repoURL, ref) |
There was a problem hiding this comment.
[/codebase-design] resolveRefToSHAViaGit uses exec.Command instead of exec.CommandContext, so a hung git ls-remote call cannot be cancelled. This diverges from the pattern used elsewhere (e.g., downloadFileViaGit uses exec.CommandContext).
💡 Suggested fix
Add ctx context.Context to the function signature and use exec.CommandContext:
func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) (string, error) {
...
cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, ref)
...
for _, prefix := range []string{"refs/heads/", "refs/tags/"} {
cmd = exec.CommandContext(ctx, "git", "ls-remote", repoURL, prefix+ref)
}
}The caller resolveRefToSHA already has ctx and should pass it through.
@copilot please address this.
| return true | ||
| } | ||
|
|
||
| func isWorkflowSpec(path string) bool { |
There was a problem hiding this comment.
[/improve-codebase-architecture] isWorkflowSpec is a one-line wrapper over IsWorkflowSpec that serves no purpose — the two names differ only in exported vs unexported. Callers within the package can call IsWorkflowSpec directly.
💡 Suggested fix
Remove the wrapper:
// Delete:
func isWorkflowSpec(path string) bool {
return IsWorkflowSpec(path)
}Update the single call site in remote_resolve_path.go line 108:
// Before
if isWorkflowSpec(filePath) {
// After
if IsWorkflowSpec(filePath) {This reduces indirection and keeps the codebase navigable.
@copilot please address this.
|
|
||
| // Do a minimal clone using filter=blob:none for faster cloning (metadata only, no blobs) | ||
| // Use --depth=1 for shallow clone and --no-checkout to skip checkout initially | ||
| cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir) |
There was a problem hiding this comment.
[/codebase-design] listWorkflowFilesViaGitForHost does its own MkdirTemp + defer os.RemoveAll clone while getOrCreateListRepoClone exists precisely to deduplicate clones across calls for the same repo/ref. Using the shared cache here would eliminate a redundant clone on repeated calls.
💡 Suggested fix
Replace lines 169–182 with:
tmpDir, err := getOrCreateListRepoClone(owner, repo, ref, host)
if err != nil {
return nil, err
}
// Note: no defer os.RemoveAll — getOrCreateListRepoClone manages lifetime via the cachelistDirAllFilesViaGitForHost and listDirSubdirsViaGitForHost already follow this pattern (lines 298, 518). Aligning listWorkflowFilesViaGitForHost removes the inconsistency and avoids duplicate clone overhead.
@copilot please address this.
| } | ||
|
|
||
| remoteLog.Printf("Fetching file from GitHub: %s/%s/%s@%s", owner, repo, filePath, ref) | ||
| content, err := downloadFileFromGitHub(context.Background(), owner, repo, filePath, ref) |
There was a problem hiding this comment.
[/codebase-design] context.Background() is used here as a noted workaround, but the NOTE comment (lines 71–73) describes the follow-up task without creating an issue reference. This makes it easy to lose track of the context-threading work.
💡 Suggestion
Convert the NOTE comment into a tracked GitHub issue and reference it:
// TODO(#<issue>): Thread ctx through ResolveIncludePath and callers.
// Using context.Background() in the interim.
content, err := downloadFileFromGitHub(context.Background(), ...)The same applies to line 134 (resolveRefToSHA(context.Background(), ...)). A filed issue keeps the technical debt visible and discoverable.
@copilot please address this.
There was a problem hiding this comment.
Review: refactor(parser): split remote_fetch.go into focused modules
The structural decomposition is clean and the domain boundaries are well-chosen. All exported symbols are preserved, build tags are consistent across new files, and the cross-file dependency graph is acyclic. No logic was changed.
One new finding — rawClient not consolidated
downloadFileViaRawURL (remote_download_file.go:327) creates a new &http.Client{...} per call rather than reusing the publicAPIClient shared instance introduced in remote_client.go. See inline comment.
Previously flagged (existing review thread):
exec.Commandused instead ofexec.CommandContextin three git fallback paths (resolveRefToSHAViaGit,downloadFileViaGitClone, clone-cache inremote_list_files.go) — context cancellation cannot interrupt long-running git operations.downloadFileViaGitClonehas nocontext.Contextparameter despite being invoked from context-aware callers.- Comment on
isRepositoryImportclaims "GitHub identifiers can't start with numbers" but the code does not enforce that rule.
Overall: COMMENT — no blocking correctness or security issues, but the rawClient inconsistency and missing context propagation to git subprocesses are worth addressing before the PR merges.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 84.7 AIC · ⌖ 5.43 AIC · ⊞ 4.9K
| remoteLog.Printf("Attempting raw URL download: %s", rawURL) | ||
|
|
||
| // Use a client with a timeout to prevent indefinite hangs on slow/unresponsive hosts. | ||
| rawClient := &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
rawClient should reuse publicAPIClient
The refactoring elevated publicAPIClient to a module-level shared instance in remote_client.go, but downloadFileViaRawURL still allocates a fresh &http.Client{Timeout: constants.DefaultHTTPClientTimeout} on every call. These two clients carry identical configuration, so rawClient should be replaced with the shared publicAPIClient:
// before
rawClient := &http.Client{Timeout: constants.DefaultHTTPClientTimeout}
resp, err := rawClient.Do(req)
// after
resp, err := publicAPIClient.Do(req)Reusing the shared client avoids creating a new TCP transport on each download attempt and is consistent with how resolveRefToSHAViaPublicAPI and fetchPublicGitHubContentsAPI already use publicAPIClient.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — refactor entrenches multiple correctness, concurrency, and security-adjacent bugs.
### Blocking themes
1. Context propagation dropped at every git subprocess boundary
resolveRefToSHAViaGit, downloadFileViaGitClone, and all six exec.Command calls in remote_list_files.go ignore the context.Context that callers provide. Cancellation and deadlines silently don't fire; a slow remote can block a goroutine for ~2 minutes.
2. GHE host hardcoded as "" in workflow-spec SHA cache
resolveWorkflowSpecSHAForCache always resolves against github.com. On GitHub Enterprise, SHA lookups hit the wrong host, causing cache misses or silent cache corruption where a GHE SHA is associated with the wrong org/repo.
3. isRepositoryImport over-broad dot-check breaks valid repo names
strings.Contains(repo, ".") rejects repos like my.service or go-server.v2, silently falling through to relative-path resolution and a wrong-file fetch or 404.
4. TOCTOU race in getOrCreateListRepoClone
The mutex is released before the clone runs. Two concurrent callers for the same key both clone; the loser's tmpDir can leak if os.RemoveAll fails. A singleflight.Group is the correct fix.
5. Symlink remaining path not cleaned before concatenation
resolvedBase + "/" + remaining is returned without path.Clean — crafted .. segments after the symlink target bypass the root-escape guard that only covers resolvedBase.
6. Per-call http.Client allocation in downloadFileViaRawURL
Allocating &http.Client{Timeout: ...} on every call prevents connection reuse and adds TCP+TLS overhead on repeated fetches.
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.
🔎 Code quality review by PR Code Quality Reviewer · 245.4 AIC · ⌖ 9.31 AIC · ⊞ 5.5K
Comment /review to run again
|
|
||
| // Try to resolve the ref using git ls-remote | ||
| // Format: git ls-remote <repo> <ref> | ||
| cmd := exec.Command("git", "ls-remote", repoURL, ref) |
There was a problem hiding this comment.
Context not threaded to resolveRefToSHAViaGit: exec.Command is used instead of exec.CommandContext, so cancellation and deadline signals are silently dropped — git ls-remote can hang indefinitely on a slow or unreachable remote.
💡 Suggested fix
Change the function signature to accept context.Context and use exec.CommandContext:
func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) (string, error) {
cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, ref)
// retry loop also uses exec.CommandContext(ctx, ...)
}The caller resolveRefToSHA (in remote_resolve_sha.go) already carries a context.Context but cannot thread it through because the callee signature doesn't accept one. Without this, an unresponsive remote can block the calling goroutine until the OS TCP timeout fires (~2 min), stalling the entire workflow parse.
| if isSHA { | ||
| // For SHA refs, we need to clone without --branch and then checkout the specific commit | ||
| // Clone with minimal depth and no branch specified | ||
| cloneCmd = exec.Command("git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir) |
There was a problem hiding this comment.
exec.Command in git fallback drops context: downloadFileViaGitClone uses bare exec.Command for clone, checkout, and archive — context cancellation is silently ignored.
💡 Suggested fix
Replace all three exec.Command calls with exec.CommandContext(ctx, ...):
// line ~381
cloneCmd = exec.CommandContext(ctx, "git", "clone", ...)
// line ~392
checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref)
// line ~398
cloneCmd = exec.CommandContext(ctx, "git", "clone", "--depth", "1", ...)The function already receives a ctx context.Context parameter (used only for the REST branch) — thread it through to all subprocess invocations. The same pattern applies to remote_list_files.go lines 65, 177, 185, 303, 442, 524.
| if cache == nil { | ||
| return "" | ||
| } | ||
| resolvedSHA, err := resolveRefToSHA(context.Background(), owner, repo, ref, "") |
There was a problem hiding this comment.
GHE host hardcoded as "" in SHA cache resolution: resolveWorkflowSpecSHAForCache always calls resolveRefToSHA(..., ""), ignoring the GHE host; on GitHub Enterprise the SHA is resolved against github.com instead of the enterprise host, causing cache misses or silent cache corruption when the same ref resolves to a different SHA on GHE.
💡 Suggested fix
Thread the host through from the call site. The caller downloadIncludeFromWorkflowSpec already has the parsed host from parseWorkflowSpecParts:
func resolveWorkflowSpecSHAForCache(owner, repo, ref, host string, cache *ImportCache) string {
...
resolvedSHA, err := resolveRefToSHA(context.Background(), owner, repo, ref, host)
}Update the call site at line 82 accordingly: resolveWorkflowSpecSHAForCache(owner, repo, ref, host, cache).
| } | ||
|
|
||
| // Reject if repo part looks like a file extension (ends with .md, .yaml, etc.) | ||
| if strings.Contains(repo, ".") { |
There was a problem hiding this comment.
isRepositoryImport dot-check is over-broad: strings.Contains(repo, ".") rejects valid repository names like my.service, go-server.v2, or any package-style names — these would silently fall through to a relative-path resolution and either 404 or resolve to the wrong file.
💡 Suggested fix
Check only the extensions actually used for workflow include files, not any dot:
// Reject only if the repo segment has a recognized file extension
for _, ext := range []string{".md", ".yaml", ".yml", ".json"} {
if strings.HasSuffix(repo, ext) {
return false
}
}The comment on line 87–88 already says the intent is to avoid treating file extensions as repository names — the implementation is more aggressive than the intent.
| return "", fmt.Errorf("failed to create temp directory: %w", err) | ||
| } | ||
|
|
||
| cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir) |
There was a problem hiding this comment.
TOCTOU race in getOrCreateListRepoClone: the mutex is released after the cache-miss check (line 56), the clone runs outside the lock (line 65), and the mutex is re-acquired to store the result. Two concurrent calls for the same key will both clone, and if os.RemoveAll on the losing tmpDir fails, the directory leaks.
💡 Suggested fix
Use a singleflight.Group keyed on cacheKey to coalesce concurrent clone requests:
var cloneGroup singleflight.Group
func getOrCreateListRepoClone(owner, repo, ref, host string) (string, error) {
...
dir, err, _ := cloneGroup.Do(cacheKey, func() (interface{}, error) {
// existing clone + cache-store logic
})
return dir.(string), err
}The current double-checked locking pattern (lines 46–57 and 75–90) is a well-known Go anti-pattern without singleflight — it avoids cache corruption but not redundant clones or tmpDir leaks on RemoveAll failure.
| remoteLog.Printf("Attempting raw URL download: %s", rawURL) | ||
|
|
||
| // Use a client with a timeout to prevent indefinite hangs on slow/unresponsive hosts. | ||
| rawClient := &http.Client{Timeout: constants.DefaultHTTPClientTimeout} |
There was a problem hiding this comment.
New http.Client allocated per downloadFileViaRawURL call: line 327 creates &http.Client{Timeout: ...} on every invocation, bypassing the package-level publicAPIClient — each call pays full TCP and TLS handshake cost.
💡 Suggested fix
Use the existing package-level client or declare a package-level raw client once:
// package-level, next to publicAPIClient
var rawURLClient = &http.Client{Timeout: constants.DefaultHTTPClientTimeout}
// inside downloadFileViaRawURL
resp, err := rawURLClient.Do(req)The http.Client docs explicitly note that clients should be reused; per-call allocation prevents connection pooling and adds measurable latency for repositories that are fetched repeatedly.
| if err != nil { | ||
| return "", false, err | ||
| } | ||
| remaining := strings.Join(parts[index:], "/") |
There was a problem hiding this comment.
remaining path not cleaned before concatenation in symlink resolution: after a symlink base is validated (line 249), remaining = strings.Join(parts[index:], "/") is concatenated directly as resolvedBase + "/" + remaining without path.Clean — a crafted path with .. or . segments in the post-symlink portion bypasses the repository-root guard applied only to resolvedBase.
💡 Suggested fix
Clean the full assembled path before returning:
resolvedPath := pathpkg.Clean(resolvedBase + "/" + remaining)
if strings.HasPrefix(resolvedPath, "..") || pathpkg.IsAbs(resolvedPath) {
return "", false, fmt.Errorf("resolved symlink path escapes repository root: %s", resolvedPath)
}Example: a symlink at a/link → b, with a request for a/link/../../../etc/passwd produces remaining = "../../../etc/passwd". resolvedBase is validated as safe, but b/../../../etc/passwd after Clean becomes ../../etc/passwd and is never re-checked. The fix is a single path.Clean + bounds check on the final assembled path.
🤖 PR Triage
Summary: Pure refactor — splits
|
Ran the |
🤖 PR Triage — Run §28834001067
Score Breakdown:
CI: 2x CHANGES_REQUESTED by bot — needs remediation before merge. Rationale: Large file split refactor. Pure structural. Review together with #43833 (caching refactor) as Labels: already applied from prior run.
|
…actor-remote-fetch Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. I ran the |
|
@copilot please run the
|
Ran the |
|
@copilot please run the
|
Ran a full PR-finisher pass on current head
CI on this head remains unverified from agent pushes; please re-trigger workflows from a maintainer context ( |
|
🎉 This pull request is included in a new release. Release: |
pkg/parser/remote_fetch.gohad grown to 1553 lines containing five unrelated functional domains, making it hard to navigate, review, and test.Changes
Split into six focused files, all under 450 lines, with no logic changes:
remote_client.gopublicAPIClient,createRESTClientForHost,buildContentsAPIPath,fetchRemoteFileContent,fetchPublicGitHubContentsAPI)remote_resolve_path.goResolveIncludePathand helpers)remote_workflow_spec.goIsWorkflowSpec,downloadIncludeFromWorkflowSpec)remote_resolve_sha.goresolveRefToSHA,ResolveRefToSHAForHost)remote_download_file.goremote_list_files.gopublicAPIClient,createRESTClientForHost,fetchPublicGitHubContentsAPI) live inremote_client.go, accessible to both download and listing code within the same package — no circular dependency concern.ResolveIncludePath,IsWorkflowSpec,DownloadFileFromGitHub,ListWorkflowFiles, etc.) are preserved unchanged.remote_fetch_wasm.gois untouched; the wasm stubs remain in their own file.