Skip to content

[refactor] pkg/parser: wasm build-tag copies have drifted, plus 3 duplicate-function clustersΒ #53888

Description

@github-actions

πŸ”§ Semantic Function Clustering Analysis

Scope: pkg/parser (53 non-test files) and pkg/modelsdev (1 file) β€” the precomputed package slice for this run.

Executive Summary

Cataloged ~580 non-test functions across 54 files. Overall organization is good: the schedule_*, schema_*, include_*, and import_* file clusters each have a clear single purpose, and no meaningful "function in the wrong file" outliers were found. pkg/modelsdev/catalog.go (2 functions) is clean.

Four concrete duplication findings survived verification. One of them (#1) is not just cosmetic β€” the two copies have actually drifted apart in behavior.

# Finding Severity
1 Wasm/non-wasm copies of remote path resolution have drifted High β€” behavioral divergence
2 splitImportPathAndSection / splitIncludePathAndSection byte-identical Medium
3 extractFrontmatterForTopologicalSort duplicates extractFrontmatterForImport Medium
4 Four near-identical "list with API→git→public-API fallback" chains Medium

1. Wasm build-tag copies have drifted (High)

pkg/parser/remote_fetch_wasm.go ((go/redacted):build js || wasm) hand-duplicates four functions that also exist in the (go/redacted):build !js && !wasm files remote_resolve_path.go and remote_workflow_spec.go. These are not thin platform shims β€” they are copy-pasted logic, and the copies no longer agree.

Confirmed divergence in isRepositoryImport:

// remote_resolve_path.go:92 (native) β€” rejects only known data/workflow extensions
for _, ext := range []string{".md", ".yaml", ".yml", ".json"} {
    if strings.HasSuffix(strings.ToLower(repo), ext) {
        return false
    }
}

// remote_fetch_wasm.go:52 (wasm) β€” rejects ANY dot in the repo name
if strings.Contains(repo, ".") {
    return false
}

The wasm predicate is strictly more restrictive. An import spec like githubnext/gh-aw.dev resolves as a repository import on native builds and is silently rejected on wasm. Any repo whose name legitimately contains a dot behaves differently depending on build target.

Secondary drift β€” constants vs. hardcoded literals:

remote_resolve_path.go uses constants.WorkflowsDirSlash, constants.AgentsDir, and constants.GithubDir. The wasm copy hardcodes ".github/workflows/", ".github/agents/", and ".github/". Changing a constant updates only one of the two code paths.

IsWorkflowSpec is duplicated near-verbatim (~50 lines) at remote_workflow_spec.go:16 and remote_fetch_wasm.go:129, including the identical "Preserve legacy behavior expected by parser tests" comment. The wasm file also adds a redundant one-line alias isWorkflowSpec (remote_fetch_wasm.go:165) that exists only to call IsWorkflowSpec.

The repo already has the right pattern for this. GetGitHubToken is split across github.go (native) and github_wasm.go (wasm), but the shared env-var logic lives in the build-tag-free github_token_env.go and both call into it. github_wasm.go is 17 lines because of it.

Recommendation: apply the same treatment. Move the platform-independent predicates β€” isUnderWorkflowsDirectory, isCustomAgentFile, isRepositoryImport, IsWorkflowSpec β€” plus the pure path arithmetic in findGitHubFolder and computeIncludeResolveAndSecurityBases into a build-tag-free file (e.g. remote_path_predicates.go). Only ResolveIncludePath's final filesystem probe genuinely differs (os.Stat vs VirtualFileExists). Decide deliberately which isRepositoryImport behavior is correct rather than leaving both. Delete the isWorkflowSpec alias.

Impact: removes ~120 duplicated lines and eliminates a class of silent native/wasm behavior differences.


2. Byte-identical path/section splitters (Medium)

Three functions in the same package implement the same path#section split:

Function Location Callers
splitImportPathAndSection pkg/parser/import_bfs.go:143 import_bfs.go:131, import_bfs.go:402
splitIncludePathAndSection pkg/parser/include_processor.go:163 include_processor.go:135
stripImportSection pkg/parser/import_topological.go:93 import_topological.go:77

The first two are identical apart from the parameter name:

func splitImportPathAndSection(importPath string) (string, string) {
	if strings.Contains(importPath, "#") {
		parts := strings.SplitN(importPath, "#", 2)
		return parts[0], parts[1]
	}
	return importPath, ""
}

stripImportSection is the same logic discarding the second return value.

Recommendation: keep one exported-within-package helper (splitPathAndSection); replace stripImportSection's single call site with path, _ := splitPathAndSection(...). All five call sites are in pkg/parser, so this is a contained change.


3. Duplicate frontmatter extraction for builtin paths (Medium)

// pkg/parser/import_bfs.go:367
func extractFrontmatterForImport(fullPath string, content []byte) (*FrontmatterResult, error) {
	if strings.HasPrefix(fullPath, BuiltinPathPrefix) {
		return ExtractFrontmatterFromBuiltinFile(fullPath, content)
	}
	return ExtractFrontmatterFromContent(string(content))
}

// pkg/parser/import_topological.go:101
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
}

