From fc6527eb98e88b3c06cd718fca4698d0ed9d5048 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 12:50:37 +0000 Subject: [PATCH 1/3] Initial plan From 5723981615795cf96e1965e41ca2dee9f1271dd7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 12:58:50 +0000 Subject: [PATCH 2/3] Add BasePath field to remoteImportOrigin for correct nested import resolution Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/import_processor.go | 56 ++++++--- pkg/parser/import_remote_nested_test.go | 158 ++++++++++++++++++------ 2 files changed, 161 insertions(+), 53 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 6bb7256996..dd2fc1829a 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -89,9 +89,10 @@ func ProcessImportsFromFrontmatter(frontmatter map[string]any, baseDir string) ( // When a file is fetched from a remote GitHub repository via workflowspec, // its nested relative imports must be resolved against the same remote repo. type remoteImportOrigin struct { - Owner string // Repository owner (e.g., "elastic") - Repo string // Repository name (e.g., "ai-github-actions") - Ref string // Git ref - branch, tag, or SHA (e.g., "main", "v1.0.0", "abc123...") + Owner string // Repository owner (e.g., "elastic") + Repo string // Repository name (e.g., "ai-github-actions") + Ref string // Git ref - branch, tag, or SHA (e.g., "main", "v1.0.0", "abc123...") + BasePath string // Base directory path within the repo (e.g., "gh-agent-workflows" for gh-agent-workflows/gh-aw-workflows/file.md) } // importQueueItem represents a file to be imported with its context @@ -104,9 +105,12 @@ type importQueueItem struct { remoteOrigin *remoteImportOrigin // Remote origin context (non-nil when imported from a remote repo) } -// parseRemoteOrigin extracts the remote origin (owner, repo, ref) from a workflowspec path. +// parseRemoteOrigin extracts the remote origin (owner, repo, ref, basePath) from a workflowspec path. // Returns nil if the path is not a valid workflowspec. // Format: owner/repo/path[@ref] where ref defaults to "main" if not specified. +// BasePath is derived from the parent workflowspec path and used for resolving nested relative imports. +// For example, "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/file.md@main" +// produces BasePath="gh-agent-workflows" so nested imports resolve relative to that directory. func parseRemoteOrigin(spec string) *remoteImportOrigin { // Remove section reference if present cleanSpec := spec @@ -128,10 +132,28 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { return nil } + // Derive BasePath: everything between owner/repo and the last two path components + // Since imports are always 2-level (dir/file.md), the base is everything before the last 2 parts + // Examples: + // - "owner/repo/.github/workflows/file.md" -> BasePath = ".github/workflows" + // - "owner/repo/gh-agent-workflows/gh-aw-workflows/file.md" -> BasePath = "gh-agent-workflows" + // - "owner/repo/a/b/c/d/file.md" -> BasePath = "a/b/c" + var basePath string + repoRelativeParts := slashParts[2:] // Everything after owner/repo + if len(repoRelativeParts) >= 2 { + // Take everything except the last component (the file itself) + // For nested imports, we want the directory containing the file + baseDirParts := repoRelativeParts[:len(repoRelativeParts)-1] + if len(baseDirParts) > 0 { + basePath = strings.Join(baseDirParts, "/") + } + } + return &remoteImportOrigin{ - Owner: slashParts[0], - Repo: slashParts[1], - Ref: ref, + Owner: slashParts[0], + Repo: slashParts[1], + Ref: ref, + BasePath: basePath, } } @@ -492,19 +514,25 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a if item.remoteOrigin != nil && !isWorkflowSpec(nestedFilePath) { // Parent was fetched from a remote repo and nested path is relative. - // Convert to a workflowspec that resolves against the remote repo's - // .github/workflows/ directory (mirrors local compilation behavior). + // Convert to a workflowspec that resolves against the parent workflowspec's + // base directory (e.g., gh-agent-workflows for gh-agent-workflows/gh-aw-workflows/file.md). cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) - // Reject paths that escape .github/workflows/ (e.g., ../../../etc/passwd) + // Reject paths that escape the base directory (e.g., ../../../etc/passwd) if cleanPath == ".." || strings.HasPrefix(cleanPath, "../") || path.IsAbs(cleanPath) { - return nil, fmt.Errorf("nested import '%s' from remote file '%s' escapes .github/workflows/ base directory", nestedFilePath, item.importPath) + return nil, fmt.Errorf("nested import '%s' from remote file '%s' escapes base directory", nestedFilePath, item.importPath) + } + + // Use the parent's BasePath if available, otherwise default to .github/workflows + basePath := item.remoteOrigin.BasePath + if basePath == "" { + basePath = ".github/workflows" } - resolvedPath = fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - item.remoteOrigin.Owner, item.remoteOrigin.Repo, cleanPath, item.remoteOrigin.Ref) + resolvedPath = fmt.Sprintf("%s/%s/%s/%s@%s", + item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePath, cleanPath, item.remoteOrigin.Ref) nestedRemoteOrigin = item.remoteOrigin - importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s", nestedFilePath, resolvedPath) + importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath=%s)", nestedFilePath, resolvedPath, basePath) } else if isWorkflowSpec(nestedFilePath) { // Nested import is itself a workflowspec - parse its remote origin nestedRemoteOrigin = parseRemoteOrigin(nestedFilePath) diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index 36d2dc3369..225d42d188 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -24,36 +24,70 @@ func TestParseRemoteOrigin(t *testing.T) { name: "basic workflowspec with ref", spec: "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "gh-agent-workflows/mention-in-pr", }, }, { name: "workflowspec with SHA ref", spec: "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@160c33700227b5472dc3a08aeea1e774389a1a84", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", + BasePath: "gh-agent-workflows/mention-in-pr", }, }, { name: "workflowspec without ref defaults to main", spec: "elastic/ai-github-actions/gh-agent-workflows/file.md", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "gh-agent-workflows", }, }, { name: "workflowspec with section reference", spec: "owner/repo/path/file.md@v1.0#SectionName", expected: &remoteImportOrigin{ - Owner: "owner", - Repo: "repo", - Ref: "v1.0", + Owner: "owner", + Repo: "repo", + Ref: "v1.0", + BasePath: "path", + }, + }, + { + name: "workflowspec under .github/workflows", + spec: "owner/repo/.github/workflows/test.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: ".github/workflows", + }, + }, + { + name: "workflowspec with deep path", + spec: "owner/repo/a/b/c/d/e/file.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "a/b/c/d/e", + }, + }, + { + name: "workflowspec directly in repo root (minimal path)", + spec: "owner/repo/file.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "", }, }, { @@ -78,6 +112,7 @@ func TestParseRemoteOrigin(t *testing.T) { assert.Equal(t, tt.expected.Owner, result.Owner, "Owner mismatch") assert.Equal(t, tt.expected.Repo, result.Repo, "Repo mismatch") assert.Equal(t, tt.expected.Ref, result.Ref, "Ref mismatch") + assert.Equal(t, tt.expected.BasePath, result.BasePath, "BasePath mismatch") } }) } @@ -126,6 +161,7 @@ func TestRemoteOriginPropagation(t *testing.T) { assert.Equal(t, "elastic", origin.Owner, "Owner should be elastic") assert.Equal(t, "ai-github-actions", origin.Repo, "Repo should be ai-github-actions") assert.Equal(t, "main", origin.Ref, "Ref should be main") + assert.Equal(t, "gh-agent-workflows/mention-in-pr", origin.BasePath, "BasePath should be gh-agent-workflows/mention-in-pr") }) t.Run("local import does not get remote origin", func(t *testing.T) { @@ -136,24 +172,55 @@ func TestRemoteOriginPropagation(t *testing.T) { assert.Nil(t, origin, "Local paths should not produce remote origin") }) - t.Run("nested relative path from remote parent produces correct workflowspec", func(t *testing.T) { + t.Run("nested relative path from remote parent with BasePath produces correct workflowspec", func(t *testing.T) { + origin := &remoteImportOrigin{ + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "gh-agent-workflows", + } + nestedPath := "gh-aw-fragments/elastic-tools.md" + + // This is the NEW logic from the import processor: + // When parent is remote and has a BasePath, use that BasePath instead of .github/workflows/ + basePath := origin.BasePath + if basePath == "" { + basePath = ".github/workflows" + } + expectedSpec := fmt.Sprintf("%s/%s/%s/%s@%s", + origin.Owner, origin.Repo, basePath, nestedPath, origin.Ref) + + assert.Equal(t, + "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/elastic-tools.md@main", + expectedSpec, + "Nested relative import should resolve to parent's BasePath", + ) + + // The constructed spec should be recognized as a workflowspec + assert.True(t, isWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec") + }) + + t.Run("nested relative path from remote parent without BasePath uses .github/workflows", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "", // Empty BasePath - should fall back to .github/workflows } nestedPath := "shared/elastic-tools.md" - // This is the logic from the import processor: - // When parent is remote and nested path is not a workflowspec, - // construct a workflowspec resolving against .github/workflows/ - expectedSpec := fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - origin.Owner, origin.Repo, nestedPath, origin.Ref) + // When BasePath is empty, fall back to .github/workflows/ + basePath := origin.BasePath + if basePath == "" { + basePath = ".github/workflows" + } + expectedSpec := fmt.Sprintf("%s/%s/%s/%s@%s", + origin.Owner, origin.Repo, basePath, nestedPath, origin.Ref) assert.Equal(t, "elastic/ai-github-actions/.github/workflows/shared/elastic-tools.md@main", expectedSpec, - "Nested relative import should resolve to remote .github/workflows/ path", + "Nested relative import with empty BasePath should fall back to .github/workflows/", ) // The constructed spec should be recognized as a workflowspec @@ -162,9 +229,10 @@ func TestRemoteOriginPropagation(t *testing.T) { t.Run("nested relative path with ./ prefix is cleaned", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "org", - Repo: "repo", - Ref: "v1.0", + Owner: "org", + Repo: "repo", + Ref: "v1.0", + BasePath: "custom-path", } nestedPath := "./shared/tools.md" @@ -174,11 +242,15 @@ func TestRemoteOriginPropagation(t *testing.T) { cleanPath = cleanPath[2:] } - expectedSpec := fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - origin.Owner, origin.Repo, cleanPath, origin.Ref) + basePath := origin.BasePath + if basePath == "" { + basePath = ".github/workflows" + } + expectedSpec := fmt.Sprintf("%s/%s/%s/%s@%s", + origin.Owner, origin.Repo, basePath, cleanPath, origin.Ref) assert.Equal(t, - "org/repo/.github/workflows/shared/tools.md@v1.0", + "org/repo/custom-path/shared/tools.md@v1.0", expectedSpec, "Dot-prefix should be stripped when constructing remote spec", ) @@ -195,6 +267,7 @@ func TestRemoteOriginPropagation(t *testing.T) { assert.Equal(t, "other-org", origin.Owner, "Should use nested spec's owner") assert.Equal(t, "other-repo", origin.Repo, "Should use nested spec's repo") assert.Equal(t, "v2.0", origin.Ref, "Should use nested spec's ref") + assert.Equal(t, "path", origin.BasePath, "Should use nested spec's base path") }) t.Run("path traversal in nested import is rejected", func(t *testing.T) { @@ -207,22 +280,27 @@ func TestRemoteOriginPropagation(t *testing.T) { "Cleaned path should start with .. and be rejected by the import processor") }) - t.Run("SHA ref is preserved in nested resolution", func(t *testing.T) { + t.Run("SHA ref is preserved in nested resolution with BasePath", func(t *testing.T) { sha := "160c33700227b5472dc3a08aeea1e774389a1a84" origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: sha, + Owner: "elastic", + Repo: "ai-github-actions", + Ref: sha, + BasePath: "gh-agent-workflows", } nestedPath := "shared/formatting.md" - resolvedSpec := fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - origin.Owner, origin.Repo, nestedPath, origin.Ref) + basePath := origin.BasePath + if basePath == "" { + basePath = ".github/workflows" + } + resolvedSpec := fmt.Sprintf("%s/%s/%s/%s@%s", + origin.Owner, origin.Repo, basePath, nestedPath, origin.Ref) assert.Equal(t, - "elastic/ai-github-actions/.github/workflows/shared/formatting.md@"+sha, + "elastic/ai-github-actions/gh-agent-workflows/shared/formatting.md@"+sha, resolvedSpec, - "SHA ref should be preserved for nested imports", + "SHA ref should be preserved for nested imports with BasePath", ) }) } @@ -243,9 +321,10 @@ func TestImportQueueItemRemoteOriginField(t *testing.T) { t.Run("queue item with remote origin", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "path", } item := importQueueItem{ importPath: "elastic/ai-github-actions/path/file.md@main", @@ -258,6 +337,7 @@ func TestImportQueueItemRemoteOriginField(t *testing.T) { assert.Equal(t, "elastic", item.remoteOrigin.Owner, "Owner should match") assert.Equal(t, "ai-github-actions", item.remoteOrigin.Repo, "Repo should match") assert.Equal(t, "main", item.remoteOrigin.Ref, "Ref should match") + assert.Equal(t, "path", item.remoteOrigin.BasePath, "BasePath should match") }) } From d1227cc2092965288de2271d5209448e2ded40c9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 13:13:17 +0000 Subject: [PATCH 3/3] Add path cleaning, enhanced logging, and comprehensive tests for BasePath - Clean BasePath using path.Clean() to normalize paths (remove ./ and redundant slashes) - Add detailed logging for BasePath derivation with spec, owner, repo, ref - Add logging when no BasePath can be derived (file at repo root) - Clean basePath before use in nested import resolution - Add tests for path cleaning (./ components, redundant slashes, complex paths) - Add tests for URL-like paths (document current behavior) - Add test for enterprise domain workflowspecs - Fix BasePath derivation to include full directory path (not just parent dir) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/import_processor.go | 17 ++-- pkg/parser/import_remote_nested_test.go | 112 ++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 5 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index dd2fc1829a..3b58041ff4 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -132,12 +132,12 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { return nil } - // Derive BasePath: everything between owner/repo and the last two path components - // Since imports are always 2-level (dir/file.md), the base is everything before the last 2 parts + // Derive BasePath: everything between owner/repo and the last component (filename) + // Since imports are always 2-level (dir/file.md), the base is everything before the filename // Examples: // - "owner/repo/.github/workflows/file.md" -> BasePath = ".github/workflows" - // - "owner/repo/gh-agent-workflows/gh-aw-workflows/file.md" -> BasePath = "gh-agent-workflows" - // - "owner/repo/a/b/c/d/file.md" -> BasePath = "a/b/c" + // - "owner/repo/gh-agent-workflows/gh-aw-workflows/file.md" -> BasePath = "gh-agent-workflows/gh-aw-workflows" + // - "owner/repo/a/b/c/d/file.md" -> BasePath = "a/b/c/d" var basePath string repoRelativeParts := slashParts[2:] // Everything after owner/repo if len(repoRelativeParts) >= 2 { @@ -145,8 +145,13 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { // For nested imports, we want the directory containing the file baseDirParts := repoRelativeParts[:len(repoRelativeParts)-1] if len(baseDirParts) > 0 { - basePath = strings.Join(baseDirParts, "/") + // Clean the path to normalize it (remove ./ and resolve ..) + basePath = path.Clean(strings.Join(baseDirParts, "/")) + importLog.Printf("Derived BasePath=%q from spec=%q (owner=%s, repo=%s, ref=%s)", + basePath, spec, slashParts[0], slashParts[1], ref) } + } else { + importLog.Printf("No BasePath derived from spec=%q (file at repo root)", spec) } return &remoteImportOrigin{ @@ -528,6 +533,8 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a if basePath == "" { basePath = ".github/workflows" } + // Clean the basePath to ensure it's normalized + basePath = path.Clean(basePath) resolvedPath = fmt.Sprintf("%s/%s/%s/%s@%s", item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePath, cleanPath, item.remoteOrigin.Ref) diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index 225d42d188..195502cfe6 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -90,6 +90,26 @@ func TestParseRemoteOrigin(t *testing.T) { BasePath: "", }, }, + { + name: "path with ./ should be cleaned", + spec: "owner/repo/./path/./file.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "path", + }, + }, + { + name: "path with redundant slashes should be cleaned", + spec: "owner/repo/path//to///file.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "path/to", + }, + }, { name: "too few parts returns nil", spec: "owner/repo", @@ -451,3 +471,95 @@ func TestResolveRemoteSymlinksPathConstruction(t *testing.T) { "Nested symlink with ../ target should resolve correctly") }) } + +func TestParseRemoteOriginWithCleanedPaths(t *testing.T) { + tests := []struct { + name string + spec string + expected *remoteImportOrigin + }{ + { + name: "path with ./ components should be cleaned", + spec: "owner/repo/./workflows/./test.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "workflows", + }, + }, + { + name: "path with redundant slashes should be cleaned", + spec: "owner/repo/workflows//subdir///test.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "workflows/subdir", + }, + }, + { + name: "complex path cleaning", + spec: "owner/repo/./a//b/./c///test.md@main", + expected: &remoteImportOrigin{ + Owner: "owner", + Repo: "repo", + Ref: "main", + BasePath: "a/b/c", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := parseRemoteOrigin(tt.spec) + require.NotNil(t, result, "Should parse remote origin for spec: %s", tt.spec) + assert.Equal(t, tt.expected.Owner, result.Owner, "Owner mismatch") + assert.Equal(t, tt.expected.Repo, result.Repo, "Repo mismatch") + assert.Equal(t, tt.expected.Ref, result.Ref, "Ref mismatch") + assert.Equal(t, tt.expected.BasePath, result.BasePath, "BasePath mismatch") + }) + } +} + +func TestParseRemoteOriginWithURLFormats(t *testing.T) { + t.Run("URL-like paths are currently accepted by isWorkflowSpec", func(t *testing.T) { + // URLs are currently accepted by isWorkflowSpec because they have >3 parts when split by / + // This documents the current behavior - URLs might need special handling in the future + urlPaths := []string{ + "https://github.com/owner/repo/path/file.md", + "http://github.com/owner/repo/path/file.md", + "https://github.enterprise.com/owner/repo/path/file.md", + } + + for _, urlPath := range urlPaths { + // Currently, isWorkflowSpec accepts URLs (they have >3 slash-separated parts) + isSpec := isWorkflowSpec(urlPath) + assert.True(t, isSpec, "URL is currently accepted as workflowspec: %s", urlPath) + + // parseRemoteOrigin will parse the URL parts literally + // For "https://github.com/owner/repo/path/file.md": + // - Parts: ["https:", "", "github.com", "owner", "repo", "path", "file.md"] + // - Owner would be "https:" (first part after splitting by /) + // This test documents the current behavior for future reference + origin := parseRemoteOrigin(urlPath) + if origin != nil { + t.Logf("URL %s parsed as: owner=%s, repo=%s, basePath=%s", + urlPath, origin.Owner, origin.Repo, origin.BasePath) + } + } + }) + + t.Run("enterprise domain workflowspec format", func(t *testing.T) { + // Enterprise GitHub uses the same owner/repo/path format + // The domain is handled by GH_HOST environment variable, not in the workflowspec + spec := "enterprise-org/enterprise-repo/workflows/test.md@main" + + result := parseRemoteOrigin(spec) + require.NotNil(t, result, "Should parse enterprise workflowspec") + assert.Equal(t, "enterprise-org", result.Owner) + assert.Equal(t, "enterprise-repo", result.Repo) + assert.Equal(t, "main", result.Ref) + assert.Equal(t, "workflows", result.BasePath) + }) +}