From 877268589e2cfae84a3e91a833736754e5ffe500 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 04:11:56 +0000 Subject: [PATCH 1/4] Initial plan From 89f862d95ac6b49e970f652b1611f3b1dfbfdabc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 04:37:08 +0000 Subject: [PATCH 2/4] fix: gh aw add rewrites {{#import}} with incorrect cross-repo path - Use resolveImportPath in processIncludesWithWorkflowSpec so body-level {{#import shared/X.md}} paths are resolved relative to the workflow file's directory (.github/workflows/) rather than the repo root. Before: owner/repo/shared/config.md@SHA After: owner/repo/.github/workflows/shared/config.md@SHA - Add localWorkflowDir parameter to processIncludesWithWorkflowSpec so imports whose target files already exist locally are preserved as-is, matching the behaviour already in processIncludesInContent (update cmd). - Update add_command.go to pass githubWorkflowsDir as localWorkflowDir so consuming repos with their own local shared/ files are not forced onto cross-repo references after every gh aw add. - Add 3 new tests: PathResolution, PreservesLocalIncludes, RewritesBodyWhenLocalMissing. Closes #(issue) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/106643c3-a705-4a25-9619-35c55a98d102 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../patch-fix-body-import-path-resolution.md | 5 + pkg/cli/add_command.go | 9 +- pkg/cli/imports.go | 34 ++++- pkg/cli/imports_test.go | 140 +++++++++++++++++- 4 files changed, 174 insertions(+), 14 deletions(-) create mode 100644 .changeset/patch-fix-body-import-path-resolution.md diff --git a/.changeset/patch-fix-body-import-path-resolution.md b/.changeset/patch-fix-body-import-path-resolution.md new file mode 100644 index 00000000000..e43c010bc1c --- /dev/null +++ b/.changeset/patch-fix-body-import-path-resolution.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fix `gh aw add` incorrectly resolving body-level `{{#import shared/X.md}}` from the repo root instead of the workflow file's directory (`.github/workflows/`). Also preserve body-level imports as local references when the target file already exists in the consuming repository, matching the behaviour already implemented for `gh aw update`. diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 00c7ec40c18..605128957e7 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -416,13 +416,16 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o // fetchAndSaveRemoteFrontmatterImports already downloaded those files locally, so // the compiler can resolve them from disk without any GitHub API calls. - // Process @include directives and replace with workflowspec - // For local workflows, use the workflow's directory as the base path + // Process @include directives and replace with workflowspec. + // For local workflows, use the workflow's directory as the package source path. + // Pass githubWorkflowsDir as localWorkflowDir so that any body-level import + // whose target already exists locally is preserved as a local reference rather + // than being rewritten to a cross-repo workflowspec. includeSourceDir := "" if sourceInfo != nil && sourceInfo.IsLocal { includeSourceDir = filepath.Dir(workflowSpec.WorkflowPath) } - processedContent, err := processIncludesWithWorkflowSpec(content, workflowSpec, commitSHA, includeSourceDir, opts.Verbose) + processedContent, err := processIncludesWithWorkflowSpec(content, workflowSpec, commitSHA, includeSourceDir, githubWorkflowsDir, opts.Verbose) if err != nil { if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process includes: %v", err))) diff --git a/pkg/cli/imports.go b/pkg/cli/imports.go index c96df5fc9f9..1dee4461433 100644 --- a/pkg/cli/imports.go +++ b/pkg/cli/imports.go @@ -182,9 +182,11 @@ func reconstructWorkflowFileFromMap(frontmatter map[string]any, markdown string) } // processIncludesWithWorkflowSpec processes @include directives in content and replaces local file references -// with workflowspec format (owner/repo/path@sha) for all includes found in the package -func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, commitSHA, packagePath string, verbose bool) (string, error) { - importsLog.Printf("Processing @include directives: repo=%s, sha=%s, package=%s", workflow.RepoSlug, commitSHA, packagePath) +// with workflowspec format (owner/repo/path@sha) for all includes found in the package. +// If localWorkflowDir is non-empty, any relative import path whose file exists under that directory is +// left as a local relative path rather than being rewritten to a cross-repo reference. +func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, commitSHA, packagePath, localWorkflowDir string, verbose bool) (string, error) { + importsLog.Printf("Processing @include directives: repo=%s, sha=%s, package=%s, localWorkflowDir=%s", workflow.RepoSlug, commitSHA, packagePath, localWorkflowDir) if verbose { fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Processing @include directives to replace with workflowspec")) } @@ -211,6 +213,12 @@ func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, com isOptional := directive.IsOptional includePath := directive.Path + // Skip if it's already a workflowspec (owner/repo/path@sha format) + if isWorkflowSpecFormat(includePath) { + result.WriteString(line + "\n") + continue + } + // Handle section references (file.md#Section) var filePath, sectionName string if strings.Contains(includePath, "#") { @@ -241,8 +249,22 @@ func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, com // Mark as visited visited[filePath] = true + // Preserve relative {{#import}} paths whose files exist in the local workflow directory. + if localWorkflowDir != "" && !strings.HasPrefix(filePath, "/") { + if isLocalFileForUpdate(localWorkflowDir, filePath) { + importsLog.Printf("Include path exists locally, preserving: %s", filePath) + result.WriteString(line + "\n") + // Add file to queue for processing nested includes + queue = append(queue, fileToProcess{path: filePath}) + continue + } + } + + // Resolve the file path relative to the workflow file's directory + resolvedPath := resolveImportPath(filePath, workflow.WorkflowPath) + // Build workflowspec for this include - workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, filePath, commitSHA, workflow.Version) + workflowSpec := buildWorkflowSpecRef(workflow.RepoSlug, resolvedPath, commitSHA, workflow.Version) // Add section if present if sectionName != "" { @@ -360,7 +382,7 @@ func processIncludesInContent(content string, workflow *WorkflowSpec, commitSHA isOptional := directive.IsOptional includePath := directive.Path - // Skip if it's already a workflowspec (contains repo/path format) + // Skip if it's already a workflowspec (owner/repo/path@sha format) if isWorkflowSpecFormat(includePath) { result.WriteString(line + "\n") continue @@ -385,7 +407,7 @@ func processIncludesInContent(content string, workflow *WorkflowSpec, commitSHA continue } - // Preserve relative @include paths whose files exist in the local workflow directory. + // Preserve relative {{#import}} paths whose files exist in the local workflow directory. if localWorkflowDir != "" && !strings.HasPrefix(filePath, "/") { if isLocalFileForUpdate(localWorkflowDir, filePath) { importsLog.Printf("Include path exists locally, preserving: %s", filePath) diff --git a/pkg/cli/imports_test.go b/pkg/cli/imports_test.go index 8b1dfa68f61..12fa2d1d2c2 100644 --- a/pkg/cli/imports_test.go +++ b/pkg/cli/imports_test.go @@ -30,7 +30,7 @@ More content. }, } - result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", false) + result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", "", false) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -70,7 +70,7 @@ More content. }, } - result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", false) + result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", "", false) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -101,7 +101,7 @@ engine: claude commitSHA := "e2770974a7eaccb58ddafd5606c38a05ba52c631" - result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "/tmp/package", false) + result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "/tmp/package", "", false) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -133,7 +133,7 @@ More content. }, } - result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", false) + result, err := processIncludesWithWorkflowSpec(content, workflow, "", "/tmp/package", "", false) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -246,7 +246,7 @@ Do research. commitSHA := "e2770974a7eaccb58ddafd5606c38a05ba52c631" - result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "/tmp/package", false) + result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "/tmp/package", "", false) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -264,6 +264,136 @@ Do research. } } +// TestProcessIncludesWithWorkflowSpec_PathResolution tests that body-level {{#import}} +// paths are resolved relative to the workflow file's location, not the repo root. +// Regression test for: gh aw add rewrites {{#import shared/X.md}} with incorrect +// cross-repo path (resolves from repo root instead of .github/workflows/). +func TestProcessIncludesWithWorkflowSpec_PathResolution(t *testing.T) { + content := `--- +engine: copilot +--- + +# My Workflow + +{{#import shared/config.md}} +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/source-repo", + }, + WorkflowPath: ".github/workflows/my-workflow.md", + } + + commitSHA := "abc123def456" + + result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "", "", false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // Path should be resolved relative to .github/workflows/, not the repo root + expectedInclude := "{{#import github/source-repo/.github/workflows/shared/config.md@abc123def456}}" + if !strings.Contains(result, expectedInclude) { + t.Errorf("Expected result to contain '%s'\nGot:\n%s", expectedInclude, result) + } + + // The old wrong form (resolving from repo root) must not appear + wrongPath := "{{#import github/source-repo/shared/config.md@abc123def456}}" + if strings.Contains(result, wrongPath) { + t.Errorf("Result must NOT contain repo-root-relative path '%s'\nGot:\n%s", wrongPath, result) + } +} + +// TestProcessIncludesWithWorkflowSpec_PreservesLocalIncludes tests that body-level +// {{#import}} directives are preserved as-is when the target file exists in the +// local workflow directory. This is the add-command equivalent of the update-command's +// local preservation fix. +func TestProcessIncludesWithWorkflowSpec_PreservesLocalIncludes(t *testing.T) { + // Create a temporary directory to act as the local workflow directory + tmpDir := t.TempDir() + if err := os.MkdirAll(tmpDir+"/shared", 0755); err != nil { + t.Fatalf("Failed to create shared dir: %v", err) + } + if err := os.WriteFile(tmpDir+"/shared/config.md", []byte("# Local config"), 0644); err != nil { + t.Fatalf("Failed to create local config file: %v", err) + } + + content := `--- +engine: copilot +--- + +# My Workflow + +{{#import shared/config.md}} +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/source-repo", + }, + WorkflowPath: ".github/workflows/my-workflow.md", + } + + commitSHA := "abc123def456" + + result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "", tmpDir, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // The import must be preserved as-is since the file exists locally + if !strings.Contains(result, "{{#import shared/config.md}}") { + t.Errorf("Expected local import to be preserved, got:\n%s", result) + } + + // Cross-repo ref must NOT appear + if strings.Contains(result, "github/source-repo") { + t.Errorf("Cross-repo ref should NOT appear when local file exists, got:\n%s", result) + } +} + +// TestProcessIncludesWithWorkflowSpec_RewritesBodyWhenLocalMissing tests that body-level +// {{#import}} directives are rewritten to cross-repo refs when the target does not +// exist in the local workflow directory. +func TestProcessIncludesWithWorkflowSpec_RewritesBodyWhenLocalMissing(t *testing.T) { + tmpDir := t.TempDir() // empty — no shared files present + + content := `--- +engine: copilot +--- + +# My Workflow + +{{#import shared/config.md}} +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/source-repo", + }, + WorkflowPath: ".github/workflows/my-workflow.md", + } + + commitSHA := "abc123def456" + + result, err := processIncludesWithWorkflowSpec(content, workflow, commitSHA, "", tmpDir, false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // File not present locally → must be rewritten with correct full path + expectedRef := "{{#import github/source-repo/.github/workflows/shared/config.md@abc123def456}}" + if !strings.Contains(result, expectedRef) { + t.Errorf("Expected cross-repo ref '%s' when file is missing locally, got:\n%s", expectedRef, result) + } + + // Relative path must not remain + if strings.Contains(result, "{{#import shared/config.md}}") { + t.Errorf("Relative path should have been rewritten when file is missing locally, got:\n%s", result) + } +} + func TestIsWorkflowSpecFormat(t *testing.T) { tests := []struct { name string From 8c43068822d9d5a0f69bbbfb214f39faf63be755 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 04:47:48 +0000 Subject: [PATCH 3/4] test: add comprehensive path-type tests for body and frontmatter imports Add two table-driven tests covering every meaningful import-path variant: TestProcessIncludesWithWorkflowSpec_PathTypes (15 sub-tests): - simple relative (shared/config.md) - optional flag preserved ({{#import?}}) - deep nested path - absolute path (/tools/config.md) - path with section reference (#Section) - optional + section - already a workflowspec (passed through) - already workflowspec with section - section-only reference (#SectionName) - parent dir (../shared/config.md) - current dir (./config.md) - legacy @include syntax - legacy optional @include? syntax - three-segment path (no @ sign, treated as local) - plain filename TestProcessImportsWithWorkflowSpec_PathTypes (8 sub-tests): - same path categories for the frontmatter imports: field Agent-Logs-Url: https://github.com/github/gh-aw/sessions/47f3fec7-c300-4ecb-9103-6c1d1a96903a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/imports_test.go | 231 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 231 insertions(+) diff --git a/pkg/cli/imports_test.go b/pkg/cli/imports_test.go index 12fa2d1d2c2..78c0bb394ab 100644 --- a/pkg/cli/imports_test.go +++ b/pkg/cli/imports_test.go @@ -394,6 +394,237 @@ engine: copilot } } +// TestProcessIncludesWithWorkflowSpec_PathTypes exercises every meaningful import-path +// variant for the body-level {{#import}} processor used by `gh aw add`. +// The workflow file is assumed to live at `.github/workflows/my-workflow.md` inside +// "acme/repo", pinned to commit "deadbeef". +func TestProcessIncludesWithWorkflowSpec_PathTypes(t *testing.T) { + workflowPath := ".github/workflows/my-workflow.md" + repoSlug := "acme/repo" + sha := "deadbeef" + + tests := []struct { + name string + line string // body line to process + wantLine string // expected line in output + notWant string // substring that must NOT appear (optional) + }{ + { + // Simple two-segment relative path: resolved relative to workflow dir + name: "simple relative shared/file.md", + line: "{{#import shared/config.md}}", + wantLine: "{{#import acme/repo/.github/workflows/shared/config.md@deadbeef}}", + notWant: "acme/repo/shared/config.md@deadbeef", + }, + { + // Optional flag must be preserved + name: "optional relative path", + line: "{{#import? shared/config.md}}", + wantLine: "{{#import? acme/repo/.github/workflows/shared/config.md@deadbeef}}", + }, + { + // Deep nested relative path + name: "deep relative path shared/mcp/deep/file.md", + line: "{{#import shared/mcp/deep/file.md}}", + wantLine: "{{#import acme/repo/.github/workflows/shared/mcp/deep/file.md@deadbeef}}", + }, + { + // Absolute path (starts with /): strips leading slash, repo-root relative + name: "absolute path /tools/config.md", + line: "{{#import /tools/config.md}}", + wantLine: "{{#import acme/repo/tools/config.md@deadbeef}}", + }, + { + // Path with section reference: section preserved after @sha + name: "relative path with section", + line: "{{#import shared/config.md#Introduction}}", + wantLine: "{{#import acme/repo/.github/workflows/shared/config.md@deadbeef#Introduction}}", + }, + { + // Optional path with section + name: "optional relative path with section", + line: "{{#import? shared/config.md#Setup}}", + wantLine: "{{#import? acme/repo/.github/workflows/shared/config.md@deadbeef#Setup}}", + }, + { + // Already a workflowspec (contains @): must be passed through unchanged + name: "already a workflowspec", + line: "{{#import other/repo/file.md@abc123}}", + wantLine: "{{#import other/repo/file.md@abc123}}", + }, + { + // Already a workflowspec with section: pass through unchanged + name: "already a workflowspec with section", + line: "{{#import other/repo/file.md@abc123#Section}}", + wantLine: "{{#import other/repo/file.md@abc123#Section}}", + }, + { + // Section-only reference (empty file path): preserved as-is + name: "section-only #SectionName", + line: "{{#import? #SectionName}}", + wantLine: "{{#import? #SectionName}}", + notWant: "acme/repo", + }, + { + // Parent directory traversal: resolves up from .github/workflows/ + // ../shared/config.md from .github/workflows/ → .github/shared/config.md + name: "parent dir ../shared/config.md", + line: "{{#import ../shared/config.md}}", + wantLine: "{{#import acme/repo/.github/shared/config.md@deadbeef}}", + }, + { + // Explicit current-dir prefix ./ + name: "current dir ./config.md", + line: "{{#import ./config.md}}", + wantLine: "{{#import acme/repo/.github/workflows/config.md@deadbeef}}", + }, + { + // Legacy @include syntax: output must use new {{#import}} syntax + name: "legacy @include shared/config.md", + line: "@include shared/config.md", + wantLine: "{{#import acme/repo/.github/workflows/shared/config.md@deadbeef}}", + }, + { + // Legacy optional @include? syntax + name: "legacy @include? optional", + line: "@include? shared/config.md", + wantLine: "{{#import? acme/repo/.github/workflows/shared/config.md@deadbeef}}", + }, + { + // Three-segment path that has no @: treated as local relative, not a workflowspec + name: "three-segment path shared/mcp/arxiv.md", + line: "{{#import shared/mcp/arxiv.md}}", + wantLine: "{{#import acme/repo/.github/workflows/shared/mcp/arxiv.md@deadbeef}}", + }, + { + // Plain filename (single segment) + name: "plain filename config.md", + line: "{{#import config.md}}", + wantLine: "{{#import acme/repo/.github/workflows/config.md@deadbeef}}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Wrap the single import line in a minimal workflow + content := "---\nengine: copilot\n---\n\n" + tt.line + "\n" + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: repoSlug, + }, + WorkflowPath: workflowPath, + } + + result, err := processIncludesWithWorkflowSpec(content, workflow, sha, "", "", false) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if !strings.Contains(result, tt.wantLine) { + t.Errorf("Expected output to contain:\n %q\nGot:\n%s", tt.wantLine, result) + } + + if tt.notWant != "" && strings.Contains(result, tt.notWant) { + t.Errorf("Output must NOT contain %q\nGot:\n%s", tt.notWant, result) + } + }) + } +} + +// TestProcessImportsWithWorkflowSpec_PathTypes exercises every meaningful import-path +// variant for the frontmatter imports: field processor used by both `gh aw add` and +// `gh aw update`. +func TestProcessImportsWithWorkflowSpec_PathTypes(t *testing.T) { + workflowPath := ".github/workflows/my-workflow.md" + repoSlug := "acme/repo" + sha := "deadbeef" + + tests := []struct { + name string + importPath string // raw value in the imports: list + wantRef string // expected substring in YAML output + notWant string // substring that must NOT appear (optional) + }{ + { + // Simple two-segment relative path + name: "simple relative shared/config.md", + importPath: "shared/config.md", + wantRef: "acme/repo/.github/workflows/shared/config.md@deadbeef", + notWant: "acme/repo/shared/config.md@deadbeef", + }, + { + // Deep nested relative path + name: "deep relative shared/mcp/file.md", + importPath: "shared/mcp/file.md", + wantRef: "acme/repo/.github/workflows/shared/mcp/file.md@deadbeef", + }, + { + // Absolute path (starts with /) + name: "absolute /tools/setup.md", + importPath: "/tools/setup.md", + wantRef: "acme/repo/tools/setup.md@deadbeef", + }, + { + // Already a workflowspec: must be passed through unchanged + name: "already workflowspec other/repo/file.md@v1", + importPath: "other/repo/file.md@v1", + wantRef: "other/repo/file.md@v1", + notWant: "acme/repo/other/repo", + }, + { + // Three-segment path with no @: treated as relative, NOT a workflowspec + name: "three-segment shared/mcp/arxiv.md", + importPath: "shared/mcp/arxiv.md", + wantRef: "acme/repo/.github/workflows/shared/mcp/arxiv.md@deadbeef", + }, + { + // Parent dir traversal + name: "parent dir ../shared/config.md", + importPath: "../shared/config.md", + wantRef: "acme/repo/.github/shared/config.md@deadbeef", + }, + { + // Current-dir prefix + name: "current dir ./config.md", + importPath: "./config.md", + wantRef: "acme/repo/.github/workflows/config.md@deadbeef", + }, + { + // Plain filename + name: "plain filename config.md", + importPath: "config.md", + wantRef: "acme/repo/.github/workflows/config.md@deadbeef", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := "---\nengine: copilot\nimports:\n - " + tt.importPath + "\n---\n\n# Test\n" + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: repoSlug, + }, + WorkflowPath: workflowPath, + } + + result, err := processImportsWithWorkflowSpec(content, workflow, sha, "", false) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if !strings.Contains(result, tt.wantRef) { + t.Errorf("Expected output to contain:\n %q\nGot:\n%s", tt.wantRef, result) + } + + if tt.notWant != "" && strings.Contains(result, tt.notWant) { + t.Errorf("Output must NOT contain %q\nGot:\n%s", tt.notWant, result) + } + }) + } +} + func TestIsWorkflowSpecFormat(t *testing.T) { tests := []struct { name string From e40ad58a2b27c0fdf71ba7e04e06dba6684254e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 05:17:03 +0000 Subject: [PATCH 4/4] fix: preserve duplicate imports and use filepath.Join in tests - Fix cycle-detection in processIncludesWithWorkflowSpec: mark visited and skip re-enqueueing only on second occurrence, but still write the rewritten directive so duplicate {{#import}} lines are preserved in the output - Add TestProcessIncludesWithWorkflowSpec_DuplicateInclude to cover the fix - Replace tmpDir+"/..." string concatenation in tests with filepath.Join(...) for OS-agnosticism (addresses code review feedback) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f99a3393-1a0c-42bb-acb7-68920cee5eb6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/imports.go | 27 +++++++++------------ pkg/cli/imports_test.go | 54 +++++++++++++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/pkg/cli/imports.go b/pkg/cli/imports.go index 1dee4461433..b3f71760bd2 100644 --- a/pkg/cli/imports.go +++ b/pkg/cli/imports.go @@ -238,24 +238,16 @@ func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, com continue } - // Check for cycle detection - if visited[filePath] { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Cycle detected for include: %s, skipping", filePath))) - } - continue - } - - // Mark as visited - visited[filePath] = true - // Preserve relative {{#import}} paths whose files exist in the local workflow directory. if localWorkflowDir != "" && !strings.HasPrefix(filePath, "/") { if isLocalFileForUpdate(localWorkflowDir, filePath) { importsLog.Printf("Include path exists locally, preserving: %s", filePath) result.WriteString(line + "\n") - // Add file to queue for processing nested includes - queue = append(queue, fileToProcess{path: filePath}) + // Add file to queue for processing nested includes (first visit only) + if !visited[filePath] { + visited[filePath] = true + queue = append(queue, fileToProcess{path: filePath}) + } continue } } @@ -271,15 +263,18 @@ func processIncludesWithWorkflowSpec(content string, workflow *WorkflowSpec, com workflowSpec += "#" + sectionName } - // Write the updated @include directive + // Write the updated @include directive (even for duplicate occurrences) if isOptional { result.WriteString("{{#import? " + workflowSpec + "}}\n") } else { result.WriteString("{{#import " + workflowSpec + "}}\n") } - // Add file to queue for processing nested includes - queue = append(queue, fileToProcess{path: filePath}) + // Only enqueue for nested-include processing on the first visit to prevent cycles + if !visited[filePath] { + visited[filePath] = true + queue = append(queue, fileToProcess{path: filePath}) + } } else { // Regular line, pass through result.WriteString(line + "\n") diff --git a/pkg/cli/imports_test.go b/pkg/cli/imports_test.go index 78c0bb394ab..8f86bae852c 100644 --- a/pkg/cli/imports_test.go +++ b/pkg/cli/imports_test.go @@ -4,6 +4,7 @@ package cli import ( "os" + "path/filepath" "strings" "testing" ) @@ -312,10 +313,10 @@ engine: copilot func TestProcessIncludesWithWorkflowSpec_PreservesLocalIncludes(t *testing.T) { // Create a temporary directory to act as the local workflow directory tmpDir := t.TempDir() - if err := os.MkdirAll(tmpDir+"/shared", 0755); err != nil { + if err := os.MkdirAll(filepath.Join(tmpDir, "shared"), 0755); err != nil { t.Fatalf("Failed to create shared dir: %v", err) } - if err := os.WriteFile(tmpDir+"/shared/config.md", []byte("# Local config"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(tmpDir, "shared", "config.md"), []byte("# Local config"), 0644); err != nil { t.Fatalf("Failed to create local config file: %v", err) } @@ -394,6 +395,43 @@ engine: copilot } } +// TestProcessIncludesWithWorkflowSpec_DuplicateInclude verifies that a body-level +// {{#import}} directive that appears more than once is preserved in full for each +// occurrence, rather than being silently dropped on the second occurrence by the +// cycle-detection guard. +func TestProcessIncludesWithWorkflowSpec_DuplicateInclude(t *testing.T) { + content := `--- +engine: copilot +--- + +# My Workflow + +{{#import shared/config.md}} + +Some text. + +{{#import shared/config.md}} +` + + workflow := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "acme/repo", + }, + WorkflowPath: ".github/workflows/my-workflow.md", + } + + result, err := processIncludesWithWorkflowSpec(content, workflow, "deadbeef", "", "", false) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + expectedRef := "{{#import acme/repo/.github/workflows/shared/config.md@deadbeef}}" + count := strings.Count(result, expectedRef) + if count != 2 { + t.Errorf("Expected rewritten import to appear 2 times, got %d\nOutput:\n%s", count, result) + } +} + // TestProcessIncludesWithWorkflowSpec_PathTypes exercises every meaningful import-path // variant for the body-level {{#import}} processor used by `gh aw add`. // The workflow file is assumed to live at `.github/workflows/my-workflow.md` inside @@ -756,11 +794,11 @@ func TestProcessImportsWithWorkflowSpec_PreservesLocalRelativePaths(t *testing.T // Create the shared import files locally for _, rel := range []string{"shared/team-config.md", "shared/aor-index.md"} { - dir := tmpDir + "/" + rel[:strings.LastIndex(rel, "/")] + dir := filepath.Join(tmpDir, rel[:strings.LastIndex(rel, "/")]) if err := os.MkdirAll(dir, 0755); err != nil { t.Fatalf("Failed to create dir %s: %v", dir, err) } - if err := os.WriteFile(tmpDir+"/"+rel, []byte("# Shared content"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(tmpDir, rel), []byte("# Shared content"), 0644); err != nil { t.Fatalf("Failed to create file %s: %v", rel, err) } } @@ -847,10 +885,10 @@ imports: func TestProcessIncludesInContent_PreservesLocalIncludeDirectives(t *testing.T) { // Create a temporary directory with the shared include file tmpDir := t.TempDir() - if err := os.MkdirAll(tmpDir+"/shared", 0755); err != nil { + if err := os.MkdirAll(filepath.Join(tmpDir, "shared"), 0755); err != nil { t.Fatalf("Failed to create shared dir: %v", err) } - if err := os.WriteFile(tmpDir+"/shared/config.md", []byte("# Config"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(tmpDir, "shared", "config.md"), []byte("# Config"), 0644); err != nil { t.Fatalf("Failed to create config file: %v", err) } @@ -905,10 +943,10 @@ func TestIsLocalFileForUpdate_PathTraversal(t *testing.T) { // A normal path within tmpDir that DOES exist should return true validFile := "shared/file.md" - if err := os.MkdirAll(tmpDir+"/shared", 0755); err != nil { + if err := os.MkdirAll(filepath.Join(tmpDir, "shared"), 0755); err != nil { t.Fatal(err) } - if err := os.WriteFile(tmpDir+"/"+validFile, []byte("content"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(tmpDir, validFile), []byte("content"), 0644); err != nil { t.Fatal(err) } if !isLocalFileForUpdate(tmpDir, validFile) {