refactor(parser): split remote_fetch.go (1553 lines) into focused modules

docs/adr/43834-split-remote-fetch-into-focused-modules.md

@@ -0,0 +1,45 @@
+# ADR-43834: Split remote_fetch.go into Focused Single-Responsibility Modules
+
+**Date**: 2026-07-06
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+`pkg/parser/remote_fetch.go` had grown to 1,553 lines across five unrelated functional domains: shared HTTP client utilities, local include-path resolution and security validation, WorkflowSpec parsing and download dispatch, ref-to-SHA resolution with multi-tier auth fallbacks, single-file download with symlink handling, and directory listing (workflow files, flat, recursive, subdirs). A single file spanning five concerns made it hard to navigate, review, and isolate in tests. Merge conflicts were more frequent because unrelated features competed for changes in the same file.
+
+### Decision
+
+We will split `remote_fetch.go` into six focused files, all residing in the same `parser` package, each under 450 lines and responsible for exactly one functional domain: `remote_client.go` (shared HTTP/REST utilities), `remote_resolve_path.go` (include-path resolution and security validation), `remote_workflow_spec.go` (WorkflowSpec parsing, SHA caching, download dispatch), `remote_resolve_sha.go` (ref→SHA resolution with auth/git/public-API fallback chain), `remote_download_file.go` (single-file download with symlink resolution and three-tier fallback), and `remote_list_files.go` (directory listing). All exported symbols are preserved unchanged; no logic is altered.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the monolithic file with internal comment sections
+
+Add region-style comments (e.g., `// --- HTTP Client ---`) to demarcate the five domains within `remote_fetch.go` without splitting files. This is zero-risk from a compilation and test perspective and avoids any directory-structure change. It was not chosen because comment markers are non-enforced conventions — they drift and get ignored under deadline pressure, and navigating a 1,500-line file still requires IDE search. The size problem recurs the moment a new domain is added.
+
+#### Alternative 2: Extract into separate sub-packages under `pkg/parser/remote/`
+
+Create sub-packages such as `pkg/parser/remote/client`, `pkg/parser/remote/download`, etc., moving each domain into its own package. This enforces separation at the language level (the compiler will catch circular imports) and makes inter-domain dependencies explicit. It was not chosen because it would require changing all call sites across the `parser` package, potentially introduce circular imports given the current tight coupling, and is a larger refactor scope than the immediate problem warrants. Same-package file splits achieve the navigability goal with zero API breakage.
+
+### Consequences
+
+#### Positive
+- Each file is under 450 lines with a single clearly named responsibility, making targeted code review and debugging faster.
+- Merge conflicts on `remote_fetch.go` are eliminated; changes to download logic no longer race with changes to SHA resolution or directory listing.
+- Easier to unit-test each domain in isolation without loading the other four domains into view.
+- New contributors can discover where a specific function lives by filename rather than grepping a 1,500-line file.
+
+#### Negative
+- The `pkg/parser` directory now contains more files (six new files replace one), which may feel noisy at a glance before the reader understands the naming convention.
+- The same-package split does not enforce domain boundaries at compile time — a function in `remote_download_file.go` can still call unexported helpers in `remote_list_files.go` without the compiler flagging it.
+
+#### Neutral
+- No logic changes mean all existing tests pass without modification; behavior for external callers of exported symbols (`DownloadFileFromGitHub`, `ListWorkflowFiles`, etc.) is identical.
+- The wasm build stub `remote_fetch_wasm.go` is untouched; the build-tag-gated split is consistent with that pattern.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/stretchr/testify v1.11.1
 	golang.org/x/crypto v0.53.0
 	golang.org/x/mod v0.37.0
+	golang.org/x/sync v0.21.0
 	golang.org/x/term v0.44.0
 	golang.org/x/tools v0.47.0
 	golang.org/x/vuln v1.5.0
