From 3c00411f00aa8444f13ddcb519fce08469d28ff5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 17:49:30 +0000 Subject: [PATCH 1/6] Initial plan From a7f23b60c003b87df64a059193bfbc3f892ceef0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 18:20:44 +0000 Subject: [PATCH 2/6] fix: fetch frontmatter imports locally during add-wizard to prevent compilation failure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 20 ++--- pkg/cli/remote_workflow.go | 137 ++++++++++++++++++++++++++++++++ pkg/cli/remote_workflow_test.go | 124 +++++++++++++++++++++++++++++ 3 files changed, 272 insertions(+), 9 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 1f2ae6dd8f..75dd15bd7c 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -457,6 +457,14 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch include dependencies: %v", err))) } } + // Also fetch and save frontmatter 'imports:' dependencies so they are available + // locally during compilation. Keeping these as relative paths (not workflowspecs) + // ensures the compiler resolves them from disk rather than downloading from GitHub. + if err := fetchAndSaveRemoteFrontmatterImports(string(sourceContent), workflowSpec, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil { + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch frontmatter import dependencies: %v", err))) + } + } } else if sourceInfo != nil && sourceInfo.IsLocal { // For local workflows, collect and copy include dependencies from local paths // The source directory is derived from the workflow's path @@ -501,15 +509,9 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o content = updatedContent } - // Process imports field and replace with workflowspec - processedImportsContent, err := processImportsWithWorkflowSpec(content, workflowSpec, commitSHA, opts.Verbose) - if err != nil { - if opts.Verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to process imports: %v", err))) - } - } else { - content = processedImportsContent - } + // Note: frontmatter 'imports:' are intentionally kept as relative paths here. + // 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 diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go index 061b594891..3b7ec01c15 100644 --- a/pkg/cli/remote_workflow.go +++ b/pkg/cli/remote_workflow.go @@ -243,6 +243,143 @@ func FetchIncludeFromSource(includePath string, baseSpec *WorkflowSpec, verbose return nil, section, fmt.Errorf("cannot resolve include path: %s (no base spec provided)", includePath) } +// fetchAndSaveRemoteFrontmatterImports fetches and saves files referenced in the frontmatter +// 'imports:' field of a remote workflow. These relative-path imports are resolved against +// the workflow's location in the source repository and saved locally so compilation can find them. +// This is analogous to fetchAndSaveRemoteIncludes, which handles @include directives in the +// markdown body; this function handles the YAML frontmatter 'imports:' field. +func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { + if spec.RepoSlug == "" { + return nil + } + + result, err := parser.ExtractFrontmatterFromContent(content) + if err != nil || result.Frontmatter == nil { + return nil + } + + importsField, exists := result.Frontmatter["imports"] + if !exists { + return nil + } + + var importPaths []string + switch v := importsField.(type) { + case []any: + for _, item := range v { + if s, ok := item.(string); ok { + importPaths = append(importPaths, s) + } + } + case []string: + importPaths = v + } + + if len(importPaths) == 0 { + return nil + } + + parts := strings.SplitN(spec.RepoSlug, "/", 2) + if len(parts) != 2 { + return nil + } + owner, repo := parts[0], parts[1] + ref := spec.Version + if ref == "" { + ref = "main" + } + + // Base directory of the workflow in the remote repo (e.g. ".github/workflows") + workflowBaseDir := getParentDir(spec.WorkflowPath) + + seen := make(map[string]bool) + + for _, importPath := range importPaths { + // Skip workflowspec-format imports (they already have an @-pinned ref) + if isWorkflowSpecFormat(importPath) { + continue + } + + // Strip any section reference (file.md#Section) + filePath := importPath + if before, _, hasSec := strings.Cut(importPath, "#"); hasSec { + filePath = before + } + if filePath == "" || seen[filePath] { + continue + } + seen[filePath] = true + + // Resolve the remote file path relative to the workflow's directory + var remoteFilePath string + if rest, ok := strings.CutPrefix(filePath, "/"); ok { + remoteFilePath = rest + } else if workflowBaseDir != "" { + remoteFilePath = workflowBaseDir + "/" + filePath + } else { + remoteFilePath = filePath + } + + // Download from the source repository + importContent, err := parser.DownloadFileFromGitHub(owner, repo, remoteFilePath, ref) + if err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch import %s: %v", filePath, err))) + } + continue + } + + // Target path in the user's repo: resolve relative to targetDir (the workflows directory) + targetPath := filepath.Join(targetDir, filepath.FromSlash(filePath)) + + // Create the target directory if needed + if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", filePath, err))) + } + continue + } + + // Check whether the file already exists + fileExists := false + if _, err := os.Stat(targetPath); err == nil { + fileExists = true + if !force { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Import file already exists, skipping: "+targetPath)) + } + continue + } + } + + // Write the file + if err := os.WriteFile(targetPath, importContent, 0600); err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to write import %s: %v", filePath, err))) + } + continue + } + + if verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Fetched import: "+targetPath)) + } + + // Track the file for git staging and potential rollback + if tracker != nil { + if fileExists { + tracker.TrackModified(targetPath) + } else { + tracker.TrackCreated(targetPath) + } + } + + // Recursively fetch any nested imports declared in this file + _ = fetchAndSaveRemoteFrontmatterImports(string(importContent), spec, targetDir, verbose, force, tracker) + } + + return nil +} + // fetchAndSaveRemoteIncludes parses the workflow content for @include directives and fetches them from the remote source func fetchAndSaveRemoteIncludes(content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { remoteWorkflowLog.Printf("Fetching remote includes for workflow: %s", spec.String()) diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 61eea95a3a..db8912e6b7 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -288,3 +288,127 @@ func TestGetParentDir(t *testing.T) { }) } } + +// TestFetchAndSaveRemoteFrontmatterImports_NoImports verifies that the function +// is a no-op when the workflow has no imports field. +func TestFetchAndSaveRemoteFrontmatterImports_NoImports(t *testing.T) { + content := `--- +engine: copilot +--- + +# Workflow with no imports +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "main", + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + tmpDir := t.TempDir() + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "should not error when no imports are present") + + // No files should have been created + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "no files should be created when no imports are present") +} + +// TestFetchAndSaveRemoteFrontmatterImports_EmptyRepoSlug verifies that the function +// is a no-op when the spec has no remote repo (local workflow). +func TestFetchAndSaveRemoteFrontmatterImports_EmptyRepoSlug(t *testing.T) { + content := `--- +engine: copilot +imports: + - shared/ci-data-analysis.md +--- + +# Workflow +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "", // local workflow – no remote repo + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + tmpDir := t.TempDir() + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "should not error for local workflow with empty RepoSlug") + + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "no files should be created for local workflows") +} + +// TestFetchAndSaveRemoteFrontmatterImports_WorkflowSpecSkipped verifies that imports +// that are already in workflowspec format (owner/repo/path@ref) are skipped. +func TestFetchAndSaveRemoteFrontmatterImports_WorkflowSpecSkipped(t *testing.T) { + content := `--- +engine: copilot +imports: + - github/gh-aw/.github/workflows/shared/ci-data-analysis.md@abc123 +--- + +# Workflow +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "main", + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + tmpDir := t.TempDir() + // This should not attempt any network calls; already-pinned imports are skipped. + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "should not error for workflowspec imports") + + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "already-pinned workflowspec imports should not be downloaded") +} + +// TestFetchAndSaveRemoteFrontmatterImports_FileTracking verifies that files saved by +// the function are properly tracked (created vs modified). +func TestFetchAndSaveRemoteFrontmatterImports_FileTracking(t *testing.T) { + // Use a git repo temp dir so the FileTracker can initialise + tmpDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".git"), 0755)) + + sharedDir := filepath.Join(tmpDir, "shared") + require.NoError(t, os.MkdirAll(sharedDir, 0755)) + + // Pre-create one of the two files so we can verify the modified-vs-created distinction + existingFile := filepath.Join(sharedDir, "existing.md") + require.NoError(t, os.WriteFile(existingFile, []byte("old content"), 0600)) + + tracker, err := NewFileTracker() + if err != nil { + t.Skip("skipping: not in a git repository", err) + } + + // We cannot invoke real GitHub downloads in unit tests, so instead we call + // the function with an empty imports list and verify no files were changed. + content := `--- +engine: copilot +--- + +# No imports +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "v1.0.0", + }, + WorkflowPath: ".github/workflows/test.md", + } + + err = fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) + require.NoError(t, err) + assert.Empty(t, tracker.CreatedFiles, "no files should be created") + assert.Empty(t, tracker.ModifiedFiles, "no files should be modified") +} From edfaaea766da89be4843d7cd5215b72d4cc0f5c4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 18:23:18 +0000 Subject: [PATCH 3/6] fix: address code review - use path.Join for remote paths and log nested import errors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/remote_workflow.go | 11 ++++++++--- pkg/cli/remote_workflow_test.go | 23 +++++++++++------------ 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go index 3b7ec01c15..5665139ae5 100644 --- a/pkg/cli/remote_workflow.go +++ b/pkg/cli/remote_workflow.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "path" "path/filepath" "regexp" "strings" @@ -310,12 +311,14 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } seen[filePath] = true - // Resolve the remote file path relative to the workflow's directory + // Resolve the remote file path relative to the workflow's directory. + // Use path.Join (not filepath.Join) because this is a URL/API path that always + // uses forward slashes regardless of the OS. var remoteFilePath string if rest, ok := strings.CutPrefix(filePath, "/"); ok { remoteFilePath = rest } else if workflowBaseDir != "" { - remoteFilePath = workflowBaseDir + "/" + filePath + remoteFilePath = path.Join(workflowBaseDir, filePath) } else { remoteFilePath = filePath } @@ -374,7 +377,9 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } // Recursively fetch any nested imports declared in this file - _ = fetchAndSaveRemoteFrontmatterImports(string(importContent), spec, targetDir, verbose, force, tracker) + if err := fetchAndSaveRemoteFrontmatterImports(string(importContent), spec, targetDir, verbose, force, tracker); err != nil && verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch nested imports from %s: %v", filePath, err))) + } } return nil diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index db8912e6b7..ba4e54b2aa 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -375,24 +375,23 @@ imports: // TestFetchAndSaveRemoteFrontmatterImports_FileTracking verifies that files saved by // the function are properly tracked (created vs modified). func TestFetchAndSaveRemoteFrontmatterImports_FileTracking(t *testing.T) { - // Use a git repo temp dir so the FileTracker can initialise + // Use a temp dir that acts as the workflows directory. tmpDir := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".git"), 0755)) + // Pre-create one file so we can test the "modified" branch. sharedDir := filepath.Join(tmpDir, "shared") require.NoError(t, os.MkdirAll(sharedDir, 0755)) - - // Pre-create one of the two files so we can verify the modified-vs-created distinction existingFile := filepath.Join(sharedDir, "existing.md") require.NoError(t, os.WriteFile(existingFile, []byte("old content"), 0600)) - tracker, err := NewFileTracker() - if err != nil { - t.Skip("skipping: not in a git repository", err) + // Build a minimal FileTracker without calling NewFileTracker (which requires a real + // git repository). We only need the tracking lists populated. + tracker := &FileTracker{ + OriginalContent: make(map[string][]byte), + gitRoot: tmpDir, } - // We cannot invoke real GitHub downloads in unit tests, so instead we call - // the function with an empty imports list and verify no files were changed. + // A workflow with no imports should leave the tracker empty. content := `--- engine: copilot --- @@ -407,8 +406,8 @@ engine: copilot WorkflowPath: ".github/workflows/test.md", } - err = fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) require.NoError(t, err) - assert.Empty(t, tracker.CreatedFiles, "no files should be created") - assert.Empty(t, tracker.ModifiedFiles, "no files should be modified") + assert.Empty(t, tracker.CreatedFiles, "no files should be created when there are no imports") + assert.Empty(t, tracker.ModifiedFiles, "no files should be modified when there are no imports") } From d561689d5afb39efd3a98fb5a33f541491e64e5b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 18:36:34 +0000 Subject: [PATCH 4/6] fix: use getRepoDefaultBranch helper and add more unit tests for fetchAndSaveRemoteFrontmatterImports Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/remote_workflow.go | 9 ++- pkg/cli/remote_workflow_test.go | 104 ++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go index 5665139ae5..779e0af1ac 100644 --- a/pkg/cli/remote_workflow.go +++ b/pkg/cli/remote_workflow.go @@ -287,7 +287,14 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta owner, repo := parts[0], parts[1] ref := spec.Version if ref == "" { - ref = "main" + // Resolve the actual default branch of the source repo rather than assuming "main" + defaultBranch, err := getRepoDefaultBranch(spec.RepoSlug) + if err != nil { + remoteWorkflowLog.Printf("Failed to resolve default branch for %s, falling back to 'main': %v", spec.RepoSlug, err) + ref = "main" + } else { + ref = defaultBranch + } } // Base directory of the workflow in the remote repo (e.g. ".github/workflows") diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index ba4e54b2aa..c0533632e1 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -411,3 +411,107 @@ engine: copilot assert.Empty(t, tracker.CreatedFiles, "no files should be created when there are no imports") assert.Empty(t, tracker.ModifiedFiles, "no files should be modified when there are no imports") } + +// TestFetchAndSaveRemoteFrontmatterImports_SectionSkipped verifies that imports +// with a section reference (file.md#Section) have the section stripped but the +// file path is still used for deduplication. +func TestFetchAndSaveRemoteFrontmatterImports_SectionSkipped(t *testing.T) { + // Content with two imports that differ only in their #section fragment. + // Both should resolve to the same file (deduplication by file path). + content := `--- +engine: copilot +imports: + - github/gh-aw/.github/workflows/shared/reporting.md@abc123 + - github/gh-aw/.github/workflows/shared/reporting.md@abc123 +--- + +# Workflow +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "v1.0.0", + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + tmpDir := t.TempDir() + // Both entries are already workflowspec format – neither should trigger a download. + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "duplicate workflowspec imports should not error") + + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "workflowspec imports must not be downloaded") +} + +// TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce verifies that +// an import that is already present on disk is skipped when force=false. +func TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce(t *testing.T) { + // Pre-create the shared file that the workflow would import. + tmpDir := t.TempDir() + sharedDir := filepath.Join(tmpDir, "shared") + require.NoError(t, os.MkdirAll(sharedDir, 0755)) + existingContent := []byte("existing content") + existingFile := filepath.Join(sharedDir, "ci-data-analysis.md") + require.NoError(t, os.WriteFile(existingFile, existingContent, 0600)) + + tracker := &FileTracker{ + OriginalContent: make(map[string][]byte), + gitRoot: tmpDir, + } + + // Use a content with a relative import that would resolve to the pre-created file. + // But since the import path is workflowspec-format, it's skipped regardless – + // this test verifies no modification is done to the pre-created file. + content := `--- +engine: copilot +imports: + - github/gh-aw/.github/workflows/shared/ci-data-analysis.md@abc123 +--- +# Workflow +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "v1.0.0", + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) + require.NoError(t, err) + + // The existing file should not have been touched. + gotContent, readErr := os.ReadFile(existingFile) + require.NoError(t, readErr) + assert.Equal(t, existingContent, gotContent, "pre-existing file must not be modified when force=false") + assert.Empty(t, tracker.CreatedFiles) + assert.Empty(t, tracker.ModifiedFiles) +} + +// TestFetchAndSaveRemoteFrontmatterImports_InvalidRepoSlug verifies that an invalid +// RepoSlug (not in owner/repo format) causes the function to return early without error. +func TestFetchAndSaveRemoteFrontmatterImports_InvalidRepoSlug(t *testing.T) { + content := `--- +engine: copilot +imports: + - shared/ci-data-analysis.md +--- +# Workflow +` + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "not-a-valid-slug", // missing slash → only one part + }, + WorkflowPath: ".github/workflows/ci-coach.md", + } + + tmpDir := t.TempDir() + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "invalid RepoSlug should return nil without error") + + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "no files should be created for an invalid RepoSlug") +} From fd8956284e7557ded98b0a1a66d7141ec4593758 Mon Sep 17 00:00:00 2001 From: Peli de Halleux Date: Sun, 22 Feb 2026 10:50:25 -0800 Subject: [PATCH 5/6] Update remote_workflow.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/cli/remote_workflow.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go index 779e0af1ac..df13e2b3c6 100644 --- a/pkg/cli/remote_workflow.go +++ b/pkg/cli/remote_workflow.go @@ -295,6 +295,8 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } else { ref = defaultBranch } + // Persist the resolved default ref so other callers do not need to re-resolve it + spec.Version = ref } // Base directory of the workflow in the remote repo (e.g. ".github/workflows") From e0308c9ca96c5ba8824aef32bc7edad79b54c9a7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Feb 2026 19:07:02 +0000 Subject: [PATCH 6/6] fix: add recursive import guardrails, path traversal protection, and improve tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/remote_workflow.go | 179 ++++++++++++++++++++++---------- pkg/cli/remote_workflow_test.go | 114 +++++++++++++------- 2 files changed, 204 insertions(+), 89 deletions(-) diff --git a/pkg/cli/remote_workflow.go b/pkg/cli/remote_workflow.go index df13e2b3c6..1ac587a881 100644 --- a/pkg/cli/remote_workflow.go +++ b/pkg/cli/remote_workflow.go @@ -249,19 +249,64 @@ func FetchIncludeFromSource(includePath string, baseSpec *WorkflowSpec, verbose // the workflow's location in the source repository and saved locally so compilation can find them. // This is analogous to fetchAndSaveRemoteIncludes, which handles @include directives in the // markdown body; this function handles the YAML frontmatter 'imports:' field. +// Import failures are non-fatal (best-effort); the compiler will report any still-missing files. func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { if spec.RepoSlug == "" { return nil } + parts := strings.SplitN(spec.RepoSlug, "/", 2) + if len(parts) != 2 { + return nil + } + owner, repo := parts[0], parts[1] + ref := spec.Version + if ref == "" { + // Resolve the actual default branch of the source repo rather than assuming "main" + defaultBranch, err := getRepoDefaultBranch(spec.RepoSlug) + if err != nil { + remoteWorkflowLog.Printf("Failed to resolve default branch for %s, falling back to 'main': %v", spec.RepoSlug, err) + ref = "main" + } else { + ref = defaultBranch + } + // Persist the resolved default ref so other callers do not need to re-resolve it + spec.Version = ref + } + + // workflowBaseDir is the directory of the top-level workflow in the source repo + // (e.g. ".github/workflows"). It serves as both the starting point for resolving + // relative imports and as the prefix to strip when computing local target paths. + workflowBaseDir := getParentDir(spec.WorkflowPath) + + // seen is keyed by fully-resolved remote file path. It is shared across all recursion + // levels so that every import (at any depth) is downloaded at most once and import + // cycles (A imports B, B imports A) are broken without infinite recursion. + seen := make(map[string]bool) + fetchFrontmatterImportsRecursive(content, owner, repo, ref, workflowBaseDir, workflowBaseDir, targetDir, verbose, force, tracker, seen) + return nil +} + +// fetchFrontmatterImportsRecursive is the internal worker for fetchAndSaveRemoteFrontmatterImports. +// +// Parameters that change per recursion level: +// - content: the text of the file whose imports are being processed +// - currentBaseDir: directory of that file inside the source repo (used to resolve relative paths) +// +// Parameters that remain constant across all recursion levels: +// - owner, repo, ref: source repository coordinates +// - originalBaseDir: directory of the top-level workflow (used to map remote paths → local paths) +// - targetDir: the `.github/workflows` directory in the user's repo +// - seen: shared visited set (keyed by fully-resolved remote path) — prevents cycles & duplicates +func fetchFrontmatterImportsRecursive(content, owner, repo, ref, currentBaseDir, originalBaseDir, targetDir string, verbose, force bool, tracker *FileTracker, seen map[string]bool) { result, err := parser.ExtractFrontmatterFromContent(content) if err != nil || result.Frontmatter == nil { - return nil + return } importsField, exists := result.Frontmatter["imports"] if !exists { - return nil + return } var importPaths []string @@ -277,84 +322,99 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } if len(importPaths) == 0 { - return nil + return } - parts := strings.SplitN(spec.RepoSlug, "/", 2) - if len(parts) != 2 { - return nil - } - owner, repo := parts[0], parts[1] - ref := spec.Version - if ref == "" { - // Resolve the actual default branch of the source repo rather than assuming "main" - defaultBranch, err := getRepoDefaultBranch(spec.RepoSlug) - if err != nil { - remoteWorkflowLog.Printf("Failed to resolve default branch for %s, falling back to 'main': %v", spec.RepoSlug, err) - ref = "main" - } else { - ref = defaultBranch - } - // Persist the resolved default ref so other callers do not need to re-resolve it - spec.Version = ref + // Pre-compute the absolute target directory once for path-traversal boundary checks. + absTargetDir, err := filepath.Abs(targetDir) + if err != nil { + return } - // Base directory of the workflow in the remote repo (e.g. ".github/workflows") - workflowBaseDir := getParentDir(spec.WorkflowPath) - - seen := make(map[string]bool) - for _, importPath := range importPaths { - // Skip workflowspec-format imports (they already have an @-pinned ref) + // Skip workflowspec-format imports (already pinned to a remote ref) if isWorkflowSpecFormat(importPath) { continue } - // Strip any section reference (file.md#Section) + // Strip any section reference (file.md#Section → file.md) filePath := importPath if before, _, hasSec := strings.Cut(importPath, "#"); hasSec { filePath = before } - if filePath == "" || seen[filePath] { + if filePath == "" { continue } - seen[filePath] = true - // Resolve the remote file path relative to the workflow's directory. - // Use path.Join (not filepath.Join) because this is a URL/API path that always - // uses forward slashes regardless of the OS. + // Resolve the remote file path relative to the current file's directory. + // Use path (not filepath) because this is always a forward-slash URL/API path. var remoteFilePath string if rest, ok := strings.CutPrefix(filePath, "/"); ok { + // Absolute path from repo root (e.g. "/scripts/helper.md") remoteFilePath = rest - } else if workflowBaseDir != "" { - remoteFilePath = path.Join(workflowBaseDir, filePath) + } else if currentBaseDir != "" { + remoteFilePath = path.Join(currentBaseDir, filePath) } else { remoteFilePath = filePath } + remoteFilePath = path.Clean(remoteFilePath) - // Download from the source repository - importContent, err := parser.DownloadFileFromGitHub(owner, repo, remoteFilePath, ref) - if err != nil { + // Reject paths that try to escape the repository root (e.g. "../../etc/passwd") + if remoteFilePath == ".." || strings.HasPrefix(remoteFilePath, "../") { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch import %s: %v", filePath, err))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Skipping import with unsafe path: %q", importPath))) } continue } - // Target path in the user's repo: resolve relative to targetDir (the workflows directory) - targetPath := filepath.Join(targetDir, filepath.FromSlash(filePath)) + // Cycle/duplicate prevention: use the fully-resolved remote path as the key. + if seen[remoteFilePath] { + continue + } + seen[remoteFilePath] = true + + // Derive the local path relative to targetDir by stripping the original base-dir + // prefix from the remote path. This ensures that imports in nested files resolve + // to the correct location regardless of how many levels deep the recursion goes. + // + // Example: originalBaseDir=".github/workflows" + // remoteFilePath=".github/workflows/shared/analysis.md" → localRelPath="shared/analysis.md" + // (nested) remoteFilePath=".github/workflows/other.md" → localRelPath="other.md" + var localRelPath string + if originalBaseDir != "" && strings.HasPrefix(remoteFilePath, originalBaseDir+"/") { + localRelPath = remoteFilePath[len(originalBaseDir)+1:] + } else { + // Workflow at repo root, or import outside the original base dir: + // use the full remote path relative to targetDir. + localRelPath = remoteFilePath + } + localRelPath = filepath.Clean(filepath.FromSlash(localRelPath)) + // Strip any leading separator produced by Clean on root-relative paths. + localRelPath = strings.TrimLeft(localRelPath, string(filepath.Separator)) + // Reject empty or "." paths (would point to targetDir itself) as a safety guard. + // ".." cannot appear here because remoteFilePath was already rejected above if it + // started with "..", and path.Clean cannot introduce new ".." components. + if localRelPath == "" || localRelPath == "." { + continue + } + targetPath := filepath.Join(targetDir, localRelPath) - // Create the target directory if needed - if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { + // Belt-and-suspenders: verify the resolved path is inside targetDir + absTargetPath, absErr := filepath.Abs(targetPath) + if absErr != nil { + continue + } + if rel, relErr := filepath.Rel(absTargetDir, absTargetPath); relErr != nil || strings.HasPrefix(rel, "..") { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", filePath, err))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Refusing to write import outside target directory: %q", importPath))) } continue } - // Check whether the file already exists + // Check existence before downloading: if the file already exists and force=false, + // skip the download entirely (no unnecessary network round-trip). fileExists := false - if _, err := os.Stat(targetPath); err == nil { + if _, statErr := os.Stat(targetPath); statErr == nil { fileExists = true if !force { if verbose { @@ -364,10 +424,27 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } } + // Download from the source repository + importContent, err := parser.DownloadFileFromGitHub(owner, repo, remoteFilePath, ref) + if err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch import %s: %v", remoteFilePath, err))) + } + continue + } + + // Create the parent directory if needed + if err := os.MkdirAll(filepath.Dir(targetPath), 0755); err != nil { + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to create directory for import %s: %v", remoteFilePath, err))) + } + continue + } + // Write the file if err := os.WriteFile(targetPath, importContent, 0600); err != nil { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to write import %s: %v", filePath, err))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to write import %s: %v", remoteFilePath, err))) } continue } @@ -385,13 +462,11 @@ func fetchAndSaveRemoteFrontmatterImports(content string, spec *WorkflowSpec, ta } } - // Recursively fetch any nested imports declared in this file - if err := fetchAndSaveRemoteFrontmatterImports(string(importContent), spec, targetDir, verbose, force, tracker); err != nil && verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch nested imports from %s: %v", filePath, err))) - } + // Recurse into the imported file's imports. Use the imported file's directory as + // currentBaseDir so that relative paths inside it resolve correctly. + importedBaseDir := path.Dir(remoteFilePath) + fetchFrontmatterImportsRecursive(string(importContent), owner, repo, ref, importedBaseDir, originalBaseDir, targetDir, verbose, force, tracker, seen) } - - return nil } // fetchAndSaveRemoteIncludes parses the workflow content for @include directives and fetches them from the remote source diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index c0533632e1..f03dd36efb 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -3,6 +3,7 @@ package cli import ( + "fmt" "os" "path/filepath" "testing" @@ -372,26 +373,16 @@ imports: assert.Empty(t, entries, "already-pinned workflowspec imports should not be downloaded") } -// TestFetchAndSaveRemoteFrontmatterImports_FileTracking verifies that files saved by -// the function are properly tracked (created vs modified). -func TestFetchAndSaveRemoteFrontmatterImports_FileTracking(t *testing.T) { - // Use a temp dir that acts as the workflows directory. - tmpDir := t.TempDir() - - // Pre-create one file so we can test the "modified" branch. - sharedDir := filepath.Join(tmpDir, "shared") - require.NoError(t, os.MkdirAll(sharedDir, 0755)) - existingFile := filepath.Join(sharedDir, "existing.md") - require.NoError(t, os.WriteFile(existingFile, []byte("old content"), 0600)) - +// TestFetchAndSaveRemoteFrontmatterImports_NoImportsNoOpTracker verifies that a workflow with +// no imports field leaves the FileTracker completely untouched. +func TestFetchAndSaveRemoteFrontmatterImports_NoImportsNoOpTracker(t *testing.T) { // Build a minimal FileTracker without calling NewFileTracker (which requires a real - // git repository). We only need the tracking lists populated. + // git repository). We only need the tracking lists populated. tracker := &FileTracker{ OriginalContent: make(map[string][]byte), - gitRoot: tmpDir, + gitRoot: t.TempDir(), } - // A workflow with no imports should leave the tracker empty. content := `--- engine: copilot --- @@ -406,23 +397,25 @@ engine: copilot WorkflowPath: ".github/workflows/test.md", } - err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tracker.gitRoot, false, false, tracker) require.NoError(t, err) assert.Empty(t, tracker.CreatedFiles, "no files should be created when there are no imports") assert.Empty(t, tracker.ModifiedFiles, "no files should be modified when there are no imports") } -// TestFetchAndSaveRemoteFrontmatterImports_SectionSkipped verifies that imports -// with a section reference (file.md#Section) have the section stripped but the -// file path is still used for deduplication. -func TestFetchAndSaveRemoteFrontmatterImports_SectionSkipped(t *testing.T) { - // Content with two imports that differ only in their #section fragment. - // Both should resolve to the same file (deduplication by file path). +// TestFetchAndSaveRemoteFrontmatterImports_SectionStrippedDedup verifies that two imports +// pointing to the same file via different #section fragments are treated as one file +// (deduplication via the shared seen set). +func TestFetchAndSaveRemoteFrontmatterImports_SectionStrippedDedup(t *testing.T) { + // Both imports resolve to the same base file after stripping the #section fragment. + // The first triggers a (failing) download attempt; the second is deduplicated and never + // even reaches the download step. Both use relative paths so the workflowspec-format + // skip path is not taken. content := `--- engine: copilot imports: - - github/gh-aw/.github/workflows/shared/reporting.md@abc123 - - github/gh-aw/.github/workflows/shared/reporting.md@abc123 + - shared/reporting.md#SectionA + - shared/reporting.md#SectionB --- # Workflow @@ -436,19 +429,20 @@ imports: } tmpDir := t.TempDir() - // Both entries are already workflowspec format – neither should trigger a download. + // No network in unit tests: the download attempt for the first import will fail silently + // (verbose=false). The second import must be deduplicated without a second download. err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) - require.NoError(t, err, "duplicate workflowspec imports should not error") + require.NoError(t, err, "section-fragment deduplication should not error") entries, readErr := os.ReadDir(tmpDir) require.NoError(t, readErr) - assert.Empty(t, entries, "workflowspec imports must not be downloaded") + assert.Empty(t, entries, "no files should be written (download fails in unit tests)") } -// TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce verifies that -// an import that is already present on disk is skipped when force=false. +// TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce verifies that a relative +// import whose target file already exists on disk is skipped (not re-downloaded) when force=false. +// Because the existence check happens before the download, this test requires no network access. func TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce(t *testing.T) { - // Pre-create the shared file that the workflow would import. tmpDir := t.TempDir() sharedDir := filepath.Join(tmpDir, "shared") require.NoError(t, os.MkdirAll(sharedDir, 0755)) @@ -461,13 +455,13 @@ func TestFetchAndSaveRemoteFrontmatterImports_SkipExistingWithoutForce(t *testin gitRoot: tmpDir, } - // Use a content with a relative import that would resolve to the pre-created file. - // But since the import path is workflowspec-format, it's skipped regardless – - // this test verifies no modification is done to the pre-created file. + // Relative import: resolves to tmpDir/shared/ci-data-analysis.md which already exists. + // With force=false, the function detects the file via os.Stat *before* attempting a + // download, so no network call is made and the file is preserved unchanged. content := `--- engine: copilot imports: - - github/gh-aw/.github/workflows/shared/ci-data-analysis.md@abc123 + - shared/ci-data-analysis.md --- # Workflow ` @@ -482,12 +476,58 @@ imports: err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, tracker) require.NoError(t, err) - // The existing file should not have been touched. + // The existing file must be untouched and not added to the tracker. gotContent, readErr := os.ReadFile(existingFile) require.NoError(t, readErr) assert.Equal(t, existingContent, gotContent, "pre-existing file must not be modified when force=false") - assert.Empty(t, tracker.CreatedFiles) - assert.Empty(t, tracker.ModifiedFiles) + assert.Empty(t, tracker.CreatedFiles, "pre-existing file must not appear in CreatedFiles") + assert.Empty(t, tracker.ModifiedFiles, "pre-existing file must not appear in ModifiedFiles") +} + +// TestFetchAndSaveRemoteFrontmatterImports_PathTraversal verifies that import paths that +// attempt to escape the repository root via ".." sequences are rejected by the +// remoteFilePath safety check (not just because of a download failure). +// The workflow is placed at the repo root (WorkflowPath="ci-coach.md") so that +// workflowBaseDir="" and path.Join("", "../etc/passwd") = "../etc/passwd", which +// triggers the explicit ".." rejection before any network call. +func TestFetchAndSaveRemoteFrontmatterImports_PathTraversal(t *testing.T) { + tests := []struct { + name string + importPath string + }{ + {name: "parent directory traversal", importPath: "../etc/passwd"}, + {name: "deep traversal", importPath: "../../tmp/evil.md"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + content := fmt.Sprintf(`--- +engine: copilot +imports: + - %s +--- +# Workflow +`, tc.importPath) + // WorkflowPath at repo root → workflowBaseDir="" → path.Join("","../etc/passwd")="../etc/passwd" + // which triggers the explicit ".." rejection before any network call. + spec := &WorkflowSpec{ + RepoSpec: RepoSpec{ + RepoSlug: "github/gh-aw", + Version: "v1.0.0", + }, + WorkflowPath: "ci-coach.md", + } + + tmpDir := t.TempDir() + err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) + require.NoError(t, err, "path traversal should be silently rejected, not return an error") + + // No file must have been written anywhere + entries, readErr := os.ReadDir(tmpDir) + require.NoError(t, readErr) + assert.Empty(t, entries, "traversal import %q must not write any file", tc.importPath) + }) + } } // TestFetchAndSaveRemoteFrontmatterImports_InvalidRepoSlug verifies that an invalid