From 7af89b721ac742e49edf46528009228d5e8fba7d Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 08:40:15 -0600 Subject: [PATCH 1/6] Fix nested remote imports to resolve relative to parent's base path Nested relative imports from remote workflowspecs were hardcoded to resolve against .github/workflows/ in the remote repo. When the parent workflowspec was imported through a different path (e.g., gh-agent-workflows/), this produced inconsistent cache paths in the lock file and caused security scanner warnings for nonexistent local files. Add a BasePath field to remoteImportOrigin, derived from the parent workflowspec by stripping owner/repo and the last two path components (the 2-level import dir/file.md). Nested imports now resolve relative to this base, keeping cache paths consistent with the parent's path. Fixes #16370 Co-authored-by: Cursor --- pkg/parser/import_processor.go | 71 +++++--- pkg/parser/import_remote_nested_test.go | 205 ++++++++++++++++++------ 2 files changed, 205 insertions(+), 71 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index cc16c650de..3fe465895b 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -90,9 +90,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 path for resolving nested imports (e.g., "gh-agent-workflows" or ".github/workflows") } // importQueueItem represents a file to be imported with its context @@ -105,9 +106,21 @@ 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 workflowspec path by removing the last two path components +// (which represent the 2-level import: dir/file.md). For example: +// +// "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/file.md@sha" +// → BasePath = "gh-agent-workflows" +// +// "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/file.md@sha" +// → BasePath = ".github/workflows" +// +// "elastic/ai-github-actions/dir/file.md@sha" +// → BasePath = "" (import is directly under repo root) func parseRemoteOrigin(spec string) *remoteImportOrigin { // Remove section reference if present cleanSpec := spec @@ -129,10 +142,20 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { return nil } + // Extract base path: everything between owner/repo/ and the last 2 path components. + // Import paths are always 2-level (dir/file.md), so the base is the prefix + // before the import portion. For "owner/repo/base/dir/file.md", base = "base". + repoPathParts := slashParts[2:] // everything after owner/repo + var basePath string + if len(repoPathParts) > 2 { + basePath = strings.Join(repoPathParts[:len(repoPathParts)-2], "/") + } + return &remoteImportOrigin{ - Owner: slashParts[0], - Repo: slashParts[1], - Ref: ref, + Owner: slashParts[0], + Repo: slashParts[1], + Ref: ref, + BasePath: basePath, } } @@ -307,7 +330,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a if isWorkflowSpec(filePath) { origin = parseRemoteOrigin(filePath) if origin != nil { - importLog.Printf("Tracking remote origin for workflowspec: %s/%s@%s", origin.Owner, origin.Repo, origin.Ref) + importLog.Printf("Tracking remote origin for workflowspec: %s/%s@%s (basePath: %q)", origin.Owner, origin.Repo, origin.Ref, origin.BasePath) } } @@ -491,21 +514,27 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a resolvedPath := nestedFilePath var nestedRemoteOrigin *remoteImportOrigin - 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). - cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) + 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 parent's base + // path in the remote repo. The base path is derived from the parent + // workflowspec (e.g., "gh-agent-workflows" or ".github/workflows"). + cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) - // Reject paths that escape .github/workflows/ (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) - } + // 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 base directory", nestedFilePath, item.importPath) + } + + var basePrefix string + if item.remoteOrigin.BasePath != "" { + basePrefix = item.remoteOrigin.BasePath + "/" + } - resolvedPath = fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - item.remoteOrigin.Owner, item.remoteOrigin.Repo, cleanPath, item.remoteOrigin.Ref) - nestedRemoteOrigin = item.remoteOrigin - importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s", nestedFilePath, resolvedPath) + resolvedPath = fmt.Sprintf("%s/%s/%s%s@%s", + item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePrefix, cleanPath, item.remoteOrigin.Ref) + nestedRemoteOrigin = item.remoteOrigin + importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath: %q)", nestedFilePath, resolvedPath, item.remoteOrigin.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..d992d83e9f 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -21,39 +21,73 @@ func TestParseRemoteOrigin(t *testing.T) { expected *remoteImportOrigin }{ { - name: "basic workflowspec with ref", + name: "basic workflowspec with ref and deep base path", 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", }, }, { - name: "workflowspec with SHA ref", + name: "workflowspec with SHA ref and deep base path", 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", }, }, { - name: "workflowspec without ref defaults to main", + name: "workflowspec without ref defaults to main, 2-level path has empty base", 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: "", }, }, { 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: "", + }, + }, + { + name: "workflowspec through .github/workflows", + spec: "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/mention-in-pr-rwxp.md@main", + expected: &remoteImportOrigin{ + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: ".github/workflows", + }, + }, + { + name: "workflowspec with multi-level base path", + spec: "org/repo/a/b/c/dir/file.md@v2.0", + expected: &remoteImportOrigin{ + Owner: "org", + Repo: "repo", + Ref: "v2.0", + BasePath: "a/b/c", + }, + }, + { + name: "workflowspec with minimal path (owner/repo/file.md) has empty base", + 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") } }) } @@ -117,7 +152,19 @@ func TestRemoteOriginPropagation(t *testing.T) { // We can't easily test the full remote fetch flow in a unit test, // but we can verify the parsing and propagation logic - t.Run("workflowspec import gets remote origin", func(t *testing.T) { + // Helper to construct the resolved workflowspec for a nested import, + // mirroring the logic in the import processor. + resolveNested := func(origin *remoteImportOrigin, nestedPath string) string { + cleanPath := path.Clean(strings.TrimPrefix(nestedPath, "./")) + var basePrefix string + if origin.BasePath != "" { + basePrefix = origin.BasePath + "/" + } + return fmt.Sprintf("%s/%s/%s%s@%s", + origin.Owner, origin.Repo, basePrefix, cleanPath, origin.Ref) + } + + t.Run("workflowspec import gets remote origin with base path", func(t *testing.T) { spec := "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main" assert.True(t, isWorkflowSpec(spec), "Should be recognized as workflowspec") @@ -126,6 +173,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", origin.BasePath, "BasePath should be gh-agent-workflows") }) t.Run("local import does not get remote origin", func(t *testing.T) { @@ -136,50 +184,77 @@ 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 import resolves relative to parent base path (gh-agent-workflows)", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: "gh-agent-workflows", } - nestedPath := "shared/elastic-tools.md" + nestedPath := "gh-aw-fragments/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) + resolvedSpec := resolveNested(origin, nestedPath) 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", + "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/elastic-tools.md@main", + resolvedSpec, + "Nested import should resolve relative to parent's base path", ) + assert.True(t, isWorkflowSpec(resolvedSpec), "Constructed path should be a valid workflowspec") + }) + + t.Run("nested import resolves relative to .github/workflows base path", func(t *testing.T) { + origin := &remoteImportOrigin{ + Owner: "elastic", + Repo: "ai-github-actions", + Ref: "main", + BasePath: ".github/workflows", + } + nestedPath := "gh-aw-fragments/elastic-tools.md" + + resolvedSpec := resolveNested(origin, nestedPath) - // The constructed spec should be recognized as a workflowspec - assert.True(t, isWorkflowSpec(expectedSpec), "Constructed path should be a valid workflowspec") + assert.Equal(t, + "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/elastic-tools.md@main", + resolvedSpec, + "Nested import should resolve relative to .github/workflows/ base path", + ) + assert.True(t, isWorkflowSpec(resolvedSpec), "Constructed path should be a valid workflowspec") }) - t.Run("nested relative path with ./ prefix is cleaned", func(t *testing.T) { + t.Run("nested import with empty base path resolves at repo root", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "org", - Repo: "repo", - Ref: "v1.0", + Owner: "org", + Repo: "repo", + Ref: "main", + BasePath: "", } - nestedPath := "./shared/tools.md" + nestedPath := "shared/tools.md" - // Clean the ./ prefix as the import processor does - cleanPath := nestedPath - if len(cleanPath) > 2 && cleanPath[:2] == "./" { - cleanPath = cleanPath[2:] + resolvedSpec := resolveNested(origin, nestedPath) + + assert.Equal(t, + "org/repo/shared/tools.md@main", + resolvedSpec, + "Nested import with empty base should resolve directly under repo root", + ) + assert.True(t, isWorkflowSpec(resolvedSpec), "Constructed path should be a valid workflowspec") + }) + + t.Run("nested relative path with ./ prefix is cleaned", func(t *testing.T) { + origin := &remoteImportOrigin{ + Owner: "org", + Repo: "repo", + Ref: "v1.0", + BasePath: ".github/workflows", } + nestedPath := "./shared/tools.md" - expectedSpec := fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - origin.Owner, origin.Repo, cleanPath, origin.Ref) + resolvedSpec := resolveNested(origin, nestedPath) assert.Equal(t, "org/repo/.github/workflows/shared/tools.md@v1.0", - expectedSpec, + resolvedSpec, "Dot-prefix should be stripped when constructing remote spec", ) }) @@ -210,21 +285,51 @@ func TestRemoteOriginPropagation(t *testing.T) { t.Run("SHA ref is preserved in nested resolution", 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" + nestedPath := "gh-aw-fragments/formatting.md" - resolvedSpec := fmt.Sprintf("%s/%s/.github/workflows/%s@%s", - origin.Owner, origin.Repo, nestedPath, origin.Ref) + resolvedSpec := resolveNested(origin, nestedPath) assert.Equal(t, - "elastic/ai-github-actions/.github/workflows/shared/formatting.md@"+sha, + "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/formatting.md@"+sha, resolvedSpec, "SHA ref should be preserved for nested imports", ) }) + + t.Run("base path consistency between parent and nested imports", func(t *testing.T) { + // When the shim imports through gh-agent-workflows/, nested imports + // should also resolve through gh-agent-workflows/, not .github/workflows/ + parentSpec := "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/test-improvement-rwxp.md@abc123" + parentOrigin := parseRemoteOrigin(parentSpec) + require.NotNil(t, parentOrigin) + assert.Equal(t, "gh-agent-workflows", parentOrigin.BasePath) + + // Nested fragment import should use the same base + nestedSpec := resolveNested(parentOrigin, "gh-aw-fragments/elastic-tools.md") + assert.Equal(t, + "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/elastic-tools.md@abc123", + nestedSpec, + "Nested import should be consistent with parent's base path", + ) + + // Compare: if the same import goes through .github/workflows/ + altSpec := "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/test-improvement-rwxp.md@abc123" + altOrigin := parseRemoteOrigin(altSpec) + require.NotNil(t, altOrigin) + assert.Equal(t, ".github/workflows", altOrigin.BasePath) + + altNestedSpec := resolveNested(altOrigin, "gh-aw-fragments/elastic-tools.md") + assert.Equal(t, + "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/elastic-tools.md@abc123", + altNestedSpec, + "Nested import through .github/workflows should also be consistent", + ) + }) } func TestImportQueueItemRemoteOriginField(t *testing.T) { From 7034e6c722a86b3f8e7ebdbe8f8f46144a167c8e Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 09:06:40 -0600 Subject: [PATCH 2/6] use the resolved path as the import path --- pkg/parser/import_processor.go | 51 +++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 3fe465895b..5d24787c33 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -514,27 +514,27 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a resolvedPath := nestedFilePath var nestedRemoteOrigin *remoteImportOrigin - 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 parent's base - // path in the remote repo. The base path is derived from the parent - // workflowspec (e.g., "gh-agent-workflows" or ".github/workflows"). - cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) - - // 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 base directory", nestedFilePath, item.importPath) - } + 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 parent's base + // path in the remote repo. The base path is derived from the parent + // workflowspec (e.g., "gh-agent-workflows" or ".github/workflows"). + cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) + + // 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 base directory", nestedFilePath, item.importPath) + } - var basePrefix string - if item.remoteOrigin.BasePath != "" { - basePrefix = item.remoteOrigin.BasePath + "/" - } + var basePrefix string + if item.remoteOrigin.BasePath != "" { + basePrefix = item.remoteOrigin.BasePath + "/" + } - resolvedPath = fmt.Sprintf("%s/%s/%s%s@%s", - item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePrefix, cleanPath, item.remoteOrigin.Ref) - nestedRemoteOrigin = item.remoteOrigin - importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath: %q)", nestedFilePath, resolvedPath, item.remoteOrigin.BasePath) + resolvedPath = fmt.Sprintf("%s/%s/%s%s@%s", + item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePrefix, cleanPath, item.remoteOrigin.Ref) + nestedRemoteOrigin = item.remoteOrigin + importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath: %q)", nestedFilePath, resolvedPath, item.remoteOrigin.BasePath) } else if isWorkflowSpec(nestedFilePath) { // Nested import is itself a workflowspec - parse its remote origin nestedRemoteOrigin = parseRemoteOrigin(nestedFilePath) @@ -566,8 +566,19 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a // Check for cycles - skip if already visited if !visited[nestedFullPath] { visited[nestedFullPath] = true + + // Use the resolved path as the import path so that downstream + // phases (topological sort, security scan) can re-resolve it. + // For remote nested imports, resolvedPath is the full workflowspec + // (e.g., "elastic/repo/base/dir/file.md@sha"); for local and + // workflowspec imports it equals nestedFilePath (unchanged). + queueImportPath := resolvedPath + if nestedSectionName != "" { + queueImportPath += "#" + nestedSectionName + } + queue = append(queue, importQueueItem{ - importPath: nestedImportPath, + importPath: queueImportPath, fullPath: nestedFullPath, sectionName: nestedSectionName, baseDir: baseDir, // Use original baseDir, not nestedBaseDir From 2756063709c4b32224da00b90ab5b9a3e60db7de Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 09:48:28 -0600 Subject: [PATCH 3/6] Switch to FormatWorkflowSpec helper --- pkg/parser/import_cache.go | 10 +- pkg/parser/import_cache_integration_test.go | 14 +- pkg/parser/import_processor.go | 82 ++++++--- pkg/parser/import_remote_nested_test.go | 178 ++++++++++++++++++-- pkg/parser/remote_fetch.go | 24 +-- 5 files changed, 255 insertions(+), 53 deletions(-) diff --git a/pkg/parser/import_cache.go b/pkg/parser/import_cache.go index 5f9056a631..6bf0fd8190 100644 --- a/pkg/parser/import_cache.go +++ b/pkg/parser/import_cache.go @@ -76,22 +76,22 @@ func (c *ImportCache) Get(owner, repo, path, sha string) (string, bool) { // Check if the cached file exists if _, err := os.Stat(fullCachePath); err != nil { if os.IsNotExist(err) { - importCacheLog.Printf("Cache miss: %s/%s/%s@%s", owner, repo, path, sha) + importCacheLog.Printf("Cache miss: %s", FormatWorkflowSpec(owner, repo, path, sha)) } else { // Log other types of errors (permissions, I/O issues, etc.) - importCacheLog.Printf("Cache access error for %s/%s/%s@%s: %v", owner, repo, path, sha, err) + importCacheLog.Printf("Cache access error for %s: %v", FormatWorkflowSpec(owner, repo, path, sha), err) } return "", false } - importCacheLog.Printf("Cache hit: %s/%s/%s@%s -> %s", owner, repo, path, sha, fullCachePath) + importCacheLog.Printf("Cache hit: %s -> %s", FormatWorkflowSpec(owner, repo, path, sha), fullCachePath) return fullCachePath, true } // Set stores a new cache entry by saving the content to the cache directory // sha parameter should be the resolved commit SHA func (c *ImportCache) Set(owner, repo, path, sha string, content []byte) (string, error) { - importCacheLog.Printf("Setting cache entry: %s/%s/%s@%s, size=%d bytes", owner, repo, path, sha, len(content)) + importCacheLog.Printf("Setting cache entry: %s, size=%d bytes", FormatWorkflowSpec(owner, repo, path, sha), len(content)) // Validate file size (max 10MB) const maxFileSize = 10 * 1024 * 1024 @@ -131,7 +131,7 @@ func (c *ImportCache) Set(owner, repo, path, sha string, content []byte) (string return "", err } - importCacheLog.Printf("Cached import: %s/%s/%s@%s -> %s", owner, repo, path, sha, fullCachePath) + importCacheLog.Printf("Cached import: %s -> %s", FormatWorkflowSpec(owner, repo, path, sha), fullCachePath) return fullCachePath, nil } diff --git a/pkg/parser/import_cache_integration_test.go b/pkg/parser/import_cache_integration_test.go index 5f6e813b92..501126f3df 100644 --- a/pkg/parser/import_cache_integration_test.go +++ b/pkg/parser/import_cache_integration_test.go @@ -112,7 +112,7 @@ func TestImportCacheMultipleFiles(t *testing.T) { for _, f := range files { _, err := cache.Set(f.owner, f.repo, f.path, f.sha, []byte(f.content)) if err != nil { - t.Fatalf("Failed to cache file %s/%s/%s@%s: %v", f.owner, f.repo, f.path, f.sha, err) + t.Fatalf("Failed to cache file %s: %v", FormatWorkflowSpec(f.owner, f.repo, f.path, f.sha), err) } } @@ -120,7 +120,7 @@ func TestImportCacheMultipleFiles(t *testing.T) { for _, f := range files { path, found := cache.Get(f.owner, f.repo, f.path, f.sha) if !found { - t.Errorf("Failed to retrieve cached file %s/%s/%s@%s", f.owner, f.repo, f.path, f.sha) + t.Errorf("Failed to retrieve cached file %s", FormatWorkflowSpec(f.owner, f.repo, f.path, f.sha)) continue } @@ -131,8 +131,8 @@ func TestImportCacheMultipleFiles(t *testing.T) { } if string(content) != f.content { - t.Errorf("Content mismatch for %s/%s/%s@%s. Expected %q, got %q", - f.owner, f.repo, f.path, f.sha, f.content, string(content)) + t.Errorf("Content mismatch for %s. Expected %q, got %q", + FormatWorkflowSpec(f.owner, f.repo, f.path, f.sha), f.content, string(content)) } } @@ -142,7 +142,7 @@ func TestImportCacheMultipleFiles(t *testing.T) { for _, f := range files { path, found := cache2.Get(f.owner, f.repo, f.path, f.sha) if !found { - t.Errorf("Failed to retrieve cached file from new instance %s/%s/%s@%s", f.owner, f.repo, f.path, f.sha) + t.Errorf("Failed to retrieve cached file from new instance %s", FormatWorkflowSpec(f.owner, f.repo, f.path, f.sha)) continue } @@ -153,8 +153,8 @@ func TestImportCacheMultipleFiles(t *testing.T) { } if string(content) != f.content { - t.Errorf("Content mismatch from new instance for %s/%s/%s@%s. Expected %q, got %q", - f.owner, f.repo, f.path, f.sha, f.content, string(content)) + t.Errorf("Content mismatch from new instance for %s. Expected %q, got %q", + FormatWorkflowSpec(f.owner, f.repo, f.path, f.sha), f.content, string(content)) } } } diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 5d24787c33..dcb7d94115 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -86,6 +86,21 @@ func ProcessImportsFromFrontmatter(frontmatter map[string]any, baseDir string) ( return result.MergedTools, result.MergedEngines, nil } +// FormatWorkflowSpec formats a workflowspec string from its components. +// This is the canonical format used throughout the codebase for logging, +// error messages, cache keys, and spec construction. +// +// Examples: +// +// FormatWorkflowSpec("elastic", "ai-github-actions", "gh-agent-workflows/file.md", "main") +// → "elastic/ai-github-actions/gh-agent-workflows/file.md@main" +// +// FormatWorkflowSpec("org", "repo", "dir/file.md", "v1.0") +// → "org/repo/dir/file.md@v1.0" +func FormatWorkflowSpec(owner, repo, filePath, ref string) string { + return fmt.Sprintf("%s/%s/%s@%s", owner, repo, filePath, ref) +} + // remoteImportOrigin tracks the remote repository context for an imported file. // When a file is fetched from a remote GitHub repository via workflowspec, // its nested relative imports must be resolved against the same remote repo. @@ -96,6 +111,36 @@ type remoteImportOrigin struct { BasePath string // Base path for resolving nested imports (e.g., "gh-agent-workflows" or ".github/workflows") } +// String returns the origin identity as "owner/repo@ref" for display in logs. +func (o *remoteImportOrigin) String() string { + return fmt.Sprintf("%s/%s@%s", o.Owner, o.Repo, o.Ref) +} + +// ResolveNestedImport constructs a full workflowspec for a relative import path +// resolved against this origin's base path. The relative path is cleaned (e.g., +// "./" prefix stripped) and combined with the origin's owner, repo, base path, +// and ref to produce a valid workflowspec string. +// +// Examples: +// +// origin{Owner:"elastic", Repo:"ai-github-actions", Ref:"main", BasePath:"gh-agent-workflows"} +// .ResolveNestedImport("gh-aw-fragments/tools.md") +// → "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@main" +// +// origin{Owner:"org", Repo:"repo", Ref:"v1.0", BasePath:""} +// .ResolveNestedImport("dir/file.md") +// → "org/repo/dir/file.md@v1.0" +func (o *remoteImportOrigin) ResolveNestedImport(relativePath string) string { + cleanPath := path.Clean(strings.TrimPrefix(relativePath, "./")) + var fullPath string + if o.BasePath != "" { + fullPath = o.BasePath + "/" + cleanPath + } else { + fullPath = cleanPath + } + return FormatWorkflowSpec(o.Owner, o.Repo, fullPath, o.Ref) +} + // importQueueItem represents a file to be imported with its context type importQueueItem struct { importPath string // Original import path (e.g., "file.md" or "file.md#Section") @@ -110,17 +155,21 @@ type importQueueItem struct { // 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 workflowspec path by removing the last two path components -// (which represent the 2-level import: dir/file.md). For example: +// BasePath is the directory prefix between owner/repo/ and the final path components. +// It is extracted by removing the last 2 components (dir/file.md) from the repo-relative +// path when there are more than 2 components. Workflowspecs can have varying depths: // -// "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/file.md@sha" -// → BasePath = "gh-agent-workflows" +// 4+ components after owner/repo → BasePath = everything before the last 2: +// "elastic/repo/gh-agent-workflows/gh-aw-workflows/file.md@sha" +// → BasePath = "gh-agent-workflows" +// "elastic/repo/.github/workflows/gh-aw-workflows/file.md@sha" +// → BasePath = ".github/workflows" // -// "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/file.md@sha" -// → BasePath = ".github/workflows" +// Exactly 2 components after owner/repo → BasePath = "" (empty): +// "elastic/repo/dir/file.md@sha" → BasePath = "" // -// "elastic/ai-github-actions/dir/file.md@sha" -// → BasePath = "" (import is directly under repo root) +// Exactly 1 component after owner/repo → BasePath = "" (empty): +// "elastic/repo/file.md@sha" → BasePath = "" func parseRemoteOrigin(spec string) *remoteImportOrigin { // Remove section reference if present cleanSpec := spec @@ -143,8 +192,9 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { } // Extract base path: everything between owner/repo/ and the last 2 path components. - // Import paths are always 2-level (dir/file.md), so the base is the prefix - // before the import portion. For "owner/repo/base/dir/file.md", base = "base". + // When there are more than 2 components after owner/repo, the prefix before the + // last 2 is the base path. For "owner/repo/base/dir/file.md", base = "base". + // When there are 2 or fewer, the base path is empty. repoPathParts := slashParts[2:] // everything after owner/repo var basePath string if len(repoPathParts) > 2 { @@ -330,7 +380,7 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a if isWorkflowSpec(filePath) { origin = parseRemoteOrigin(filePath) if origin != nil { - importLog.Printf("Tracking remote origin for workflowspec: %s/%s@%s (basePath: %q)", origin.Owner, origin.Repo, origin.Ref, origin.BasePath) + importLog.Printf("Tracking remote origin for workflowspec: %s (basePath: %q)", origin, origin.BasePath) } } @@ -526,20 +576,14 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a return nil, fmt.Errorf("nested import '%s' from remote file '%s' escapes base directory", nestedFilePath, item.importPath) } - var basePrefix string - if item.remoteOrigin.BasePath != "" { - basePrefix = item.remoteOrigin.BasePath + "/" - } - - resolvedPath = fmt.Sprintf("%s/%s/%s%s@%s", - item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePrefix, cleanPath, item.remoteOrigin.Ref) + resolvedPath = item.remoteOrigin.ResolveNestedImport(nestedFilePath) nestedRemoteOrigin = item.remoteOrigin importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath: %q)", nestedFilePath, resolvedPath, item.remoteOrigin.BasePath) } else if isWorkflowSpec(nestedFilePath) { // Nested import is itself a workflowspec - parse its remote origin nestedRemoteOrigin = parseRemoteOrigin(nestedFilePath) if nestedRemoteOrigin != nil { - importLog.Printf("Nested workflowspec import detected: %s (origin: %s/%s@%s)", nestedFilePath, nestedRemoteOrigin.Owner, nestedRemoteOrigin.Repo, nestedRemoteOrigin.Ref) + importLog.Printf("Nested workflowspec import detected: %s (origin: %s)", nestedFilePath, nestedRemoteOrigin) } } diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index d992d83e9f..f88a7764c7 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -3,7 +3,6 @@ package parser import ( - "fmt" "os" "path" "path/filepath" @@ -118,6 +117,171 @@ func TestParseRemoteOrigin(t *testing.T) { } } +func TestFormatWorkflowSpec(t *testing.T) { + tests := []struct { + name string + owner string + repo string + filePath string + ref string + expected string + }{ + { + name: "basic spec with branch ref", + owner: "elastic", + repo: "ai-github-actions", + filePath: "gh-agent-workflows/file.md", + ref: "main", + expected: "elastic/ai-github-actions/gh-agent-workflows/file.md@main", + }, + { + name: "spec with SHA ref", + owner: "elastic", + repo: "ai-github-actions", + filePath: "gh-agent-workflows/gh-aw-fragments/tools.md", + ref: "160c33700227b5472dc3a08aeea1e774389a1a84", + expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@160c33700227b5472dc3a08aeea1e774389a1a84", + }, + { + name: "spec with tag ref", + owner: "org", + repo: "repo", + filePath: "dir/file.md", + ref: "v1.0", + expected: "org/repo/dir/file.md@v1.0", + }, + { + name: "spec with deep path", + owner: "org", + repo: "repo", + filePath: ".github/workflows/shared/tools.md", + ref: "main", + expected: "org/repo/.github/workflows/shared/tools.md@main", + }, + { + name: "spec with single file", + owner: "org", + repo: "repo", + filePath: "file.md", + ref: "main", + expected: "org/repo/file.md@main", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := FormatWorkflowSpec(tt.owner, tt.repo, tt.filePath, tt.ref) + assert.Equal(t, tt.expected, result, "FormatWorkflowSpec should produce canonical format") + }) + } +} + +func TestRemoteImportOriginString(t *testing.T) { + tests := []struct { + name string + origin *remoteImportOrigin + expected string + }{ + { + name: "basic origin", + origin: &remoteImportOrigin{Owner: "elastic", Repo: "ai-github-actions", Ref: "main"}, + expected: "elastic/ai-github-actions@main", + }, + { + name: "origin with SHA", + origin: &remoteImportOrigin{Owner: "org", Repo: "repo", Ref: "abc123"}, + expected: "org/repo@abc123", + }, + { + name: "origin with tag", + origin: &remoteImportOrigin{Owner: "org", Repo: "repo", Ref: "v2.0"}, + expected: "org/repo@v2.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.origin.String() + assert.Equal(t, tt.expected, result, "String() should return owner/repo@ref") + }) + } +} + +func TestResolveNestedImport(t *testing.T) { + tests := []struct { + name string + origin *remoteImportOrigin + relativePath string + expected string + }{ + { + name: "resolves relative to non-empty base path", + origin: &remoteImportOrigin{ + Owner: "elastic", Repo: "ai-github-actions", Ref: "main", BasePath: "gh-agent-workflows", + }, + relativePath: "gh-aw-fragments/tools.md", + expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@main", + }, + { + name: "resolves relative to .github/workflows base path", + origin: &remoteImportOrigin{ + Owner: "elastic", Repo: "ai-github-actions", Ref: "main", BasePath: ".github/workflows", + }, + relativePath: "gh-aw-fragments/tools.md", + expected: "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/tools.md@main", + }, + { + name: "resolves with empty base path at repo root", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "v1.0", BasePath: "", + }, + relativePath: "dir/file.md", + expected: "org/repo/dir/file.md@v1.0", + }, + { + name: "strips ./ prefix from relative path", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "v2.0", BasePath: ".github/workflows", + }, + relativePath: "./shared/tools.md", + expected: "org/repo/.github/workflows/shared/tools.md@v2.0", + }, + { + name: "preserves SHA ref", + origin: &remoteImportOrigin{ + Owner: "elastic", Repo: "ai-github-actions", + Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", BasePath: "gh-agent-workflows", + }, + relativePath: "gh-aw-fragments/formatting.md", + expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/formatting.md@160c33700227b5472dc3a08aeea1e774389a1a84", + }, + { + name: "multi-level base path", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "main", BasePath: "a/b/c", + }, + relativePath: "dir/file.md", + expected: "org/repo/a/b/c/dir/file.md@main", + }, + { + name: "single file without subdirectory", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "main", BasePath: "base", + }, + relativePath: "file.md", + expected: "org/repo/base/file.md@main", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.origin.ResolveNestedImport(tt.relativePath) + assert.Equal(t, tt.expected, result, "ResolveNestedImport(%q)", tt.relativePath) + assert.True(t, isWorkflowSpec(result), "Result should be a valid workflowspec") + }) + } +} + func TestLocalImportResolutionBaseline(t *testing.T) { // Baseline test: verifies local relative imports resolve correctly. // This ensures the import processor still works for non-remote imports @@ -152,16 +316,10 @@ func TestRemoteOriginPropagation(t *testing.T) { // We can't easily test the full remote fetch flow in a unit test, // but we can verify the parsing and propagation logic - // Helper to construct the resolved workflowspec for a nested import, - // mirroring the logic in the import processor. + // Use the ResolveNestedImport method directly - this is the same + // helper used by the import processor at runtime. resolveNested := func(origin *remoteImportOrigin, nestedPath string) string { - cleanPath := path.Clean(strings.TrimPrefix(nestedPath, "./")) - var basePrefix string - if origin.BasePath != "" { - basePrefix = origin.BasePath + "/" - } - return fmt.Sprintf("%s/%s/%s%s@%s", - origin.Owner, origin.Repo, basePrefix, cleanPath, origin.Ref) + return origin.ResolveNestedImport(nestedPath) } t.Run("workflowspec import gets remote origin with base path", func(t *testing.T) { diff --git a/pkg/parser/remote_fetch.go b/pkg/parser/remote_fetch.go index b2f8c4ce59..f85ce5028e 100644 --- a/pkg/parser/remote_fetch.go +++ b/pkg/parser/remote_fetch.go @@ -245,14 +245,14 @@ func downloadIncludeFromWorkflowSpec(spec string, cache *ImportCache) (string, e sha = resolvedSHA // Check cache using SHA if cachedPath, found := cache.Get(owner, repo, filePath, sha); found { - remoteLog.Printf("Using cached import: %s/%s/%s@%s (SHA: %s)", owner, repo, filePath, ref, sha) + remoteLog.Printf("Using cached import: %s (SHA: %s)", FormatWorkflowSpec(owner, repo, filePath, ref), sha) return cachedPath, nil } } } // Download the file content from GitHub - remoteLog.Printf("Fetching file from GitHub: %s/%s/%s@%s", owner, repo, filePath, ref) + remoteLog.Printf("Fetching file from GitHub: %s", FormatWorkflowSpec(owner, repo, filePath, ref)) content, err := downloadFileFromGitHub(owner, repo, filePath, ref) if err != nil { return "", fmt.Errorf("failed to download include from %s: %w", spec, err) @@ -391,7 +391,7 @@ func resolveRefToSHA(owner, repo, ref string) (string, error) { // downloadFileViaGit downloads a file from a Git repository using git commands // This is a fallback for when GitHub API authentication fails func downloadFileViaGit(owner, repo, path, ref string) ([]byte, error) { - remoteLog.Printf("Attempting git fallback for %s/%s/%s@%s", owner, repo, path, ref) + remoteLog.Printf("Attempting git fallback for %s", FormatWorkflowSpec(owner, repo, path, ref)) // Use git archive to get the file content without cloning // This works for public repositories without authentication @@ -414,14 +414,14 @@ func downloadFileViaGit(owner, repo, path, ref string) ([]byte, error) { return nil, fmt.Errorf("failed to extract file from git archive: %w", err) } - remoteLog.Printf("Successfully downloaded file via git archive: %s/%s/%s@%s", owner, repo, path, ref) + remoteLog.Printf("Successfully downloaded file via git archive: %s", FormatWorkflowSpec(owner, repo, path, ref)) return content, nil } // downloadFileViaGitClone downloads a file by shallow cloning the repository // This is used as a fallback when git archive doesn't work func downloadFileViaGitClone(owner, repo, path, ref string) ([]byte, error) { - remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref) + remoteLog.Printf("Attempting git clone fallback for %s", FormatWorkflowSpec(owner, repo, path, ref)) // Create a temporary directory for the shallow clone tmpDir, err := os.MkdirTemp("", "gh-aw-git-clone-*") @@ -469,7 +469,7 @@ func downloadFileViaGitClone(owner, repo, path, ref string) ([]byte, error) { return nil, fmt.Errorf("failed to read file from cloned repository: %w", err) } - remoteLog.Printf("Successfully downloaded file via git clone: %s/%s/%s@%s", owner, repo, path, ref) + remoteLog.Printf("Successfully downloaded file via git clone: %s", FormatWorkflowSpec(owner, repo, path, ref)) return content, nil } @@ -484,7 +484,7 @@ func isNotFoundError(errMsg string) bool { // A nil error with false means the path is not a symlink (e.g., it's a directory or file). func checkRemoteSymlink(client *api.RESTClient, owner, repo, dirPath, ref string) (string, bool, error) { endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", owner, repo, dirPath, ref) - remoteLog.Printf("Checking if path component is symlink: %s/%s/%s@%s", owner, repo, dirPath, ref) + remoteLog.Printf("Checking if path component is symlink: %s", FormatWorkflowSpec(owner, repo, dirPath, ref)) // The Contents API returns a JSON object for files/symlinks but a JSON array for directories. // Decode into json.RawMessage first to distinguish these cases without error-driven control flow. @@ -531,7 +531,7 @@ func resolveRemoteSymlinks(owner, repo, filePath, ref string) (string, error) { return "", fmt.Errorf("no directory components to resolve in path: %s", filePath) } - remoteLog.Printf("Attempting symlink resolution for %s/%s/%s@%s (%d path components)", owner, repo, filePath, ref, len(parts)) + remoteLog.Printf("Attempting symlink resolution for %s (%d path components)", FormatWorkflowSpec(owner, repo, filePath, ref), len(parts)) client, err := api.DefaultRESTClient() if err != nil { @@ -619,7 +619,7 @@ func downloadFileFromGitHubWithDepth(owner, repo, path, ref string, symlinkDepth // Check if this is an authentication error if gitutil.IsAuthError(errStr) { - remoteLog.Printf("GitHub API authentication failed, attempting git fallback for %s/%s/%s@%s", owner, repo, path, ref) + remoteLog.Printf("GitHub API authentication failed, attempting git fallback for %s", FormatWorkflowSpec(owner, repo, path, ref)) // Try fallback using git commands for public repositories content, gitErr := downloadFileViaGit(owner, repo, path, ref) if gitErr != nil { @@ -631,7 +631,7 @@ func downloadFileFromGitHubWithDepth(owner, repo, path, ref string, symlinkDepth // Check if this is a 404 — the path may traverse a symlink that the API doesn't follow if isNotFoundError(errStr) && symlinkDepth < constants.MaxSymlinkDepth { - remoteLog.Printf("File not found at %s/%s/%s@%s, checking for symlinks in path (depth: %d)", owner, repo, path, ref, symlinkDepth) + remoteLog.Printf("File not found at %s, checking for symlinks in path (depth: %d)", FormatWorkflowSpec(owner, repo, path, ref), symlinkDepth) resolvedPath, resolveErr := resolveRemoteSymlinks(owner, repo, path, ref) if resolveErr == nil && resolvedPath != path { remoteLog.Printf("Retrying download with symlink-resolved path: %s -> %s", path, resolvedPath) @@ -639,12 +639,12 @@ func downloadFileFromGitHubWithDepth(owner, repo, path, ref string, symlinkDepth } } - return nil, fmt.Errorf("failed to fetch file content from %s/%s/%s@%s: %w", owner, repo, path, ref, err) + return nil, fmt.Errorf("failed to fetch file content from %s: %w", FormatWorkflowSpec(owner, repo, path, ref), err) } // Verify we have content if fileContent.Content == "" { - return nil, fmt.Errorf("empty content returned from GitHub API for %s/%s/%s@%s", owner, repo, path, ref) + return nil, fmt.Errorf("empty content returned from GitHub API for %s", FormatWorkflowSpec(owner, repo, path, ref)) } // Decode base64 content using native Go base64 package From 6455c057ccb6187da8423ed60798c50337f2f487 Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 10:50:23 -0600 Subject: [PATCH 4/6] Update path handling, update tests --- pkg/parser/import_processor.go | 60 +++-- .../import_remote_nested_integration_test.go | 249 ++++++++++++++++++ pkg/parser/import_remote_nested_test.go | 237 ++++++++++------- 3 files changed, 423 insertions(+), 123 deletions(-) create mode 100644 pkg/parser/import_remote_nested_integration_test.go diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index dcb7d94115..953430abf7 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -117,26 +117,30 @@ func (o *remoteImportOrigin) String() string { } // ResolveNestedImport constructs a full workflowspec for a relative import path -// resolved against this origin's base path. The relative path is cleaned (e.g., -// "./" prefix stripped) and combined with the origin's owner, repo, base path, -// and ref to produce a valid workflowspec string. +// resolved against this origin's base path (the parent directory of the importing +// file). The relative path may use "../" to traverse up from the base path, +// following standard relative path semantics. // // Examples: // -// origin{Owner:"elastic", Repo:"ai-github-actions", Ref:"main", BasePath:"gh-agent-workflows"} -// .ResolveNestedImport("gh-aw-fragments/tools.md") +// origin{BasePath:"gh-agent-workflows/gh-aw-workflows"} +// .ResolveNestedImport("../gh-aw-fragments/tools.md") // → "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@main" // -// origin{Owner:"org", Repo:"repo", Ref:"v1.0", BasePath:""} +// origin{BasePath:"workflows"} +// .ResolveNestedImport("shared/reporting.md") +// → "org/repo/workflows/shared/reporting.md@main" +// +// origin{BasePath:""} // .ResolveNestedImport("dir/file.md") // → "org/repo/dir/file.md@v1.0" func (o *remoteImportOrigin) ResolveNestedImport(relativePath string) string { - cleanPath := path.Clean(strings.TrimPrefix(relativePath, "./")) + cleanRelative := path.Clean(strings.TrimPrefix(relativePath, "./")) var fullPath string if o.BasePath != "" { - fullPath = o.BasePath + "/" + cleanPath + fullPath = path.Clean(o.BasePath + "/" + cleanRelative) } else { - fullPath = cleanPath + fullPath = cleanRelative } return FormatWorkflowSpec(o.Owner, o.Repo, fullPath, o.Ref) } @@ -155,21 +159,23 @@ type importQueueItem struct { // 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 the directory prefix between owner/repo/ and the final path components. -// It is extracted by removing the last 2 components (dir/file.md) from the repo-relative -// path when there are more than 2 components. Workflowspecs can have varying depths: +// BasePath is the parent directory of the file within the repository. It is +// extracted by removing the last component (the filename) from the repo-relative +// path. Nested imports are resolved relative to this directory, following +// standard relative path semantics (including "../" traversal). +// +// Examples: // -// 4+ components after owner/repo → BasePath = everything before the last 2: -// "elastic/repo/gh-agent-workflows/gh-aw-workflows/file.md@sha" -// → BasePath = "gh-agent-workflows" -// "elastic/repo/.github/workflows/gh-aw-workflows/file.md@sha" -// → BasePath = ".github/workflows" +// "owner/repo/workflows/code-simplifier.md@main" +// → BasePath = "workflows" +// → import "shared/reporting.md" resolves to "workflows/shared/reporting.md" // -// Exactly 2 components after owner/repo → BasePath = "" (empty): -// "elastic/repo/dir/file.md@sha" → BasePath = "" +// "elastic/repo/gh-agent-workflows/gh-aw-workflows/file.md@sha" +// → BasePath = "gh-agent-workflows/gh-aw-workflows" +// → import "../gh-aw-fragments/tools.md" resolves to "gh-agent-workflows/gh-aw-fragments/tools.md" // -// Exactly 1 component after owner/repo → BasePath = "" (empty): -// "elastic/repo/file.md@sha" → BasePath = "" +// "owner/repo/file.md@sha" +// → BasePath = "" (file at repo root) func parseRemoteOrigin(spec string) *remoteImportOrigin { // Remove section reference if present cleanSpec := spec @@ -191,14 +197,14 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { return nil } - // Extract base path: everything between owner/repo/ and the last 2 path components. - // When there are more than 2 components after owner/repo, the prefix before the - // last 2 is the base path. For "owner/repo/base/dir/file.md", base = "base". - // When there are 2 or fewer, the base path is empty. + // Extract base path: the parent directory of the file within the repo. + // Remove only the last component (the filename) to get the directory path. + // For "owner/repo/dir/file.md", basePath = "dir". + // For "owner/repo/file.md", basePath = "" (file is at repo root). repoPathParts := slashParts[2:] // everything after owner/repo var basePath string - if len(repoPathParts) > 2 { - basePath = strings.Join(repoPathParts[:len(repoPathParts)-2], "/") + if len(repoPathParts) > 1 { + basePath = strings.Join(repoPathParts[:len(repoPathParts)-1], "/") } return &remoteImportOrigin{ diff --git a/pkg/parser/import_remote_nested_integration_test.go b/pkg/parser/import_remote_nested_integration_test.go new file mode 100644 index 0000000000..d9f036c8a7 --- /dev/null +++ b/pkg/parser/import_remote_nested_integration_test.go @@ -0,0 +1,249 @@ +//go:build integration + +package parser + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// skipOnAuthErr skips the test when GitHub API authentication is unavailable. +func skipOnAuthErr(t *testing.T, err error) { + t.Helper() + if err == nil { + return + } + msg := err.Error() + if strings.Contains(msg, "auth") || strings.Contains(msg, "forbidden") || + strings.Contains(msg, "authentication token not found") { + t.Skip("Skipping: GitHub API authentication not available") + } +} + +// TestRemoteNestedImportResolution verifies the end-to-end flow of importing +// a remote workflow whose nested relative imports must resolve through the +// parent's base path. +// +// Uses the githubnext/agentics repository which has this structure: +// +// workflows/ +// code-simplifier.md (imports shared/reporting.md) +// shared/ +// reporting.md (fragment with no further imports) +// +// When code-simplifier.md is imported via workflowspec, its nested import +// "shared/reporting.md" must resolve to workflows/shared/reporting.md using +// the parent file's directory (workflows/) as the base path. +func TestRemoteNestedImportResolution(t *testing.T) { + spec := "githubnext/agentics/workflows/code-simplifier.md@main" + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755), "Failed to create workflows directory") + + cache := NewImportCache(tmpDir) + + frontmatter := map[string]any{ + "imports": []any{spec}, + } + + result, err := ProcessImportsFromFrontmatterWithManifest(frontmatter, workflowsDir, cache) + if err != nil { + skipOnAuthErr(t, err) + t.Fatalf("ProcessImportsFromFrontmatterWithManifest failed: %v", err) + } + require.NotNil(t, result, "Result should not be nil") + + // --- Verify both the parent and nested import appear in ImportedFiles --- + t.Run("imported files contain parent and nested imports", func(t *testing.T) { + require.NotEmpty(t, result.ImportedFiles, "ImportedFiles should not be empty") + + foundParent := false + foundNested := false + for _, f := range result.ImportedFiles { + if strings.Contains(f, "code-simplifier.md") { + foundParent = true + } + if strings.Contains(f, "reporting.md") { + foundNested = true + } + } + assert.True(t, foundParent, "code-simplifier.md should be in ImportedFiles") + assert.True(t, foundNested, "reporting.md (nested import) should be in ImportedFiles") + }) + + // --- Verify ImportedFiles entries are full workflowspecs --- + t.Run("imported files are full workflowspecs", func(t *testing.T) { + for _, importedFile := range result.ImportedFiles { + assert.True(t, isWorkflowSpec(importedFile), + "Import should be a full workflowspec, got: %s", importedFile) + assert.Contains(t, importedFile, "githubnext/agentics/", + "Workflowspec should include owner/repo prefix: %s", importedFile) + assert.Contains(t, importedFile, "@", + "Workflowspec should include @ref suffix: %s", importedFile) + } + }) + + // --- Verify nested import resolved through correct base path --- + t.Run("nested import resolves through workflows base path", func(t *testing.T) { + found := false + for _, importedFile := range result.ImportedFiles { + if strings.Contains(importedFile, "reporting.md") { + found = true + assert.Contains(t, importedFile, "workflows/shared/reporting.md", + "Nested import should resolve through workflows/ directory: %s", importedFile) + } + } + assert.True(t, found, "Should have found reporting.md in ImportedFiles") + }) + + // --- Verify cache entries exist with correct path prefixes --- + t.Run("remote files are cached with correct prefixes", func(t *testing.T) { + fullCacheDir := cache.GetCacheDir() + + var cachedFiles []string + err := filepath.Walk(fullCacheDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return nil + } + if !info.IsDir() && strings.HasSuffix(info.Name(), ".md") { + cachedFiles = append(cachedFiles, info.Name()) + } + return nil + }) + require.NoError(t, err, "Walking cache directory should not error") + require.NotEmpty(t, cachedFiles, "Should have cached .md files") + t.Logf("Cached files: %v", cachedFiles) + + foundReporting := false + for _, name := range cachedFiles { + if strings.Contains(name, "reporting") { + foundReporting = true + assert.Contains(t, name, "workflows_shared_reporting.md", + "Cached reporting.md should have workflows_shared_ prefix: %s", name) + } + } + assert.True(t, foundReporting, "reporting.md should be in cache") + }) + + // --- Verify ImportPaths for runtime-import macro generation --- + t.Run("import paths reference cached files", func(t *testing.T) { + require.NotEmpty(t, result.ImportPaths, "ImportPaths should not be empty") + + for _, importPath := range result.ImportPaths { + assert.Contains(t, importPath, "imports/githubnext/agentics", + "Import path should reference cache directory: %s", importPath) + } + }) +} + +// TestRemoteImportCacheReuse verifies that processing the same remote +// workflowspec twice reuses cached content without re-downloading, and +// produces identical results. +func TestRemoteImportCacheReuse(t *testing.T) { + spec := "githubnext/agentics/workflows/shared/reporting.md@main" + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0o755)) + + cache := NewImportCache(tmpDir) + + frontmatter := map[string]any{ + "imports": []any{spec}, + } + + // First pass - downloads from GitHub + result1, err := ProcessImportsFromFrontmatterWithManifest(frontmatter, workflowsDir, cache) + if err != nil { + skipOnAuthErr(t, err) + t.Fatalf("First import pass failed: %v", err) + } + require.NotNil(t, result1) + + // Second pass - should use cache + result2, err := ProcessImportsFromFrontmatterWithManifest(frontmatter, workflowsDir, cache) + require.NoError(t, err, "Second import pass (cached) should succeed") + require.NotNil(t, result2) + + // Both passes should produce the same number of ImportedFiles + assert.Equal(t, len(result1.ImportedFiles), len(result2.ImportedFiles), + "Cached pass should produce same number of imported files") + + // Both should produce the same number of ImportPaths + assert.Equal(t, len(result1.ImportPaths), len(result2.ImportPaths), + "Cached pass should produce same number of import paths") +} + +// TestRemoteImportOriginParsing verifies that parseRemoteOrigin correctly +// extracts fields from real workflowspec paths in the githubnext/agentics +// repository, including BasePath extraction at different path depths. +func TestRemoteImportOriginParsing(t *testing.T) { + tests := []struct { + name string + spec string + expectedOwner string + expectedRepo string + expectedRef string + expectedBasePath string + }{ + { + name: "file in workflows directory has workflows base path", + spec: "githubnext/agentics/workflows/code-simplifier.md@main", + expectedOwner: "githubnext", + expectedRepo: "agentics", + expectedRef: "main", + expectedBasePath: "workflows", + }, + { + name: "file in workflows/shared has deeper base path", + spec: "githubnext/agentics/workflows/shared/reporting.md@main", + expectedOwner: "githubnext", + expectedRepo: "agentics", + expectedRef: "main", + expectedBasePath: "workflows/shared", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origin := parseRemoteOrigin(tt.spec) + require.NotNil(t, origin, "Should parse remote origin for spec: %s", tt.spec) + assert.Equal(t, tt.expectedOwner, origin.Owner, "Owner mismatch") + assert.Equal(t, tt.expectedRepo, origin.Repo, "Repo mismatch") + assert.Equal(t, tt.expectedRef, origin.Ref, "Ref mismatch") + assert.Equal(t, tt.expectedBasePath, origin.BasePath, "BasePath mismatch") + }) + } +} + +// TestRemoteImportResolveIncludePath verifies that ResolveIncludePath +// correctly downloads and caches files from the githubnext/agentics +// repository when given a full workflowspec. +func TestRemoteImportResolveIncludePath(t *testing.T) { + spec := "githubnext/agentics/workflows/shared/reporting.md@main" + + tmpDir := t.TempDir() + cache := NewImportCache(tmpDir) + + resolvedPath, err := ResolveIncludePath(spec, "", cache) + if err != nil { + skipOnAuthErr(t, err) + t.Fatalf("ResolveIncludePath failed: %v", err) + } + + assert.NotEmpty(t, resolvedPath, "Resolved path should not be empty") + assert.Contains(t, resolvedPath, "imports/githubnext/agentics", + "Path should be in cache directory") + + // Verify the file exists and has content + content, err := os.ReadFile(resolvedPath) + require.NoError(t, err, "Should be able to read cached file") + assert.NotEmpty(t, content, "Cached file should have content") + assert.Contains(t, string(content), "Report", "Content should be a reporting fragment") +} diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index f88a7764c7..aa61191fa6 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -20,33 +20,43 @@ func TestParseRemoteOrigin(t *testing.T) { expected *remoteImportOrigin }{ { - name: "basic workflowspec with ref and deep base path", - spec: "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main", + name: "deep path extracts parent directory as base path", + spec: "githubnext/agentics/workflows/shared/reporting.md@main", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", - BasePath: "gh-agent-workflows", + BasePath: "workflows/shared", }, }, { - name: "workflowspec with SHA ref and deep base path", - spec: "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@160c33700227b5472dc3a08aeea1e774389a1a84", + name: "workflowspec with SHA ref", + spec: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", - BasePath: "gh-agent-workflows", + Owner: "githubnext", + Repo: "agentics", + Ref: "acea14d65af123c315230221b409f4f435b3706f", + BasePath: "workflows/shared", }, }, { - name: "workflowspec without ref defaults to main, 2-level path has empty base", - spec: "elastic/ai-github-actions/gh-agent-workflows/file.md", + name: "2-level path extracts directory as base path", + spec: "githubnext/agentics/workflows/code-simplifier.md@main", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", - BasePath: "", + BasePath: "workflows", + }, + }, + { + name: "workflowspec without ref defaults to main", + spec: "githubnext/agentics/workflows/code-simplifier.md", + expected: &remoteImportOrigin{ + Owner: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: "workflows", }, }, { @@ -56,15 +66,15 @@ func TestParseRemoteOrigin(t *testing.T) { Owner: "owner", Repo: "repo", Ref: "v1.0", - BasePath: "", + BasePath: "path", }, }, { name: "workflowspec through .github/workflows", - spec: "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/mention-in-pr-rwxp.md@main", + spec: "githubnext/agentics/.github/workflows/code-simplifier.md@main", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", BasePath: ".github/workflows", }, @@ -76,7 +86,7 @@ func TestParseRemoteOrigin(t *testing.T) { Owner: "org", Repo: "repo", Ref: "v2.0", - BasePath: "a/b/c", + BasePath: "a/b/c/dir", }, }, { @@ -128,19 +138,19 @@ func TestFormatWorkflowSpec(t *testing.T) { }{ { name: "basic spec with branch ref", - owner: "elastic", - repo: "ai-github-actions", - filePath: "gh-agent-workflows/file.md", + owner: "githubnext", + repo: "agentics", + filePath: "workflows/code-simplifier.md", ref: "main", - expected: "elastic/ai-github-actions/gh-agent-workflows/file.md@main", + expected: "githubnext/agentics/workflows/code-simplifier.md@main", }, { name: "spec with SHA ref", - owner: "elastic", - repo: "ai-github-actions", - filePath: "gh-agent-workflows/gh-aw-fragments/tools.md", - ref: "160c33700227b5472dc3a08aeea1e774389a1a84", - expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@160c33700227b5472dc3a08aeea1e774389a1a84", + owner: "githubnext", + repo: "agentics", + filePath: "workflows/shared/reporting.md", + ref: "acea14d65af123c315230221b409f4f435b3706f", + expected: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", }, { name: "spec with tag ref", @@ -184,8 +194,8 @@ func TestRemoteImportOriginString(t *testing.T) { }{ { name: "basic origin", - origin: &remoteImportOrigin{Owner: "elastic", Repo: "ai-github-actions", Ref: "main"}, - expected: "elastic/ai-github-actions@main", + origin: &remoteImportOrigin{Owner: "githubnext", Repo: "agentics", Ref: "main"}, + expected: "githubnext/agentics@main", }, { name: "origin with SHA", @@ -215,20 +225,20 @@ func TestResolveNestedImport(t *testing.T) { expected string }{ { - name: "resolves relative to non-empty base path", + name: "resolves sibling file in same directory", origin: &remoteImportOrigin{ - Owner: "elastic", Repo: "ai-github-actions", Ref: "main", BasePath: "gh-agent-workflows", + Owner: "githubnext", Repo: "agentics", Ref: "main", BasePath: "workflows", }, - relativePath: "gh-aw-fragments/tools.md", - expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/tools.md@main", + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/workflows/shared/reporting.md@main", }, { name: "resolves relative to .github/workflows base path", origin: &remoteImportOrigin{ - Owner: "elastic", Repo: "ai-github-actions", Ref: "main", BasePath: ".github/workflows", + Owner: "githubnext", Repo: "agentics", Ref: "main", BasePath: ".github/workflows", }, - relativePath: "gh-aw-fragments/tools.md", - expected: "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/tools.md@main", + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/.github/workflows/shared/reporting.md@main", }, { name: "resolves with empty base path at repo root", @@ -249,11 +259,11 @@ func TestResolveNestedImport(t *testing.T) { { name: "preserves SHA ref", origin: &remoteImportOrigin{ - Owner: "elastic", Repo: "ai-github-actions", - Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", BasePath: "gh-agent-workflows", + Owner: "githubnext", Repo: "agentics", + Ref: "acea14d65af123c315230221b409f4f435b3706f", BasePath: "workflows", }, - relativePath: "gh-aw-fragments/formatting.md", - expected: "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/formatting.md@160c33700227b5472dc3a08aeea1e774389a1a84", + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", }, { name: "multi-level base path", @@ -266,10 +276,26 @@ func TestResolveNestedImport(t *testing.T) { { name: "single file without subdirectory", origin: &remoteImportOrigin{ - Owner: "org", Repo: "repo", Ref: "main", BasePath: "base", + Owner: "org", Repo: "repo", Ref: "main", BasePath: "workflows", }, relativePath: "file.md", - expected: "org/repo/base/file.md@main", + expected: "org/repo/workflows/file.md@main", + }, + { + name: "parent traversal with ../ resolves correctly", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "main", BasePath: "base/subdir", + }, + relativePath: "../sibling/file.md", + expected: "org/repo/base/sibling/file.md@main", + }, + { + name: "double parent traversal ../../ resolves correctly", + origin: &remoteImportOrigin{ + Owner: "org", Repo: "repo", Ref: "main", BasePath: "a/b/c", + }, + relativePath: "../../other/file.md", + expected: "org/repo/a/other/file.md@main", }, } @@ -323,15 +349,15 @@ func TestRemoteOriginPropagation(t *testing.T) { } t.Run("workflowspec import gets remote origin with base path", func(t *testing.T) { - spec := "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main" + spec := "githubnext/agentics/workflows/shared/reporting.md@main" assert.True(t, isWorkflowSpec(spec), "Should be recognized as workflowspec") origin := parseRemoteOrigin(spec) require.NotNil(t, origin, "Should parse remote origin") - 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, "githubnext", origin.Owner, "Owner should be githubnext") + assert.Equal(t, "agentics", origin.Repo, "Repo should be agentics") assert.Equal(t, "main", origin.Ref, "Ref should be main") - assert.Equal(t, "gh-agent-workflows", origin.BasePath, "BasePath should be gh-agent-workflows") + assert.Equal(t, "workflows/shared", origin.BasePath, "BasePath should be workflows/shared") }) t.Run("local import does not get remote origin", func(t *testing.T) { @@ -342,38 +368,38 @@ func TestRemoteOriginPropagation(t *testing.T) { assert.Nil(t, origin, "Local paths should not produce remote origin") }) - t.Run("nested import resolves relative to parent base path (gh-agent-workflows)", func(t *testing.T) { + t.Run("nested import resolves relative to parent directory", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", - BasePath: "gh-agent-workflows", + BasePath: "workflows", } - nestedPath := "gh-aw-fragments/elastic-tools.md" + nestedPath := "shared/reporting.md" resolvedSpec := resolveNested(origin, nestedPath) assert.Equal(t, - "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/elastic-tools.md@main", + "githubnext/agentics/workflows/shared/reporting.md@main", resolvedSpec, - "Nested import should resolve relative to parent's base path", + "Nested import should resolve relative to parent's directory", ) assert.True(t, isWorkflowSpec(resolvedSpec), "Constructed path should be a valid workflowspec") }) t.Run("nested import resolves relative to .github/workflows base path", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", BasePath: ".github/workflows", } - nestedPath := "gh-aw-fragments/elastic-tools.md" + nestedPath := "shared/reporting.md" resolvedSpec := resolveNested(origin, nestedPath) assert.Equal(t, - "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/elastic-tools.md@main", + "githubnext/agentics/.github/workflows/shared/reporting.md@main", resolvedSpec, "Nested import should resolve relative to .github/workflows/ base path", ) @@ -417,6 +443,25 @@ func TestRemoteOriginPropagation(t *testing.T) { ) }) + t.Run("nested ../ traversal resolves to sibling directory", func(t *testing.T) { + origin := &remoteImportOrigin{ + Owner: "org", + Repo: "repo", + Ref: "main", + BasePath: "base/subdir", + } + nestedPath := "../fragments/tools.md" + + resolvedSpec := resolveNested(origin, nestedPath) + + assert.Equal(t, + "org/repo/base/fragments/tools.md@main", + resolvedSpec, + "../ should traverse up from base path to resolve sibling directory", + ) + assert.True(t, isWorkflowSpec(resolvedSpec), "Constructed path should be a valid workflowspec") + }) + t.Run("nested workflowspec from remote parent gets its own origin", func(t *testing.T) { // If a remote file references another workflowspec, it should // get its own origin, not inherit the parent's @@ -441,49 +486,49 @@ func TestRemoteOriginPropagation(t *testing.T) { }) t.Run("SHA ref is preserved in nested resolution", func(t *testing.T) { - sha := "160c33700227b5472dc3a08aeea1e774389a1a84" + sha := "acea14d65af123c315230221b409f4f435b3706f" origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: sha, - BasePath: "gh-agent-workflows", + BasePath: "workflows", } - nestedPath := "gh-aw-fragments/formatting.md" + nestedPath := "shared/reporting.md" resolvedSpec := resolveNested(origin, nestedPath) assert.Equal(t, - "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/formatting.md@"+sha, + "githubnext/agentics/workflows/shared/reporting.md@"+sha, resolvedSpec, "SHA ref should be preserved for nested imports", ) }) t.Run("base path consistency between parent and nested imports", func(t *testing.T) { - // When the shim imports through gh-agent-workflows/, nested imports - // should also resolve through gh-agent-workflows/, not .github/workflows/ - parentSpec := "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/test-improvement-rwxp.md@abc123" + // When importing through workflows/, nested imports + // should also resolve through workflows/ + parentSpec := "githubnext/agentics/workflows/code-simplifier.md@abc123" parentOrigin := parseRemoteOrigin(parentSpec) require.NotNil(t, parentOrigin) - assert.Equal(t, "gh-agent-workflows", parentOrigin.BasePath) + assert.Equal(t, "workflows", parentOrigin.BasePath) - // Nested fragment import should use the same base - nestedSpec := resolveNested(parentOrigin, "gh-aw-fragments/elastic-tools.md") + // Nested import should use the same base + nestedSpec := resolveNested(parentOrigin, "shared/reporting.md") assert.Equal(t, - "elastic/ai-github-actions/gh-agent-workflows/gh-aw-fragments/elastic-tools.md@abc123", + "githubnext/agentics/workflows/shared/reporting.md@abc123", nestedSpec, "Nested import should be consistent with parent's base path", ) // Compare: if the same import goes through .github/workflows/ - altSpec := "elastic/ai-github-actions/.github/workflows/gh-aw-workflows/test-improvement-rwxp.md@abc123" + altSpec := "githubnext/agentics/.github/workflows/code-simplifier.md@abc123" altOrigin := parseRemoteOrigin(altSpec) require.NotNil(t, altOrigin) assert.Equal(t, ".github/workflows", altOrigin.BasePath) - altNestedSpec := resolveNested(altOrigin, "gh-aw-fragments/elastic-tools.md") + altNestedSpec := resolveNested(altOrigin, "shared/reporting.md") assert.Equal(t, - "elastic/ai-github-actions/.github/workflows/gh-aw-fragments/elastic-tools.md@abc123", + "githubnext/agentics/.github/workflows/shared/reporting.md@abc123", altNestedSpec, "Nested import through .github/workflows should also be consistent", ) @@ -506,20 +551,20 @@ func TestImportQueueItemRemoteOriginField(t *testing.T) { t.Run("queue item with remote origin", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", + Owner: "githubnext", + Repo: "agentics", Ref: "main", } item := importQueueItem{ - importPath: "elastic/ai-github-actions/path/file.md@main", + importPath: "githubnext/agentics/workflows/file.md@main", fullPath: "/tmp/cache/file.md", sectionName: "", baseDir: "/workspace/.github/workflows", remoteOrigin: origin, } require.NotNil(t, item.remoteOrigin, "Remote import should have non-nil remote origin") - 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, "githubnext", item.remoteOrigin.Owner, "Owner should match") + assert.Equal(t, "agentics", item.remoteOrigin.Repo, "Repo should match") assert.Equal(t, "main", item.remoteOrigin.Ref, "Ref should match") }) } @@ -582,24 +627,24 @@ func TestResolveRemoteSymlinksPathConstruction(t *testing.T) { t.Run("symlink target resolution logic", func(t *testing.T) { // Verify the path math that resolveRemoteSymlinks performs internally. - // Given a symlink at .github/workflows/shared -> ../../gh-agent-workflows/shared, - // the resolution should produce gh-agent-workflows/shared/file.md + // Given a symlink at .github/workflows/shared -> ../../workflows/shared, + // the resolution should produce workflows/shared/file.md - // Simulate: parts = [".github", "workflows", "shared", "elastic-tools.md"] + // Simulate: parts = [".github", "workflows", "shared", "reporting.md"] // Symlink at index 3 (parts[:3] = ".github/workflows/shared") - // Target: "../../gh-agent-workflows/shared" + // Target: "../../workflows/shared" // Parent: ".github/workflows" parentDir := ".github/workflows" - target := "../../gh-agent-workflows/shared" + target := "../../workflows/shared" // This mirrors the logic in resolveRemoteSymlinks using path.Clean/path.Join resolvedBase := path.Clean(path.Join(parentDir, target)) - remaining := "elastic-tools.md" + remaining := "reporting.md" resolvedPath := resolvedBase + "/" + remaining - assert.Equal(t, "gh-agent-workflows/shared/elastic-tools.md", resolvedPath, - "Symlink at .github/workflows/shared pointing to ../../gh-agent-workflows/shared should resolve correctly") + assert.Equal(t, "workflows/shared/reporting.md", resolvedPath, + "Symlink at .github/workflows/shared pointing to ../../workflows/shared should resolve correctly") }) t.Run("symlink at first component", func(t *testing.T) { @@ -618,19 +663,19 @@ func TestResolveRemoteSymlinksPathConstruction(t *testing.T) { }) t.Run("nested symlink resolution", func(t *testing.T) { - // Simulate: parts = ["gh-agent-workflows", "gh-aw-workflows", "file.md"] - // Symlink at index 2 (parts[:2] = "gh-agent-workflows/gh-aw-workflows") - // Target: "../.github/workflows/gh-aw-workflows" - // Parent: "gh-agent-workflows" + // Simulate: parts = ["workflows", "nested", "file.md"] + // Symlink at index 2 (parts[:2] = "workflows/nested") + // Target: "../.github/workflows/nested" + // Parent: "workflows" - parentDir := "gh-agent-workflows" - target := "../.github/workflows/gh-aw-workflows" + parentDir := "workflows" + target := "../.github/workflows/nested" resolvedBase := path.Clean(path.Join(parentDir, target)) remaining := "file.md" resolvedPath := resolvedBase + "/" + remaining - assert.Equal(t, ".github/workflows/gh-aw-workflows/file.md", resolvedPath, + assert.Equal(t, ".github/workflows/nested/file.md", resolvedPath, "Nested symlink with ../ target should resolve correctly") }) } From c91d9d4ac33ab7ee06588c201cf42d06afc834cb Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 10:51:12 -0600 Subject: [PATCH 5/6] Allow path traversal for imports --- pkg/parser/import_processor.go | 20 ++++++++++++++------ pkg/parser/import_remote_nested_test.go | 25 +++++++++++++++++-------- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 953430abf7..cbe3833bd6 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -573,16 +573,24 @@ 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 parent's base - // path in the remote repo. The base path is derived from the parent - // workflowspec (e.g., "gh-agent-workflows" or ".github/workflows"). - cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) + // path (parent directory) in the remote repo. Relative paths including + // "../" traversals are supported via path.Clean in ResolveNestedImport. - // 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 base directory", nestedFilePath, item.importPath) + // Reject absolute paths + if path.IsAbs(nestedFilePath) { + return nil, fmt.Errorf("nested import '%s' from remote file '%s' uses absolute path", nestedFilePath, item.importPath) } resolvedPath = item.remoteOrigin.ResolveNestedImport(nestedFilePath) + + // After resolving against the base path, reject paths that escape + // the repository root (resolved path starts with "../") + resolvedFile := strings.SplitN(resolvedPath, "@", 2)[0] // strip @ref + repoRelative := strings.SplitN(resolvedFile, "/", 3) // owner/repo/path + if len(repoRelative) >= 3 && strings.HasPrefix(repoRelative[2], "..") { + return nil, fmt.Errorf("nested import '%s' from remote file '%s' escapes repository root", nestedFilePath, item.importPath) + } + nestedRemoteOrigin = item.remoteOrigin importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath: %q)", nestedFilePath, resolvedPath, item.remoteOrigin.BasePath) } else if isWorkflowSpec(nestedFilePath) { diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index aa61191fa6..3a6ce0256e 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -475,14 +475,23 @@ func TestRemoteOriginPropagation(t *testing.T) { assert.Equal(t, "v2.0", origin.Ref, "Should use nested spec's ref") }) - t.Run("path traversal in nested import is rejected", func(t *testing.T) { - // A nested import like ../../../etc/passwd should be rejected - // when constructing the remote workflowspec - nestedPath := "../../../etc/passwd" - cleanPath := path.Clean(strings.TrimPrefix(nestedPath, "./")) - - assert.True(t, strings.HasPrefix(cleanPath, ".."), - "Cleaned path should start with .. and be rejected by the import processor") + t.Run("path traversal escaping repo root produces invalid path", func(t *testing.T) { + // A nested import like ../../../etc/passwd from a shallow base path + // would resolve to a path starting with ".." after path.Clean, + // which the import processor detects and rejects. + origin := &remoteImportOrigin{ + Owner: "org", + Repo: "repo", + Ref: "main", + BasePath: "workflows", + } + resolvedSpec := resolveNested(origin, "../../../etc/passwd") + + // The resolved spec contains ".." in the repo-relative path, + // which the import processor rejects as escaping the repo root. + repoRelative := strings.SplitN(strings.SplitN(resolvedSpec, "@", 2)[0], "/", 3) + assert.True(t, len(repoRelative) >= 3 && strings.HasPrefix(repoRelative[2], ".."), + "Path escaping repo root should produce repo-relative path starting with ..: got %s", resolvedSpec) }) t.Run("SHA ref is preserved in nested resolution", func(t *testing.T) { From 6267356db0885e0db0f640bea81de45efd632b16 Mon Sep 17 00:00:00 2001 From: William Easton Date: Tue, 17 Feb 2026 10:55:18 -0600 Subject: [PATCH 6/6] lint --- pkg/parser/import_processor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 190b9cbd5f..f195768a4e 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -585,8 +585,8 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a // After resolving against the base path, reject paths that escape // the repository root (resolved path starts with "../") - resolvedFile := strings.SplitN(resolvedPath, "@", 2)[0] // strip @ref - repoRelative := strings.SplitN(resolvedFile, "/", 3) // owner/repo/path + resolvedFile := strings.SplitN(resolvedPath, "@", 2)[0] // strip @ref + repoRelative := strings.SplitN(resolvedFile, "/", 3) // owner/repo/path if len(repoRelative) >= 3 && strings.HasPrefix(repoRelative[2], "..") { return nil, fmt.Errorf("nested import '%s' from remote file '%s' escapes repository root", nestedFilePath, item.importPath) }