diff --git a/pkg/parser/import_cache.go b/pkg/parser/import_cache.go index 5f9056a631a..6bf0fd81903 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 5f6e813b92c..501126f3df0 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 6bb72569962..a8af6db3688 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -85,13 +85,63 @@ 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. 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") +} + +// 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 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{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{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 { + cleanRelative := path.Clean(strings.TrimPrefix(relativePath, "./")) + var fullPath string + if o.BasePath != "" { + fullPath = path.Clean(o.BasePath + "/" + cleanRelative) + } else { + fullPath = cleanRelative + } + return FormatWorkflowSpec(o.Owner, o.Repo, fullPath, o.Ref) } // importQueueItem represents a file to be imported with its context @@ -104,9 +154,27 @@ 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 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: +// +// "owner/repo/workflows/code-simplifier.md@main" +// → BasePath = "workflows" +// → import "shared/reporting.md" resolves to "workflows/shared/reporting.md" +// +// "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" +// +// "owner/repo/file.md@sha" +// → BasePath = "" (file at repo root) func parseRemoteOrigin(spec string) *remoteImportOrigin { // Remove section reference if present cleanSpec := spec @@ -128,10 +196,21 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { return nil } + // 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) > 1 { + basePath = strings.Join(repoPathParts[:len(repoPathParts)-1], "/") + } + return &remoteImportOrigin{ - Owner: slashParts[0], - Repo: slashParts[1], - Ref: ref, + Owner: slashParts[0], + Repo: slashParts[1], + Ref: ref, + BasePath: basePath, } } @@ -306,7 +385,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 (basePath: %q)", origin, origin.BasePath) } } @@ -492,24 +571,32 @@ 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). - cleanPath := path.Clean(strings.TrimPrefix(nestedFilePath, "./")) + // Convert to a workflowspec that resolves against the parent's base + // path (parent directory) in the remote repo. Relative paths including + // "../" traversals are supported via path.Clean in ResolveNestedImport. - // 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 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) } - 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) + 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) } } @@ -536,8 +623,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 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 00000000000..d9f036c8a70 --- /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 36d2dc3369a..3a6ce0256e5 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" @@ -21,39 +20,83 @@ func TestParseRemoteOrigin(t *testing.T) { expected *remoteImportOrigin }{ { - name: "basic workflowspec with ref", - 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", - Ref: "main", + Owner: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: "workflows/shared", }, }, { name: "workflowspec with SHA ref", - spec: "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@160c33700227b5472dc3a08aeea1e774389a1a84", + spec: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "160c33700227b5472dc3a08aeea1e774389a1a84", + Owner: "githubnext", + Repo: "agentics", + Ref: "acea14d65af123c315230221b409f4f435b3706f", + BasePath: "workflows/shared", + }, + }, + { + name: "2-level path extracts directory as base path", + spec: "githubnext/agentics/workflows/code-simplifier.md@main", + expected: &remoteImportOrigin{ + Owner: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: "workflows", }, }, { name: "workflowspec without ref defaults to main", - spec: "elastic/ai-github-actions/gh-agent-workflows/file.md", + spec: "githubnext/agentics/workflows/code-simplifier.md", expected: &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: "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 through .github/workflows", + spec: "githubnext/agentics/.github/workflows/code-simplifier.md@main", + expected: &remoteImportOrigin{ + Owner: "githubnext", + Repo: "agentics", + 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/dir", + }, + }, + { + 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,11 +121,193 @@ 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") } }) } } +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: "githubnext", + repo: "agentics", + filePath: "workflows/code-simplifier.md", + ref: "main", + expected: "githubnext/agentics/workflows/code-simplifier.md@main", + }, + { + name: "spec with SHA ref", + owner: "githubnext", + repo: "agentics", + filePath: "workflows/shared/reporting.md", + ref: "acea14d65af123c315230221b409f4f435b3706f", + expected: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", + }, + { + 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: "githubnext", Repo: "agentics", Ref: "main"}, + expected: "githubnext/agentics@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 sibling file in same directory", + origin: &remoteImportOrigin{ + Owner: "githubnext", Repo: "agentics", Ref: "main", BasePath: "workflows", + }, + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/workflows/shared/reporting.md@main", + }, + { + name: "resolves relative to .github/workflows base path", + origin: &remoteImportOrigin{ + Owner: "githubnext", Repo: "agentics", Ref: "main", BasePath: ".github/workflows", + }, + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/.github/workflows/shared/reporting.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: "githubnext", Repo: "agentics", + Ref: "acea14d65af123c315230221b409f4f435b3706f", BasePath: "workflows", + }, + relativePath: "shared/reporting.md", + expected: "githubnext/agentics/workflows/shared/reporting.md@acea14d65af123c315230221b409f4f435b3706f", + }, + { + 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: "workflows", + }, + relativePath: "file.md", + 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", + }, + } + + 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 @@ -117,15 +342,22 @@ 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) { - spec := "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main" + // Use the ResolveNestedImport method directly - this is the same + // helper used by the import processor at runtime. + resolveNested := func(origin *remoteImportOrigin, nestedPath string) string { + return origin.ResolveNestedImport(nestedPath) + } + + t.Run("workflowspec import gets remote origin with base path", func(t *testing.T) { + 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, "workflows/shared", origin.BasePath, "BasePath should be workflows/shared") }) t.Run("local import does not get remote origin", func(t *testing.T) { @@ -136,54 +368,100 @@ 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 directory", func(t *testing.T) { origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: "main", + Owner: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: "workflows", } - nestedPath := "shared/elastic-tools.md" + nestedPath := "shared/reporting.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", + "githubnext/agentics/workflows/shared/reporting.md@main", + resolvedSpec, + "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: "githubnext", + Repo: "agentics", + Ref: "main", + BasePath: ".github/workflows", + } + nestedPath := "shared/reporting.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, + "githubnext/agentics/.github/workflows/shared/reporting.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" + + 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") + }) - // Clean the ./ prefix as the import processor does - cleanPath := nestedPath - if len(cleanPath) > 2 && cleanPath[:2] == "./" { - cleanPath = cleanPath[2:] + 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", ) }) + 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 @@ -197,34 +475,73 @@ 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, "./")) + 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") - assert.True(t, strings.HasPrefix(cleanPath, ".."), - "Cleaned path should start with .. and be rejected by the import processor") + // 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) { - sha := "160c33700227b5472dc3a08aeea1e774389a1a84" + sha := "acea14d65af123c315230221b409f4f435b3706f" origin := &remoteImportOrigin{ - Owner: "elastic", - Repo: "ai-github-actions", - Ref: sha, + Owner: "githubnext", + Repo: "agentics", + Ref: sha, + BasePath: "workflows", } - nestedPath := "shared/formatting.md" + nestedPath := "shared/reporting.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, + "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 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, "workflows", parentOrigin.BasePath) + + // Nested import should use the same base + nestedSpec := resolveNested(parentOrigin, "shared/reporting.md") + assert.Equal(t, + "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 := "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, "shared/reporting.md") + assert.Equal(t, + "githubnext/agentics/.github/workflows/shared/reporting.md@abc123", + altNestedSpec, + "Nested import through .github/workflows should also be consistent", + ) + }) } func TestImportQueueItemRemoteOriginField(t *testing.T) { @@ -243,20 +560,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") }) } @@ -319,24 +636,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) { @@ -355,19 +672,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") }) } diff --git a/pkg/parser/remote_fetch.go b/pkg/parser/remote_fetch.go index e3960cbc1a4..606815badff 100644 --- a/pkg/parser/remote_fetch.go +++ b/pkg/parser/remote_fetch.go @@ -247,14 +247,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) @@ -394,7 +394,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 @@ -418,14 +418,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-*") @@ -474,7 +474,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 } @@ -489,7 +489,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. @@ -536,7 +536,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 { @@ -643,7 +643,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 { @@ -655,7 +655,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) @@ -663,12 +663,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