diff --git a/docs/adr/53895-consolidate-wasm-native-parser-helpers.md b/docs/adr/53895-consolidate-wasm-native-parser-helpers.md new file mode 100644 index 00000000000..a8cf3c4956b --- /dev/null +++ b/docs/adr/53895-consolidate-wasm-native-parser-helpers.md @@ -0,0 +1,56 @@ +# ADR-53895: Consolidate Duplicated Wasm/Native Parser Helpers into Build-Tag-Free Files + +**Date**: 2026-08-19 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +`pkg/parser` maintained separate implementations of five path-predicate helpers — `isUnderWorkflowsDirectory`, `isCustomAgentFile`, `isRepositoryImport`, `IsWorkflowSpec`, and the path-arithmetic helpers `findGitHubFolder` / `computeIncludeResolveAndSecurityBases` — duplicated across `remote_fetch_wasm.go` (build tag `js || wasm`) and `remote_resolve_path.go` / `remote_workflow_spec.go` (build tag `!js && !wasm`). These copies were not thin platform shims; they contained identical business logic, and they had already drifted: the wasm copy of `isRepositoryImport` rejected any repo name containing a dot (`strings.Contains(repo, ".")`), while the native copy rejected only known data-file extensions (`.md`, `.yaml`, `.yml`, `.json`). As a result, a valid import like `githubnext/gh-aw.dev` was accepted on native builds and silently rejected on wasm. The wasm copy also hardcoded string literals (`".github/workflows/"`, `".github/agents/"`) that the native copy correctly referenced via package constants. The repository already contained the right pattern for this situation: `github_token_env.go` is a build-tag-free file that both `github.go` (native) and `github_wasm.go` (wasm) delegate to for shared env-var logic. + +### Decision + +We will extract all platform-independent path predicates and path-arithmetic helpers into a new build-tag-free file `pkg/parser/remote_path_predicates.go` (plus `path_section.go` for the `path#section` splitter), standardize on the native `isRepositoryImport` semantics (extension-based rejection, not dot-based), and delete the duplicated implementations from the build-tag-specific files. Only the filesystem-probe step (which genuinely differs: `os.Stat` vs. `VirtualFileExists`) remains in the platform-specific files. + +### Alternatives Considered + +#### Alternative 1: Keep both copies in sync with comments and documentation + +Add comments in both files and a CONTRIBUTING note reminding developers to update both the wasm and native copy whenever either changes. This requires zero structural change and no risk of behavioral regression. + +Why not chosen: the drift in `isRepositoryImport` demonstrates this approach already failed. Cognitive load on every future contributor, with no enforcement mechanism, means further drift is certain. The behavioral bug would recur. + +#### Alternative 2: Define a platform abstraction interface + +Define a `PathPredicates` interface and have each build target provide an implementation, allowing each platform to override specific predicates with full type safety. This is the more formal Go pattern for build-tag polymorphism. + +Why not chosen: none of the five helpers in question have any platform-specific behavior after the `isRepositoryImport` semantics choice is made. An interface with five methods, two concrete implementations that are byte-identical, and zero intentional divergence adds pure indirection with no benefit. It also increases the surface area for future drift by providing a slot where platform differences *could* be introduced even when they shouldn't be. + +#### Alternative 3: Remove the wasm build target + +Eliminate the wasm build entirely to remove the need for any build-tag split, resolving the drift problem permanently. + +Why not chosen: wasm is a key deployment target for browser-based gh-aw use cases. Removing it is not on the roadmap. + +### Consequences + +#### Positive +- Eliminates approximately 120 lines of duplicated code across three files and two build targets. +- Behavioral divergence between native and wasm for `isRepositoryImport` is fixed; dotted repository names like `githubnext/gh-aw.dev` are now accepted consistently. +- Constants (`WorkflowsDirSlash`, `AgentsDir`, `GithubDir`) are used in both build targets, so future constant changes propagate to both automatically. +- Follows an established codebase pattern (`github_token_env.go`) rather than introducing a new one. + +#### Negative +- The build-tag-free file is exercised by the native test suite only; wasm-specific behavior that relied on the old (more restrictive) `isRepositoryImport` predicate could surface as a behavior change in the wasm build without a dedicated wasm test run. +- `computeIncludeResolveAndSecurityBases` is now shared and called from both `remote_resolve_path.go` and `remote_fetch_wasm.go`; any future divergence between the two platforms' security-boundary logic must be handled at the call site rather than by forking the function. + +#### Neutral +- The three byte-identical `path#section` splitters (`splitImportPathAndSection`, `splitIncludePathAndSection`, `stripImportSection`) are replaced by a single `splitPathAndSection` helper, consistently using `strings.Cut` instead of `strings.SplitN`. +- `extractFrontmatterForTopologicalSort` in `import_topological.go` is replaced by a direct call to `extractFrontmatterForImport`, reducing the number of frontmatter extraction helpers from two to one. +- `isWorkflowSpec` (the wasm-only alias calling `IsWorkflowSpec`) is deleted; call sites now reference `IsWorkflowSpec` directly. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/parser/frontmatter_utils_test.go b/pkg/parser/frontmatter_utils_test.go index bdf035d4de1..58e8851f68d 100644 --- a/pkg/parser/frontmatter_utils_test.go +++ b/pkg/parser/frontmatter_utils_test.go @@ -670,6 +670,16 @@ func TestIsRepositoryImport(t *testing.T) { importPath: "my_org/my_repo", want: true, }, + { + name: "owner/repo with dot is repository import", + importPath: "githubnext/gh-aw.dev", + want: true, + }, + { + name: "repo with non workflow-adjacent extension-like suffix remains repository import", + importPath: "owner/tool.sh", + want: true, + }, { name: "workflowspec with three parts is not repository import", importPath: "owner/repo/path/to/file.md", @@ -715,6 +725,85 @@ func TestIsRepositoryImport(t *testing.T) { } } +func TestSplitPathAndSection(t *testing.T) { + path, section := splitPathAndSection("owner/repo/file.md#section") + assert.Equal(t, "owner/repo/file.md", path) + assert.Equal(t, "section", section) + + path, section = splitPathAndSection("owner/repo/file.md") + assert.Equal(t, "owner/repo/file.md", path) + assert.Empty(t, section) + + path, section = splitPathAndSection("owner/repo/file.md#") + assert.Equal(t, "owner/repo/file.md", path) + assert.Empty(t, section) +} + +func TestComputeIncludeResolveAndSecurityBases(t *testing.T) { + baseDir := filepath.Join(string(filepath.Separator), "repo", ".github", "workflows") + repoRoot := filepath.Dir(filepath.Dir(baseDir)) + githubDir := filepath.Join(repoRoot, ".github") + + tests := []struct { + name string + filePath string + baseDir string + wantResolveBase string + wantSecurityBase string + wantNormFilePath string + }{ + { + name: ".github-prefixed path resolves from repo root", + filePath: ".github/workflows/foo.md", + baseDir: baseDir, + wantResolveBase: repoRoot, + wantSecurityBase: githubDir, + wantNormFilePath: ".github/workflows/foo.md", + }, + { + name: "absolute path inside .github resolves from repo root", + filePath: "/.github/workflows/foo.md", + baseDir: baseDir, + wantResolveBase: repoRoot, + wantSecurityBase: githubDir, + wantNormFilePath: filepath.FromSlash(".github/workflows/foo.md"), + }, + { + name: "absolute path inside .agents uses agents security base", + filePath: "/.agents/custom.md", + baseDir: baseDir, + wantResolveBase: repoRoot, + wantSecurityBase: filepath.Join(repoRoot, ".agents"), + wantNormFilePath: filepath.FromSlash(".agents/custom.md"), + }, + { + name: "absolute path outside .github is rejected", + filePath: "/etc/passwd", + baseDir: baseDir, + wantResolveBase: "", + wantSecurityBase: "", + wantNormFilePath: "/etc/passwd", + }, + { + name: "base without .github ancestor keeps original base", + filePath: "shared/foo.md", + baseDir: filepath.Join(string(filepath.Separator), "repo", "workflows"), + wantResolveBase: filepath.Join(string(filepath.Separator), "repo", "workflows"), + wantSecurityBase: filepath.Join(string(filepath.Separator), "repo", "workflows"), + wantNormFilePath: "shared/foo.md", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotResolveBase, gotSecurityBase, gotNormFilePath := computeIncludeResolveAndSecurityBases(tt.filePath, tt.baseDir) + assert.Equal(t, tt.wantResolveBase, gotResolveBase) + assert.Equal(t, tt.wantSecurityBase, gotSecurityBase) + assert.Equal(t, tt.wantNormFilePath, gotNormFilePath) + }) + } +} + // processImportsFromFrontmatter is a test helper that wraps ProcessImportsFromFrontmatterWithSource // returning only the merged tools and engines (mirrors the removed production helper). func processImportsFromFrontmatter(frontmatter map[string]any, baseDir string) (string, []string, error) { @@ -725,6 +814,30 @@ func processImportsFromFrontmatter(frontmatter map[string]any, baseDir string) ( return result.MergedTools, result.MergedEngines, nil } +func TestTwoSegmentLocalImportWinsOverRepositoryImport(t *testing.T) { + tempDir := testutil.TempDir(t, "test-*") + configDir := filepath.Join(tempDir, "configs") + require.NoError(t, os.MkdirAll(configDir, 0755)) + + localImport := filepath.Join(configDir, "tool.toml") + localImportContent := `--- +tools: + local-tool: {} +--- +# Local config +` + require.NoError(t, os.WriteFile(localImport, []byte(localImportContent), 0644)) + + result, err := ProcessImportsFromFrontmatterWithSource(map[string]any{ + "imports": []string{"configs/tool.toml"}, + }, tempDir, nil, "", "") + + require.NoError(t, err) + assert.Empty(t, result.RepositoryImports, "existing two-segment local paths should not be classified as repository imports") + assert.Equal(t, []string{"configs/tool.toml"}, result.ImportedFiles) + assert.NotEmpty(t, result.MergedTools) +} + func TestProcessImportsFromFrontmatter(t *testing.T) { // Create temp directory for test files tempDir := testutil.TempDir(t, "test-*") diff --git a/pkg/parser/import_bfs.go b/pkg/parser/import_bfs.go index 209a2ef4f52..22cbab8fdd1 100644 --- a/pkg/parser/import_bfs.go +++ b/pkg/parser/import_bfs.go @@ -123,14 +123,14 @@ func seedInitialImportQueue(importSpecs []ImportSpec, baseDir string, cache *Imp func seedSingleImportSpec(importSpec ImportSpec, baseDir string, cache *ImportCache, workflowFilePath string, yamlContent string, state *importBFSState) error { importPath := importSpec.Path - if isRepositoryImport(importPath) { - parserLog.Printf("Detected repository import: %s", importPath) - state.acc.repositoryImports = append(state.acc.repositoryImports, importPath) - return nil - } - filePath, sectionName := splitImportPathAndSection(importPath) + filePath, sectionName := splitPathAndSection(importPath) fullPath, err := resolveSeedImportPath(filePath, importPath, baseDir, cache, workflowFilePath, yamlContent) if err != nil { + if isRepositoryImport(importPath) { + parserLog.Printf("Detected repository import: %s", importPath) + state.acc.repositoryImports = append(state.acc.repositoryImports, importPath) + return nil + } return err } origin, err := detectRemoteImportOrigin(filePath) @@ -140,14 +140,6 @@ func seedSingleImportSpec(importSpec ImportSpec, baseDir string, cache *ImportCa return enqueueImportPath(state, importPath, fullPath, sectionName, baseDir, importSpec.Inputs, origin) } -func splitImportPathAndSection(importPath string) (string, string) { - if strings.Contains(importPath, "#") { - parts := strings.SplitN(importPath, "#", 2) - return parts[0], parts[1] - } - return importPath, "" -} - func resolveSeedImportPath(filePath, importPath, baseDir string, cache *ImportCache, workflowFilePath string, yamlContent string) (string, error) { fullPath, err := ResolveIncludePath(filePath, baseDir, cache) if err != nil { @@ -399,7 +391,7 @@ func nestedEntriesFromSpecs(specs []ImportSpec) []nestedImportEntry { func enqueueNestedImportEntry(entry nestedImportEntry, item importQueueItem, baseDir string, cache *ImportCache, workflowFilePath string, yamlContent string, state *importBFSState) error { nestedImportPath := entry.path - nestedFilePath, nestedSectionName := splitImportPathAndSection(nestedImportPath) + nestedFilePath, nestedSectionName := splitPathAndSection(nestedImportPath) resolvedPath, nestedRemoteOrigin, err := resolveNestedImportPathAndOrigin(item, nestedFilePath) if err != nil { return err diff --git a/pkg/parser/import_topological.go b/pkg/parser/import_topological.go index 44c7d603e88..74d8a449715 100644 --- a/pkg/parser/import_topological.go +++ b/pkg/parser/import_topological.go @@ -7,7 +7,6 @@ import ( "errors" "slices" "sort" - "strings" "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/sliceutil" @@ -74,7 +73,7 @@ func buildImportDependencies(imports []string, baseDir string, cache *ImportCach } func resolveNestedImportPaths(importPath, baseDir string, cache *ImportCache) ([]string, error) { - filePath := stripImportSection(importPath) + filePath, _ := splitPathAndSection(importPath) fullPath, err := ResolveIncludePath(filePath, baseDir, cache) if err != nil { return nil, err @@ -83,35 +82,11 @@ func resolveNestedImportPaths(importPath, baseDir string, cache *ImportCache) ([ if err != nil { return nil, err } - frontmatter, err := extractFrontmatterForTopologicalSort(fullPath, content) + result, err := extractFrontmatterForImport(fullPath, content) if err != nil { return nil, err } - return extractImportPaths(frontmatter), nil -} - -func stripImportSection(importPath string) string { - if strings.Contains(importPath, "#") { - parts := strings.SplitN(importPath, "#", 2) - return parts[0] - } - return importPath -} - -func extractFrontmatterForTopologicalSort(fullPath string, content []byte) (map[string]any, error) { - var ( - result *FrontmatterResult - err error - ) - if strings.HasPrefix(fullPath, BuiltinPathPrefix) { - result, err = ExtractFrontmatterFromBuiltinFile(fullPath, content) - } else { - result, err = ExtractFrontmatterFromContent(string(content)) - } - if err != nil { - return nil, err - } - return result.Frontmatter, nil + return extractImportPaths(result.Frontmatter), nil } func calculateInDegree(imports []string, dependencies map[string][]string, allImportsSet map[string]struct { diff --git a/pkg/parser/include_processor.go b/pkg/parser/include_processor.go index d37cca508eb..bcd022ff782 100644 --- a/pkg/parser/include_processor.go +++ b/pkg/parser/include_processor.go @@ -132,7 +132,7 @@ func resolveDirectiveWithVisited( extractTools bool, visited map[string]struct { }) (includeDirectiveResolution, bool, error) { - filePath, sectionName := splitIncludePathAndSection(directive.Path) + filePath, sectionName := splitPathAndSection(directive.Path) fullPath, err := ResolveIncludePath(filePath, baseDir, nil) if err != nil { includeLog.Printf("Failed to resolve include path '%s': %v", filePath, err) @@ -160,14 +160,6 @@ func resolveDirectiveWithVisited( }, false, nil } -func splitIncludePathAndSection(includePath string) (string, string) { - if strings.Contains(includePath, "#") { - parts := strings.SplitN(includePath, "#", 2) - return parts[0], parts[1] - } - return includePath, "" -} - // processIncludedFile processes a single included file, optionally extracting a section // processIncludedFileWithVisited processes a single included file with cycle detection for nested includes func processIncludedFileWithVisited(filePath, sectionName string, extractTools bool, visited map[string]struct { diff --git a/pkg/parser/path_section.go b/pkg/parser/path_section.go new file mode 100644 index 00000000000..3e2c316318b --- /dev/null +++ b/pkg/parser/path_section.go @@ -0,0 +1,10 @@ +package parser + +import "strings" + +func splitPathAndSection(path string) (string, string) { + if before, after, ok := strings.Cut(path, "#"); ok { + return before, after + } + return path, "" +} diff --git a/pkg/parser/remote_fetch_wasm.go b/pkg/parser/remote_fetch_wasm.go index 9da2a015940..381ebbc31c3 100644 --- a/pkg/parser/remote_fetch_wasm.go +++ b/pkg/parser/remote_fetch_wasm.go @@ -8,53 +8,6 @@ import ( "strings" ) -func isUnderWorkflowsDirectory(filePath string) bool { - normalizedPath := filepath.ToSlash(filePath) - if !strings.Contains(normalizedPath, ".github/workflows/") { - return false - } - parts := strings.Split(normalizedPath, ".github/workflows/") - if len(parts) < 2 { - return false - } - return !strings.Contains(parts[1], "/") -} - -func isCustomAgentFile(filePath string) bool { - normalizedPath := filepath.ToSlash(filePath) - return strings.Contains(normalizedPath, ".github/agents/") && strings.HasSuffix(strings.ToLower(normalizedPath), ".md") -} - -func isRepositoryImport(importPath string) bool { - cleanPath := importPath - if idx := strings.Index(importPath, "#"); idx != -1 { - cleanPath = importPath[:idx] - } - pathWithoutRef := cleanPath - if idx := strings.Index(cleanPath, "@"); idx != -1 { - pathWithoutRef = cleanPath[:idx] - } - parts := strings.Split(pathWithoutRef, "/") - if len(parts) != 2 { - return false - } - if strings.HasPrefix(pathWithoutRef, ".") || strings.HasPrefix(pathWithoutRef, "/") { - return false - } - if strings.HasPrefix(pathWithoutRef, "shared/") { - return false - } - owner := parts[0] - repo := parts[1] - if owner == "" || repo == "" { - return false - } - if strings.Contains(repo, ".") { - return false - } - return true -} - func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, error) { parserLog.Printf("ResolveIncludePath: filePath=%s, baseDir=%s", filePath, baseDir) @@ -66,45 +19,16 @@ func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, e return filePath, nil } - if isWorkflowSpec(filePath) { + if IsWorkflowSpec(filePath) { parserLog.Printf("ResolveIncludePath: rejecting remote workflowspec in Wasm build: %s", filePath) return "", fmt.Errorf("remote imports not available in Wasm: %s", filePath) } - githubFolder := baseDir - for !strings.HasSuffix(githubFolder, ".github") { - parent := filepath.Dir(githubFolder) - if parent == githubFolder || parent == "." || parent == "/" { - githubFolder = baseDir - break - } - githubFolder = parent - } - - resolveBase := baseDir - securityBase := githubFolder - if strings.HasSuffix(githubFolder, ".github") { - repoRoot := filepath.Dir(githubFolder) - filePathSlash := filepath.ToSlash(filePath) - if strings.HasPrefix(filePathSlash, ".github/") { - resolveBase = repoRoot - } else if stripped, ok := strings.CutPrefix(filePathSlash, "/"); ok { - // Repo-root-absolute path: only .github/ and .agents/ subdirectories are accessible. - if !strings.HasPrefix(stripped, ".github/") && !strings.HasPrefix(stripped, ".agents/") { - return "", fmt.Errorf("security: path %s must be within .github or .agents folder", filePath) - } - filePath = filepath.FromSlash(stripped) - resolveBase = repoRoot - if strings.HasPrefix(stripped, ".agents/") { - securityBase = filepath.Join(repoRoot, ".agents") - } else { - // .github/-prefixed: security scope is the .github folder. - securityBase = githubFolder - } - } + resolveBase, securityBase, normalizedFilePath := computeIncludeResolveAndSecurityBases(filePath, baseDir) + if resolveBase == "" { + return "", fmt.Errorf("security: path %s must be within .github or .agents folder", normalizedFilePath) } - - fullPath := filepath.Join(resolveBase, filePath) + fullPath := filepath.Join(resolveBase, normalizedFilePath) normalizedSecurityBase := filepath.Clean(securityBase) normalizedFullPath := filepath.Clean(fullPath) @@ -112,8 +36,8 @@ func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, e relativePath, err := filepath.Rel(normalizedSecurityBase, normalizedFullPath) if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { allowedFolder := filepath.Base(normalizedSecurityBase) - parserLog.Printf("ResolveIncludePath: security boundary violation: path=%s, allowedFolder=%s", filePath, allowedFolder) - return "", fmt.Errorf("security: path %s must be within %s folder (resolves to: %s)", filePath, allowedFolder, relativePath) + parserLog.Printf("ResolveIncludePath: security boundary violation: path=%s, allowedFolder=%s", normalizedFilePath, allowedFolder) + return "", fmt.Errorf("security: path %s must be within %s folder (resolves to: %s)", normalizedFilePath, allowedFolder, relativePath) } // In wasm builds, check the virtual filesystem first @@ -124,44 +48,3 @@ func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, e parserLog.Printf("ResolveIncludePath: file not found in virtual filesystem: %s", fullPath) return "", fmt.Errorf("file not found: %s", fullPath) } - -// IsWorkflowSpec checks if a path looks like a workflowspec (owner/repo/path[@ref]). -func IsWorkflowSpec(path string) bool { - cleanPath := path - if idx := strings.Index(path, "#"); idx != -1 { - cleanPath = path[:idx] - } - if idx := strings.Index(cleanPath, "@"); idx != -1 { - cleanPath = cleanPath[:idx] - } - parts := strings.Split(cleanPath, "/") - if len(parts) < 3 { - return false - } - // Preserve legacy behavior expected by parser tests: URL-like paths are - // currently treated as workflowspecs because downstream parsing supports - // repository/path extraction from slash-delimited remote references. - if strings.Contains(cleanPath, "://") { - return true - } - if strings.HasPrefix(cleanPath, ".") { - return false - } - if strings.HasPrefix(cleanPath, "shared/") { - return false - } - if strings.HasPrefix(cleanPath, "/") { - return false - } - // Safe indexing: len(parts) >= 3 is guaranteed above. - owner := parts[0] - repo := parts[1] - if owner == "" || repo == "" { - return false - } - return true -} - -func isWorkflowSpec(path string) bool { - return IsWorkflowSpec(path) -} diff --git a/pkg/parser/remote_path_predicates.go b/pkg/parser/remote_path_predicates.go new file mode 100644 index 00000000000..5b27d0a87d6 --- /dev/null +++ b/pkg/parser/remote_path_predicates.go @@ -0,0 +1,142 @@ +package parser + +import ( + "path/filepath" + "strings" + + "github.com/github/gh-aw/pkg/constants" +) + +// isUnderWorkflowsDirectory checks if a file path is a top-level workflow file (not in shared subdirectory) +func isUnderWorkflowsDirectory(filePath string) bool { + normalizedPath := filepath.ToSlash(filePath) + if !strings.Contains(normalizedPath, constants.WorkflowsDirSlash) { + return false + } + parts := strings.Split(normalizedPath, constants.WorkflowsDirSlash) + if len(parts) < 2 { + return false + } + return !strings.Contains(parts[1], "/") +} + +// isCustomAgentFile checks if a file path is a custom agent file under .github/agents/ +// Custom agent files use GitHub Copilot's agent format, which differs from gh-aw workflow format. +// These files have a different schema for the 'tools' field (array vs object). +func isCustomAgentFile(filePath string) bool { + normalizedPath := filepath.ToSlash(filePath) + return strings.Contains(normalizedPath, constants.AgentsDir) && strings.HasSuffix(strings.ToLower(normalizedPath), ".md") +} + +// isRepositoryImport checks if an import spec is a repository-only import (no file path) +// Format: owner/repo@ref or owner/repo (downloads entire .github folder, no agent extraction) +// Only common workflow-adjacent file extensions are rejected so dotted repository +// names such as "githubnext/gh-aw.dev" remain valid repository imports. +// Callers that also accept local imports should attempt local path resolution +// first so existing two-segment local paths win over this remote-import heuristic. +func isRepositoryImport(importPath string) bool { + cleanPath := importPath + if before, _, ok := strings.Cut(importPath, "#"); ok { + cleanPath = before + } + pathWithoutRef := cleanPath + if before, _, ok := strings.Cut(cleanPath, "@"); ok { + pathWithoutRef = before + } + parts := strings.Split(pathWithoutRef, "/") + if len(parts) != 2 { + return false + } + if strings.HasPrefix(pathWithoutRef, ".") || strings.HasPrefix(pathWithoutRef, "/") { + return false + } + if strings.HasPrefix(pathWithoutRef, "shared/") { + return false + } + owner := parts[0] + repo := parts[1] + if owner == "" || repo == "" { + return false + } + for _, ext := range []string{".md", ".yaml", ".yml", ".json"} { + if strings.HasSuffix(strings.ToLower(repo), ext) { + return false + } + } + return true +} + +// IsWorkflowSpec checks if a path looks like a workflowspec (owner/repo/path[@ref]). +func IsWorkflowSpec(path string) bool { + cleanPath := path + if before, _, ok := strings.Cut(path, "#"); ok { + cleanPath = before + } + if before, _, ok := strings.Cut(cleanPath, "@"); ok { + cleanPath = before + } + parts := strings.Split(cleanPath, "/") + if len(parts) < 3 { + return false + } + // Preserve legacy behavior expected by parser tests: URL-like paths are + // currently treated as workflowspecs because downstream parsing supports + // repository/path extraction from slash-delimited remote references. + if strings.Contains(cleanPath, "://") { + return true + } + if strings.HasPrefix(cleanPath, ".") { + return false + } + if strings.HasPrefix(cleanPath, "shared/") { + return false + } + if strings.HasPrefix(cleanPath, "/") { + return false + } + owner := parts[0] + repo := parts[1] + if owner == "" || repo == "" { + return false + } + return true +} + +func findGitHubFolder(baseDir string) string { + githubFolder := baseDir + for !strings.HasSuffix(githubFolder, ".github") { + parent := filepath.Dir(githubFolder) + if parent == githubFolder || parent == "." || parent == "/" { + githubFolder = baseDir + break + } + githubFolder = parent + } + return githubFolder +} + +func computeIncludeResolveAndSecurityBases(filePath, baseDir string) (string, string, string) { + githubFolder := findGitHubFolder(baseDir) + resolveBase := baseDir + securityBase := githubFolder + normalizedFilePath := filePath + if strings.HasSuffix(githubFolder, ".github") { + repoRoot := filepath.Dir(githubFolder) + filePathSlash := filepath.ToSlash(filePath) + if strings.HasPrefix(filePathSlash, constants.GithubDir) { + resolveBase = repoRoot + } else if stripped, ok := strings.CutPrefix(filePathSlash, "/"); ok { + if !strings.HasPrefix(stripped, constants.GithubDir) && !strings.HasPrefix(stripped, ".agents/") { + return "", "", filePath + } + normalizedFilePath = filepath.FromSlash(stripped) + resolveBase = repoRoot + if strings.HasPrefix(stripped, ".agents/") { + securityBase = filepath.Join(repoRoot, ".agents") + } else { + securityBase = githubFolder + } + } + } + return resolveBase, securityBase, normalizedFilePath +} diff --git a/pkg/parser/remote_resolve_path.go b/pkg/parser/remote_resolve_path.go index 4a20d813568..11b8bbe0fd8 100644 --- a/pkg/parser/remote_resolve_path.go +++ b/pkg/parser/remote_resolve_path.go @@ -11,93 +11,6 @@ import ( "github.com/github/gh-aw/pkg/constants" ) -// isUnderWorkflowsDirectory checks if a file path is a top-level workflow file (not in shared subdirectory) -func isUnderWorkflowsDirectory(filePath string) bool { - // Normalize the path to use forward slashes - normalizedPath := filepath.ToSlash(filePath) - - // Check if the path contains .github/workflows/ - if !strings.Contains(normalizedPath, constants.WorkflowsDirSlash) { - return false - } - - // Extract the part after .github/workflows/ - parts := strings.Split(normalizedPath, constants.WorkflowsDirSlash) - if len(parts) < 2 { - return false - } - - afterWorkflows := parts[1] - - // Check if there are any slashes after .github/workflows/ (indicating subdirectory) - // If there are, it's in a subdirectory like "shared/" and should not be treated as a workflow file - return !strings.Contains(afterWorkflows, "/") -} - -// isCustomAgentFile checks if a file path is a custom agent file under .github/agents/ -// Custom agent files use GitHub Copilot's agent format, which differs from gh-aw workflow format. -// These files have a different schema for the 'tools' field (array vs object). -func isCustomAgentFile(filePath string) bool { - // Normalize the path to use forward slashes - normalizedPath := filepath.ToSlash(filePath) - - // Check if the path contains .github/agents/ and ends with .md - return strings.Contains(normalizedPath, constants.AgentsDir) && strings.HasSuffix(strings.ToLower(normalizedPath), ".md") -} - -// isRepositoryImport checks if an import spec is a repository-only import (no file path) -// Format: owner/repo@ref or owner/repo (downloads entire .github folder, no agent extraction) -func isRepositoryImport(importPath string) bool { - // Remove section reference if present - cleanPath := importPath - if before, _, ok := strings.Cut(importPath, "#"); ok { - cleanPath = before - } - - // Remove ref if present to check the path structure - pathWithoutRef := cleanPath - if before, _, ok := strings.Cut(cleanPath, "@"); ok { - pathWithoutRef = before - } - - // Split by slash to count parts - parts := strings.Split(pathWithoutRef, "/") - - // Repository import has exactly 2 parts: owner/repo - // File imports have 1 part (local file) or 3+ parts (owner/repo/path/to/file) - if len(parts) != 2 { - return false - } - - // Reject local paths - if strings.HasPrefix(pathWithoutRef, ".") || strings.HasPrefix(pathWithoutRef, "/") { - return false - } - - // Reject paths that start with common local directory names - if strings.HasPrefix(pathWithoutRef, "shared/") { - return false - } - - // Additional validation: check if it looks like a valid owner/repo format. - owner := parts[0] - repo := parts[1] - - // Basic validation - ensure they're not empty and don't look like file extensions - if owner == "" || repo == "" { - return false - } - - // Reject if repo part looks like a file path with a known workflow/data extension. - for _, ext := range []string{".md", ".yaml", ".yml", ".json"} { - if strings.HasSuffix(strings.ToLower(repo), ext) { - return false - } - } - - return true -} - // ResolveIncludePath resolves include path based on workflowspec format or relative path func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, error) { remoteLog.Printf("Resolving include path: file_path=%s, base_dir=%s", filePath, baseDir) @@ -127,45 +40,6 @@ func resolveBuiltinIncludePath(filePath string) (string, bool, error) { return filePath, true, nil } -func findGitHubFolder(baseDir string) string { - githubFolder := baseDir - for !strings.HasSuffix(githubFolder, ".github") { - parent := filepath.Dir(githubFolder) - if parent == githubFolder || parent == "." || parent == "/" { - githubFolder = baseDir - break - } - githubFolder = parent - } - return githubFolder -} - -func computeIncludeResolveAndSecurityBases(filePath, baseDir string) (string, string, string) { - githubFolder := findGitHubFolder(baseDir) - resolveBase := baseDir - securityBase := githubFolder - normalizedFilePath := filePath - if strings.HasSuffix(githubFolder, ".github") { - repoRoot := filepath.Dir(githubFolder) - filePathSlash := filepath.ToSlash(filePath) - if strings.HasPrefix(filePathSlash, constants.GithubDir) { - resolveBase = repoRoot - } else if stripped, ok := strings.CutPrefix(filePathSlash, "/"); ok { - if !strings.HasPrefix(stripped, constants.GithubDir) && !strings.HasPrefix(stripped, ".agents/") { - return "", "", filePath - } - normalizedFilePath = filepath.FromSlash(stripped) - resolveBase = repoRoot - if strings.HasPrefix(stripped, ".agents/") { - securityBase = filepath.Join(repoRoot, ".agents") - } else { - securityBase = githubFolder - } - } - } - return resolveBase, securityBase, normalizedFilePath -} - func resolveAndValidateLocalIncludePath(filePath, resolveBase, securityBase string) (string, error) { if stripped, ok := strings.CutPrefix(filepath.ToSlash(filePath), "/"); ok { if !strings.HasPrefix(stripped, constants.GithubDir) && !strings.HasPrefix(stripped, ".agents/") { diff --git a/pkg/parser/remote_workflow_spec.go b/pkg/parser/remote_workflow_spec.go index bfd026c4b60..89f387c6016 100644 --- a/pkg/parser/remote_workflow_spec.go +++ b/pkg/parser/remote_workflow_spec.go @@ -12,57 +12,6 @@ import ( "github.com/github/gh-aw/pkg/gitutil" ) -// IsWorkflowSpec checks if a path looks like a workflowspec (owner/repo/path[@ref]). -func IsWorkflowSpec(path string) bool { - // Remove section reference if present - cleanPath := path - if before, _, ok := strings.Cut(path, "#"); ok { - cleanPath = before - } - - // Remove ref if present - if idx := strings.Index(cleanPath, "@"); idx != -1 { - cleanPath = cleanPath[:idx] - } - - // Check if it has at least 3 parts (owner/repo/path) - parts := strings.Split(cleanPath, "/") - if len(parts) < 3 { - return false - } - - // Preserve legacy behavior expected by parser tests: URL-like paths are - // currently treated as workflowspecs because downstream parsing supports - // repository/path extraction from slash-delimited remote references. - if strings.Contains(cleanPath, "://") { - return true - } - - // Reject paths that start with "." (local paths like .github/workflows/...) - if strings.HasPrefix(cleanPath, ".") { - return false - } - - // Reject paths that start with "shared/" (local shared files) - if strings.HasPrefix(cleanPath, "shared/") { - return false - } - - // Reject absolute paths - if strings.HasPrefix(cleanPath, "/") { - return false - } - - // Safe indexing: len(parts) >= 3 is guaranteed above. - owner := parts[0] - repo := parts[1] - if owner == "" || repo == "" { - return false - } - - return true -} - // downloadIncludeFromWorkflowSpec downloads an include file from GitHub using workflowspec. // It first checks the cache, and only downloads if not cached. //