[refactor] errorutil duplicated in 5 pkg/cli sites (one with a case-sensitivity bug); fileutil is clean

[github-actions]

🔧 Semantic Function Clustering Analysis — pkg/errorutil, pkg/fileutil

Analysis of the precomputed package slice for this run: 2 packages, 4 non-test Go files, 15 functions.

Headline: pkg/fileutil is clean — no outliers, no duplicates anywhere in the repo. pkg/errorutil is the canonical not-found/forbidden/gone classifier, but 5 call sites in pkg/cli reimplement its logic inline instead of calling it. One of those reimplementations has a latent case-sensitivity bug.

---

Key findings

#	Finding	Severity	Locations
1	isRepositoryPackageRemoteNotFound is a functional duplicate of errorutil.IsNotFoundError	High	pkg/cli/add_package_manifest.go:925
2	Inline "404" || "not found" substring checks bypassing errorutil	High	4 sites in pkg/cli
3	Case-sensitive check misses gh's Not Found output	High (latent bug)	pkg/cli/experiments_command.go:718
4	Missing string-input variant forces errors.New(...) wrapper idiom	Medium	2 sites in pkg/cli
5	fileutil.go mixes path-security validation with file/dir operations	Low	pkg/fileutil/fileutil.go

---

1. Exact functional duplicate of errorutil.IsNotFoundError

pkg/cli/add_package_manifest.go:925-931 reimplements the canonical helper verbatim:

// pkg/cli/add_package_manifest.go
func isRepositoryPackageRemoteNotFound(err error) bool {
	if err == nil {
		return false
	}
	errText := strings.ToLower(err.Error())
	return strings.Contains(errText, "404") || strings.Contains(errText, "not found")
}

// pkg/errorutil/errors.go:18 — identical semantics
func IsNotFoundError(err error) bool {
	matched := containsErrorSubstring(err, "404", "not found")
	...
}

Same nil guard, same lowercasing, same two substrings. Recommendation: delete isRepositoryPackageRemoteNotFound and call errorutil.IsNotFoundError from normalizeRepositoryPackageRemoteError (add_package_manifest.go:918). This also gains the debug logging errorutil already emits.

2 & 3. Scattered inline 404 / not found checks

errorutil is already the established shared abstraction (~10 call sites across pkg/cli and pkg/parser), yet these four sites hand-roll the same predicate:

All four inline reimplementations

// pkg/cli/branch_file_reader.go:59 — string input, otherwise identical
func isRemoteFileNotFoundOutput(output string) bool {
	s := strings.ToLower(output)
	return strings.Contains(s, "404") || strings.Contains(s, "not found")
}

// pkg/cli/checks_command.go:243 — inside classifyGHAPIError
lower := strings.ToLower(stderr)
switch {
case strings.Contains(lower, "404") || strings.Contains(lower, "not found"):

// pkg/cli/setup_repository.go:108 — same pair plus two extra clauses
if strings.Contains(message, "could not resolve to a repository") ||
	strings.Contains(message, "http 404") || strings.Contains(message, "not found") {

// pkg/cli/experiments_command.go:718 — NOT lowercased
if strings.Contains(stderr, "404") || strings.Contains(stderr, "not found") {

experiments_command.go:718 is a real bug, not just duplication. stderr is used raw with no strings.ToLower. The GitHub API and gh CLI return Not Found (title case) in most 404 responses, so this branch does not fire and the user gets the generic failed to fetch experiment branch (exit N): ... message instead of experiment %q not found in %s. Every other site in the codebase lowercases first — this one was missed. Routing it through errorutil fixes the bug and removes the duplicate in one change.

4. Missing string-input variant in the errorutil public API

errorutil only accepts error, so callers holding raw CLI output resort to wrapping:

// pkg/cli/logs_download.go:104
if errorutil.IsNotFoundError(err) || errorutil.IsNotFoundError(errors.New(string(output))) || errorutil.IsGoneError(err) {

// pkg/cli/workflow_run_metadata.go:56
errorutil.IsNotFoundError(errors.New(outputStr)) ||

Allocating a throwaway error purely to satisfy the signature is a smell, and it is plausibly why branch_file_reader.go and checks_command.go wrote their own string versions instead. Recommendation: add errorutil.IsNotFoundOutput(s string) bool (and optionally IsForbiddenOutput/IsGoneOutput) delegating to the existing unexported containsErrorSubstring logic. That gives all the string-shaped call sites a single home and removes the errors.New idiom.

Consolidating findings 1–4 collapses 5 duplicate predicates plus 2 wrapper idioms into 2 exported functions, and fixes one user-visible bug.

5. fileutil.go mixes two concerns (low priority)

pkg/fileutil/fileutil.go holds two distinct clusters: path-security validation (ValidateAbsolutePath, ValidatePathWithinBase, resolveWithAncestorSymlinks — lines 39-140) and plain file/dir operations (EnsureParentDir, FileExists, DirExists, IsDirEmpty, copyFileContents, CopyFile — lines 142-235). The security half is the part that warrants careful review and carries the most test coverage; splitting it into path_validation.go would make that boundary explicit, matching how executable.go and tar.go are already scoped. Purely organizational — no behavior change, and reasonable to skip.

---

What is already good

Clean results — no action needed

• pkg/fileutil has zero duplicates repo-wide. FileExists, DirExists, IsDirEmpty, CopyFile, EnsureParentDir, ValidateAbsolutePath, ValidatePathWithinBase, and ExtractFileFromTar are each defined exactly once. For utility functions this generic, that is unusual and worth preserving.
• File-per-feature is respected. executable.go holds only the two executable-resolution functions; tar.go holds only ExtractFileFromTar. No outliers — every function matches its file's stated purpose.
• errorutil internals are well factored. containsHTTPStatusSubstring narrows 403/410 to HTTP-shaped patterns so "forbidden character" and "connection has gone away" are not misclassified, while 404 intentionally stays broad. The asymmetry is deliberate and documented in the function comments.
• Consistent logging. Every file declares its own scoped logger.New("pkg:file") instance.

Suggested order of work

1. Fix experiments_command.go:718 case sensitivity — standalone bug fix, worth landing on its own.
2. Add errorutil.IsNotFoundOutput(string).
3. Replace the duplicate predicates with errorutil calls; delete isRepositoryPackageRemoteNotFound and isRemoteFileNotFoundOutput.
4. (Optional) Split path_validation.go out of fileutil.go.

Analysis metadata

• Scope: pkg/errorutil, pkg/fileutil (precomputed slice for this run; not a repo-wide sweep)
• Files analyzed: 4 non-test .go files
• Functions cataloged: 15 (8 exported, 7 unexported)
• Outliers found: 0
• Duplicates confirmed: 5 (all reimplementations of in-scope errorutil symbols, located in pkg/cli)
• Method: symbol inventory of the in-scope slice, then reference and pattern search anchored on those symbols to find reimplementations elsewhere
• Note: finding 3 was verified by reading the surrounding code path, not inferred from the pattern match alone

Generated by 🔧 Semantic Function Refactoring · sonnet46 · 214.3 AIC · ⌖ 18.5 AIC · ⊞ 9.6K · ◷

• expires on Aug 15, 2026, 7:17 PM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I identified this as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 2.84 AIC · ⌖ 10.3 AIC · ⊞ 12.5K · ◷