fix: gh aw add resolves uses: references transitively in imported files

docs/adr/44763-resolve-transitive-uses-imports-by-path-structure.md

@@ -0,0 +1,58 @@
+# ADR-44763: Resolve Transitive Uses Imports Using Path Structure Heuristic
+
+**Date**: 2026-07-10
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The `gh aw add` command fetches workflow files and their `imports:` dependencies from a source repository. The internal function `fetchFrontmatterImportsRecursive` handles this recursively. Two bugs caused transitive imports to be silently dropped:
+
+1. Bare filenames (e.g., `control-aux.md`, containing no `/`) were resolved against the workflow root (`originalBaseDir`) rather than the importing file's directory (`currentBaseDir`). This contradicted the compiler's own `determineNestedBaseDir` logic, producing incorrect remote paths and fetching from the wrong location.
+
+2. When `force=false` and a shared file already existed on disk, the function skipped both the download and the recursion into that file's `imports:`. Any transitive dependencies referenced by the already-present file were never fetched.
+
+A testability gap also existed: the production download function (`parser.DownloadFileFromGitHub`) was hard-coded, making it impossible to unit-test path resolution logic without real network calls.
+
+### Decision
+
+We will apply a path-structure heuristic in `fetchFrontmatterImportsRecursive` to choose the base directory for import resolution: paths **without** a `/` separator are resolved relative to `currentBaseDir` (the importing file's own directory), and paths **with** a `/` are resolved relative to `originalBaseDir` (the workflow root). This mirrors the compiler's `determineNestedBaseDir` logic.
+
+We will also ensure that when a shared file already exists on disk and is skipped for re-download, the function reads the existing file and recurses into its imports before continuing — preserving the download-skip optimization while still completing transitive dependency resolution.
+
+Additionally, we will introduce a `downloadFn` field on `frontmatterImportsOpts` that defaults to `parser.DownloadFileFromGitHub` when nil, enabling unit tests to inject a stub without network calls.
+
+### Alternatives Considered
+
+#### Alternative 1: Resolve all non-explicit paths against `currentBaseDir`
+
+Resolve every non-explicit import path relative to the importing file's directory, regardless of whether the path contains `/`. This would be simpler and fully consistent.
+
+This was not chosen because paths containing `/` (e.g., `shared/foo.md`) are authored by convention relative to the workflow root `.github/workflows`, not relative to the importing file's own location. Changing this convention would break all existing multi-segment cross-directory imports and would diverge from the compiler's resolution model, creating a split-brain between fetcher and compiler.
+
+#### Alternative 2: Remove caching and always re-download (always use `force=true` behavior)
+
+Remove the `force=false` optimization entirely so that every import is always re-downloaded, ensuring full recursion on every invocation without the need for the read-and-recurse fallback.
+
+This was not chosen because it introduces unnecessary network round-trips for unchanged files. The skip-if-exists optimization is valuable for large workflow trees. The chosen fix is surgical: retain the skip, but add a read-and-recurse step on the existing file's content before continuing to the next import.
+
+### Consequences
+
+#### Positive
+- Bare-filename imports (no `/`) now correctly resolve to the importing file's sibling directory, matching compiler behavior and eliminating silent resolution failures.
+- Transitive dependencies are fetched even when intermediate shared files already exist on disk, ensuring complete dependency trees on every `gh aw add` invocation.
+- The injected `downloadFn` field enables fast, hermetic unit tests for path resolution and recursion logic without network calls, consistent with the existing dispatch-workflow stub pattern in the codebase.
+
+#### Negative
+- Each pre-existing import file now incurs an additional `os.ReadFile` call during the skip path, increasing disk I/O proportionally to the number of already-installed shared files in large trees.
+- The slash-based resolution heuristic is an implicit convention embedded in code rather than an explicit contract; future authors importing from unconventional directory structures may encounter unexpected behavior if their paths violate the `/`-presence assumption.
+
+#### Neutral
+- The `downloadFn` nil-check pattern (default to the real function when nil) is consistent with how other injectable functions are handled in the codebase.
+- All existing paths containing `/` continue to resolve against `originalBaseDir`, so the change is backward-compatible with all current `imports:` declarations.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/includes.go

@@ -184,6 +184,10 @@ type frontmatterImportsOpts struct {
 	force           bool
 	tracker         *FileTracker
 	seen            map[string]struct{}
+	// downloadFn is the function used to fetch file content from the source repository.
+	// When nil, parser.DownloadFileFromGitHub is used. Tests may inject a stub to avoid
+	// network calls and observe which paths were requested.
+	downloadFn func(ctx context.Context, owner, repo, path, ref string) ([]byte, error)
 }
 
 // fetchFrontmatterImportsRecursive is the internal worker for fetchAndSaveRemoteFrontmatterImports.
@@ -275,17 +279,25 @@ func fetchFrontmatterImportsRecursive(ctx context.Context, content, currentBaseD
 				remoteFilePath = filePath
 			}
 		} else {
-			// Non-explicit relative path (e.g. "shared/foo.md"): resolve relative to the
-			// original base directory (the top-level workflow's directory). Workflows in
-			// this repository write shared import paths relative to the workflow root
-			// (e.g. ".github/workflows"), not relative to the importing file's own
-			// directory. Resolving against originalBaseDir instead of currentBaseDir
-			// ensures that a file at ".github/workflows/shared/base.md" can import
-			// "shared/helper.md" and have it resolve to ".github/workflows/shared/helper.md"
-			// rather than the incorrect ".github/workflows/shared/shared/helper.md".
-			baseDir := opts.originalBaseDir
-			if baseDir == "" {
+			// Non-explicit relative path: resolution strategy mirrors the compiler's
+			// determineNestedBaseDir logic — it depends on whether the path contains a
+			// "/" separator.
+			//
+			// Paths WITHOUT "/" (e.g. "helper.md"): the importing file treats them as
+			// siblings in its own directory, so resolve relative to currentBaseDir.
+			// Example: "control-precompute.md" from ".github/workflows/shared/control.md"
+			//          → ".github/workflows/shared/control-precompute.md"
+			//
+			// Paths WITH "/" (e.g. "shared/foo.md"): workflows in this repository write
+			// these paths relative to the workflow root (.github/workflows), not relative to
+			// the importing file's directory, so resolve against originalBaseDir.
+			// Example: "shared/reporting.md" from ".github/workflows/shared/daily-audit-base.md"
+			//          → ".github/workflows/shared/reporting.md"
+			var baseDir string
+			if !strings.Contains(filePath, "/") {
 				baseDir = currentBaseDir
+			} else {
+				baseDir = opts.originalBaseDir
 			}
 			if baseDir != "" {
 				remoteFilePath = path.Join(baseDir, filePath)
@@ -349,20 +361,35 @@ func fetchFrontmatterImportsRecursive(ctx context.Context, content, currentBaseD
 		}
 
 		// Check existence before downloading: if the file already exists and force=false,
-		// skip the download entirely (no unnecessary network round-trip).
+		// skip the download entirely (no unnecessary network round-trip). However, still
+		// recurse into the existing file's imports so that any transitive dependencies
+		// it references are fetched even when the parent file was already present.
 		fileExists := false
 		if fileutil.FileExists(targetPath) {
 			fileExists = true
 			if !opts.force {
 				if opts.verbose {
 					fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Import file already exists, skipping: "+targetPath))
 				}
+				// Read the existing file so we can recurse into its own imports.
+				// If the read fails, log it and skip — the compiler will report
+				// any missing transitive dependencies.
+				if existingContent, readErr := os.ReadFile(targetPath); readErr == nil {
+					importedBaseDir := path.Dir(remoteFilePath)
+					fetchFrontmatterImportsRecursive(ctx, string(existingContent), importedBaseDir, opts)
+				} else {
+					remoteWorkflowLog.Printf("Failed to read existing import %s for recursion: %v", targetPath, readErr)
+				}
 				continue
 			}
 		}
 
 		// Download from the source repository
-		importContent, err := parser.DownloadFileFromGitHub(ctx, opts.owner, opts.repo, remoteFilePath, opts.ref)
+		downloadFn := opts.downloadFn
+		if downloadFn == nil {
+			downloadFn = parser.DownloadFileFromGitHub
+		}
+		importContent, err := downloadFn(ctx, opts.owner, opts.repo, remoteFilePath, opts.ref)
 		if err != nil {
 			remoteWorkflowLog.Printf("Failed to download import %s from %s/%s@%s: %v", remoteFilePath, opts.owner, opts.repo, opts.ref, err)
 			if opts.verbose {

pkg/cli/remote_workflow_test.go

@@ -686,6 +686,171 @@ imports:
 	assert.Empty(t, entries, "no files should be created for an invalid RepoSlug")
 }
 
+// TestFetchFrontmatterImportsRecursive_SiblingPathResolution verifies that when a
+// shared file imports a sibling using a bare filename (no "/" character), the path
+// is resolved relative to the importing file's directory rather than the original
+// workflow base directory. This mirrors the compiler's determineNestedBaseDir logic.
+func TestFetchFrontmatterImportsRecursive_SiblingPathResolution(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// Track which remote paths the downloader is asked for, and return minimal content.
+	var downloadedPaths []string
+	mockDownload := func(_ context.Context, _, _, remoteFilePath, _ string) ([]byte, error) {
+		downloadedPaths = append(downloadedPaths, remoteFilePath)
+		return []byte("# stub\n"), nil
+	}
+
+	// shared/control.md imports a sibling via a bare filename (no "/").
+	controlContent := `---
+imports:
+  - control-aux.md
+---
+# Control
+`
+
+	seen := make(map[string]struct{})
+	opts := frontmatterImportsOpts{
+		owner:           "github",
+		repo:            "gh-aw",
+		ref:             "main",
+		originalBaseDir: ".github/workflows",
+		targetDir:       tmpDir,
+		verbose:         false,
+		force:           true, // force=true so the download step is always reached
+		tracker:         &FileTracker{OriginalContent: make(map[string][]byte)},
+		seen:            seen,
+		downloadFn:      mockDownload,
+	}
+
+	// Process shared/control.md; currentBaseDir mirrors its location in the source repo.
+	fetchFrontmatterImportsRecursive(t.Context(), controlContent, ".github/workflows/shared", opts)
+
+	// "control-aux.md" (bare filename, no "/") must resolve relative to the importing
+	// file's directory → ".github/workflows/shared/control-aux.md".
+	require.Len(t, downloadedPaths, 1, "downloader must be called exactly once")
+	assert.Equal(t, ".github/workflows/shared/control-aux.md", downloadedPaths[0],
+		"sibling import must resolve to the shared/ subdir, not the workflow root")
+
+	// The file must be saved at the correct local path inside targetDir.
+	assert.FileExists(t, filepath.Join(tmpDir, "shared", "control-aux.md"),
+		"file must be written to shared/control-aux.md")
+	assert.NoFileExists(t, filepath.Join(tmpDir, "control-aux.md"),
+		"file must not be written at the root level (wrong path resolution)")
+}
+
+// TestFetchFrontmatterImportsRecursive_RecurseIntoExistingFile verifies that when a
+// top-level import already exists on disk (force=false), the function still recurses
+// into that file's imports so that transitive dependencies are fetched. This test
+// also covers sibling-path resolution: the transitive dep uses a bare filename, so it
+// should resolve to the shared file's directory, not the workflow root.
+func TestFetchFrontmatterImportsRecursive_RecurseIntoExistingFile(t *testing.T) {
+	tmpDir := t.TempDir()
+	sharedDir := filepath.Join(tmpDir, "shared")
+	require.NoError(t, os.MkdirAll(sharedDir, 0755))
+
+	// shared/control.md already exists locally; it imports control-aux.md (a sibling).
+	controlContent := `---
+imports:
+  - control-aux.md
+---
+# Control
+`
+	require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "control.md"), []byte(controlContent), 0600))
+
+	// Track which remote paths are downloaded.
+	var downloadedPaths []string
+	mockDownload := func(_ context.Context, _, _, remoteFilePath, _ string) ([]byte, error) {
+		downloadedPaths = append(downloadedPaths, remoteFilePath)
+		return []byte("# stub\n"), nil
+	}
+
+	// Top-level content imports shared/control.md (which already exists on disk).
+	topContent := `---
+engine: copilot
+imports:
+  - shared/control.md
+---
+# Orchestrator
+`
+
+	seen := make(map[string]struct{})
+	opts := frontmatterImportsOpts{
+		owner:           "github",
+		repo:            "gh-aw",
+		ref:             "v1.0.0",
+		originalBaseDir: ".github/workflows",
+		targetDir:       tmpDir,
+		verbose:         false,
+		force:           false, // force=false so pre-existing files trigger the "skip + recurse" path
+		tracker:         &FileTracker{OriginalContent: make(map[string][]byte)},
+		seen:            seen,
+		downloadFn:      mockDownload,
+	}
+
+	fetchFrontmatterImportsRecursive(t.Context(), topContent, ".github/workflows", opts)
+
+	// shared/control.md existed on disk → must NOT be downloaded.
+	assert.NotContains(t, downloadedPaths, ".github/workflows/shared/control.md",
+		"pre-existing shared/control.md must not be re-downloaded")
+
+	// The function must recurse into the existing file and discover its sibling dep.
+	// control-aux.md (bare filename, no "/") must resolve to shared/control-aux.md.
+	require.Len(t, downloadedPaths, 1, "transitive dep control-aux.md must be fetched exactly once")
+	assert.Equal(t, ".github/workflows/shared/control-aux.md", downloadedPaths[0],
+		"transitive dep must be fetched from the shared/ subdir")
+
+	// The transitive dep must be saved at the correct local path.
+	assert.FileExists(t, filepath.Join(tmpDir, "shared", "control-aux.md"),
+		"transitive dep must be written to shared/control-aux.md")
+	assert.NoFileExists(t, filepath.Join(tmpDir, "control-aux.md"),
+		"transitive dep must not be written at the root level")
+}
+
+// TestFetchFrontmatterImportsRecursive_RepoRootSlashPath verifies that when
+// originalBaseDir is empty (workflow lives at the repo root) and an import
+// path contains a "/" (e.g. "shared/helper.md"), the path is used as-is
+// rather than being incorrectly prefixed with currentBaseDir.
+func TestFetchFrontmatterImportsRecursive_RepoRootSlashPath(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	var downloadedPaths []string
+	mockDownload := func(_ context.Context, _, _, remoteFilePath, _ string) ([]byte, error) {
+		downloadedPaths = append(downloadedPaths, remoteFilePath)
+		return []byte("# stub\n"), nil
+	}
+
+	// shared/control.md imports shared/helper.md (a slash path).
+	controlContent := `---
+imports:
+  - shared/helper.md
+---
+# Control
+`
+
+	seen := make(map[string]struct{})
+	opts := frontmatterImportsOpts{
+		owner:           "github",
+		repo:            "gh-aw",
+		ref:             "main",
+		originalBaseDir: "", // empty: workflow lives at the repo root
+		targetDir:       tmpDir,
+		verbose:         false,
+		force:           true,
+		tracker:         &FileTracker{OriginalContent: make(map[string][]byte)},
+		seen:            seen,
+		downloadFn:      mockDownload,
+	}
+
+	// currentBaseDir mirrors shared/control.md's location.
+	fetchFrontmatterImportsRecursive(t.Context(), controlContent, "shared", opts)
+
+	// "shared/helper.md" (slash path, empty originalBaseDir) must be used as-is,
+	// NOT prefixed with currentBaseDir ("shared/shared/helper.md" would be wrong).
+	require.Len(t, downloadedPaths, 1, "downloader must be called exactly once")
+	assert.Equal(t, "shared/helper.md", downloadedPaths[0],
+		"slash import with empty originalBaseDir must be used as-is")
+}
+
 // --- extractDispatchWorkflowNames tests ---
 
 // TestExtractDispatchWorkflowNames_ArrayFormat verifies that workflow names are extracted

pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden

@@ -35,6 +35,7 @@ jobs:
       engine_id: ${{ steps.generate_aw_info.outputs.engine_id }}
       lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }}
       model: ${{ steps.generate_aw_info.outputs.model }}