@@ -109,7 +110,6 @@ require (
 	golang.org/x/exp v0.0.0-20240909161429-701f63a606c0 // indirect
 	golang.org/x/net v0.56.0 // indirect
 	golang.org/x/oauth2 v0.36.0 // indirect
-	golang.org/x/sync v0.21.0 // indirect
 	golang.org/x/sys v0.46.0 // indirect
 	golang.org/x/telemetry v0.0.0-20260625142307-59b4966ccb57 // indirect
 	golang.org/x/text v0.38.0 // indirect

pkg/parser/import_bfs.go

@@ -184,7 +184,7 @@ func validateNoLockYMLImport(fullPath, importPath, workflowFilePath, yamlContent
 }
 
 func detectRemoteImportOrigin(filePath string) *remoteImportOrigin {
-	if !isWorkflowSpec(filePath) {
+	if !IsWorkflowSpec(filePath) {
 		return nil
 	}
 	origin := parseRemoteOrigin(filePath)
@@ -407,10 +407,10 @@ func enqueueNestedImportEntry(entry nestedImportEntry, item importQueueItem, bas
 }
 
 func resolveNestedImportPathAndOrigin(item importQueueItem, nestedFilePath string) (string, *remoteImportOrigin, error) {
-	if item.remoteOrigin != nil && !isWorkflowSpec(nestedFilePath) {
+	if item.remoteOrigin != nil && !IsWorkflowSpec(nestedFilePath) {
 		return resolveRemoteNestedPath(item, nestedFilePath)
 	}
-	if isWorkflowSpec(nestedFilePath) {
+	if IsWorkflowSpec(nestedFilePath) {
 		nestedRemoteOrigin := parseRemoteOrigin(nestedFilePath)
 		if nestedRemoteOrigin != nil {
 			importLog.Printf("Nested workflowspec import detected: %s (origin: %s/%s@%s)", nestedFilePath, nestedRemoteOrigin.Owner, nestedRemoteOrigin.Repo, nestedRemoteOrigin.Ref)
@@ -439,7 +439,7 @@ func resolveRemoteNestedPath(item importQueueItem, nestedFilePath string) (strin
 
 func determineNestedBaseDir(item importQueueItem, resolvedPath, baseDir string) string {
 	isLocalRelative := !strings.Contains(resolvedPath, "/") || strings.HasPrefix(resolvedPath, "./")
-	if item.remoteOrigin == nil && !isWorkflowSpec(resolvedPath) && isLocalRelative {
+	if item.remoteOrigin == nil && !IsWorkflowSpec(resolvedPath) && isLocalRelative {
 		return filepath.Dir(item.fullPath)
 	}
 	return baseDir

pkg/parser/import_remote_nested_test.go

@@ -277,7 +277,7 @@ func TestRemoteOriginPropagation(t *testing.T) {
 
 	t.Run("workflowspec import gets remote origin", func(t *testing.T) {
 		spec := "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main"
-		assert.True(t, isWorkflowSpec(spec), "Should be recognized as workflowspec")
+		assert.True(t, IsWorkflowSpec(spec), "Should be recognized as workflowspec")
 
 		origin := parseRemoteOrigin(spec)
 		require.NotNil(t, origin, "Should parse remote origin")
@@ -289,7 +289,7 @@ func TestRemoteOriginPropagation(t *testing.T) {
 
 	t.Run("local import does not get remote origin", func(t *testing.T) {
 		localPath := "shared/tools.md"
-		assert.False(t, isWorkflowSpec(localPath), "Should not be recognized as workflowspec")
+		assert.False(t, IsWorkflowSpec(localPath), "Should not be recognized as workflowspec")
 
 		origin := parseRemoteOrigin(localPath)
 		assert.Nil(t, origin, "Local paths should not produce remote origin")
@@ -320,7 +320,7 @@ func TestRemoteOriginPropagation(t *testing.T) {
 		)
 
 		// The constructed spec should be recognized as a workflowspec
-		assert.True(t, isWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec")
+		assert.True(t, IsWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec")
 	})
 
 	t.Run("nested relative path from remote parent without BasePath uses .github/workflows", func(t *testing.T) {
@@ -347,7 +347,7 @@ func TestRemoteOriginPropagation(t *testing.T) {
 		)
 
 		// The constructed spec should be recognized as a workflowspec
-		assert.True(t, isWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec")
+		assert.True(t, IsWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec")
 	})
 
 	t.Run("nested relative path with ./ prefix is cleaned", func(t *testing.T) {
@@ -383,7 +383,7 @@ func TestRemoteOriginPropagation(t *testing.T) {
 		// If a remote file references another workflowspec, it should
 		// get its own origin, not inherit the parent's
 		nestedSpec := "other-org/other-repo/path/file.md@v2.0"
-		assert.True(t, isWorkflowSpec(nestedSpec), "Should be recognized as workflowspec")
+		assert.True(t, IsWorkflowSpec(nestedSpec), "Should be recognized as workflowspec")
 
 		origin := parseRemoteOrigin(nestedSpec)
 		require.NotNil(t, origin, "Should parse remote origin for nested workflowspec")
@@ -694,7 +694,7 @@ func TestParseRemoteOriginWithURLFormats(t *testing.T) {
 
 		for _, urlPath := range urlPaths {
 			// Currently, isWorkflowSpec accepts URLs (they have >3 slash-separated parts)
-			isSpec := isWorkflowSpec(urlPath)
+			isSpec := IsWorkflowSpec(urlPath)
 			assert.True(t, isSpec, "URL is currently accepted as workflowspec: %s", urlPath)
 
 			// parseRemoteOrigin will parse the URL parts literally

pkg/parser/remote_client.go

@@ -0,0 +1,88 @@
+//go:build !js && !wasm
+
+package parser
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+
+	"github.com/cli/go-gh/v2/pkg/api"
+	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+var remoteLog = logger.New("parser:remote_fetch")
+
+// 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}
+
+func createRESTClientForHost(host string) (*api.RESTClient, error) {
+	opts := api.ClientOptions{Timeout: constants.DefaultHTTPClientTimeout}
+	if host != "" {
+		opts.Host = host
+	}
+	return api.NewRESTClient(opts)
+}
+
+func buildContentsAPIPath(owner, repo, path, ref string) string {
+	pathSegments := strings.Split(path, "/")
+	for i := range pathSegments {
+		pathSegments[i] = url.PathEscape(pathSegments[i])
+	}
+	return fmt.Sprintf(
+		"repos/%s/%s/contents/%s?ref=%s",
+		owner,
+		repo,
+		strings.Join(pathSegments, "/"),
+		url.QueryEscape(ref),
+	)
+}
+
+func fetchRemoteFileContent(ctx context.Context, client *api.RESTClient, owner, repo, path, ref string, fileContent any) error {
+	return client.DoWithContext(ctx, http.MethodGet, buildContentsAPIPath(owner, repo, path, ref), nil, fileContent)
+}
+
+// fetchPublicGitHubContentsAPI makes an unauthenticated GET request to the
+// GitHub public REST API contents endpoint. This is used as a last-resort
+// fallback when the current token (e.g. an enterprise SAML-enforced token)
+// cannot access cross-organization public repositories and git clone also
+// fails. Unauthenticated requests are subject to a lower rate limit
+// (60 req/hour) but are sufficient for the handful of calls during update.
+func fetchPublicGitHubContentsAPI(ctx context.Context, owner, repo, path, ref string) ([]byte, error) {
+	// Encode each path segment independently so that '/' separators are
+	// preserved — url.PathEscape would turn them into '%2F', breaking nested
+	// paths like '.github/workflows/shared/foo.md'.
+	segments := strings.Split(path, "/")
+	encodedSegments := make([]string, len(segments))
+	for i, s := range segments {
+		encodedSegments[i] = url.PathEscape(s)
+	}
+	endpoint := fmt.Sprintf("https://api.github.com/repos/%s/%s/contents/%s?ref=%s",
+		owner, repo, strings.Join(encodedSegments, "/"), url.QueryEscape(ref))
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
+	if err != nil {
+		return nil, err
+	}
+	req.Header.Set("Accept", "application/vnd.github+json")
+
+	resp, err := publicAPIClient.Do(req)
+	if err != nil {
+		return nil, err
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("GitHub API returned %d: %s", resp.StatusCode, strings.TrimSpace(string(body)))
+	}
+	return body, nil
+}