Refactor parser import/path helpers to eliminate wasm/native drift and duplicate logic

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.*

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-*")

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

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 {

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 {

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, ""
+}