+      oauth_token_check_failed: ${{ steps.check-oauth-tokens.outputs.oauth_token_check_failed == 'true' }}
       secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }}
       setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }}
       setup-span-id: ${{ steps.setup.outputs.span-id }}

pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden

@@ -35,6 +35,7 @@ jobs:
       engine_id: ${{ steps.generate_aw_info.outputs.engine_id }}
       lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }}
       model: ${{ steps.generate_aw_info.outputs.model }}
+      oauth_token_check_failed: ${{ steps.check-oauth-tokens.outputs.oauth_token_check_failed == 'true' }}
       secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }}
       setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }}
       setup-span-id: ${{ steps.setup.outputs.span-id }}

pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden

@@ -35,6 +35,7 @@ jobs:
       engine_id: ${{ steps.generate_aw_info.outputs.engine_id }}
       lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }}
       model: ${{ steps.generate_aw_info.outputs.model }}
+      oauth_token_check_failed: ${{ steps.check-oauth-tokens.outputs.oauth_token_check_failed == 'true' }}
       secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }}
       setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }}
       setup-span-id: ${{ steps.setup.outputs.span-id }}

pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden

@@ -35,6 +35,7 @@ jobs:
       engine_id: ${{ steps.generate_aw_info.outputs.engine_id }}
       lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }}
       model: ${{ steps.generate_aw_info.outputs.model }}
+      oauth_token_check_failed: ${{ steps.check-oauth-tokens.outputs.oauth_token_check_failed == 'true' }}
       secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }}
       setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }}
       setup-span-id: ${{ steps.setup.outputs.span-id }}

pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden

@@ -35,6 +35,7 @@ jobs:
       engine_id: ${{ steps.generate_aw_info.outputs.engine_id }}
       lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }}
       model: ${{ steps.generate_aw_info.outputs.model }}
+      oauth_token_check_failed: ${{ steps.check-oauth-tokens.outputs.oauth_token_check_failed == 'true' }}
       secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }}
       setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }}
       setup-span-id: ${{ steps.setup.outputs.span-id }}