Identical branch logic; the second only unwraps .Frontmatter. Both are unexported in the same package.

Recommendation: delete extractFrontmatterForTopologicalSort and have its one caller (import_topological.go) call extractFrontmatterForImport and read .Frontmatter.


4. Four repeated list-with-fallback chains (Medium)

pkg/parser/remote_list_files.go β€” 12 functions, ~460 lines, one shape repeated four times

Each of four listing operations is implemented as the same three-function chain: authenticated Contents API β†’ git-clone fallback on auth error β†’ unauthenticated public API fallback (github.com only).

Operation Primary Git fallback Public-API fallback
Workflow .md files listWorkflowFilesForHost:179 listWorkflowFilesViaGitForHost:231 listWorkflowFilesViaPublicAPI:272
All dir files listDirAllFilesForHost:305 listDirAllFilesViaGitForHost:350 listDirAllFilesViaPublicAPI:386
Subdirectories listDirSubdirsForHost:525 listDirSubdirsViaGitForHost:570 listDirSubdirsViaPublicAPI:606
Recursive files listDirAllFilesRecursivelyForHost:418 listDirAllFilesRecursivelyViaGitForHost:487 (intentionally none β€” rate limits)

The three complete chains differ only in:

  • the entry filter β€” Type == "file" && .md / Type == "file" / Type == "dir"
  • the git ls-tree flags β€” -r vs -d
  • log and error message wording

The anonymous response struct {Name, Path, Type string} with identical JSON tags is redeclared six times in this file (lines 189, 279, 314, 393, 534, 613). The auth-error-then-git-then-public-API control flow is reproduced verbatim three times (lines 198–217, 322–337, 542–557).

Note this file is also where the anonymous-struct redeclaration causes a real inconsistency: listDirAllFilesViaPublicAPI:393 declares only {Path, Type} while its five siblings declare {Name, Path, Type} β€” harmless today, but exactly the kind of drift copy-paste produces.

Recommendation: extract one generic helper parameterized by a predicate and the ls-tree mode, e.g.

type contentEntry struct {
	Name string `json:"name"`
	Path string `json:"path"`
	Type string `json:"type"`
}

func listRemoteEntries(ctx context.Context, owner, repo, ref, dirPath, host string,
	keep func(contentEntry) bool, gitMode lsTreeMode, label string) ([]string, error)

The four public entry points become thin wrappers. This is the largest single line-count reduction available in the slice (~250 lines), but it touches the auth-fallback path, so it should land with the existing remote_fetch_test.go fallback tests green.


Minor observation (not recommended for action)

findGitHubRepoRoot (include_expander.go:122) and findGitHubFolder (remote_resolve_path.go:130) both walk ancestors looking for .github, but return different things (parent-of-.github vs. the folder itself) and have different not-found behavior ("" vs. fall back to baseDir). Genuinely distinct contracts β€” flagged only so a future consolidation pass does not mistake them for duplicates.


Suggested order of work

Analysis metadata
  • Files in scope: 54 (53 in pkg/parser, 1 in pkg/modelsdev); _test.go excluded
  • Non-test functions cataloged: ~580
  • Clusters examined: schema_*, import_*, remote_*, schedule_*, include_* / inline_*, frontmatter_*, github*, virtual_fs*
  • Outliers (wrong-file functions) found: 0
  • Duplicate groups confirmed: 4
  • Method: symbol inventory per file, naming-pattern clustering, then side-by-side source comparison of every candidate pair. All four findings were verified by reading both implementations; none are inferred from names alone.

Generated by πŸ”§ Semantic Function Refactoring Β· sonnet46 Β· 222.8 AIC Β· βŒ– 21.3 AIC Β· ⊞ 10.1K Β· β—·

  • expires on Aug 20, 2026, 6:48 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions