diff --git a/docs/adr/44763-resolve-transitive-uses-imports-by-path-structure.md b/docs/adr/44763-resolve-transitive-uses-imports-by-path-structure.md new file mode 100644 index 00000000000..053761f5786 --- /dev/null +++ b/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.* diff --git a/pkg/cli/includes.go b/pkg/cli/includes.go index 6b4378eb986..c9ee684f39f 100644 --- a/pkg/cli/includes.go +++ b/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,7 +361,9 @@ 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 @@ -357,12 +371,25 @@ func fetchFrontmatterImportsRecursive(ctx context.Context, content, currentBaseD 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 { diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index df379eb713a..8a4a1364cbe 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/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 diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden index 17133844ef0..4c992a103f5 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden +++ b/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 }} diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden index a6db7fa51f4..5bc656b6f80 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden +++ b/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 }} diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden index 66e51906d44..27852b9e85a 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden +++ b/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 }} diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden index a3905c97768..ec7bcdc60b2 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden +++ b/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 }} diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden index 8d0d9578828..6ff5fa038ca 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden +++ b/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 }}