Skip to content

Export and adopt a shared NotFound-error helper across pkg/cli (sergo) #32752

@github-actions

Description

@github-actions

Summary

A isNotFoundError(errMsg string) bool helper exists at pkg/parser/remote_fetch.go:610 and centralizes the gh/HTTP 404 detection pattern (404 OR not found, case-insensitive). However, it lives in pkg/parser as unexported, so several pkg/cli call sites reimplement the same substring matching inline. The duplicate sites have already drifted: each picks a different subset of the canonical signals.

Existing helper (unexported, wrong package)

// pkg/parser/remote_fetch.go:610
func isNotFoundError(errMsg string) bool {
	lowerMsg := strings.ToLower(errMsg)
	return strings.Contains(lowerMsg, "404") || strings.Contains(lowerMsg, "not found")
}

Duplicate call sites in pkg/cli

File:Line Current code Signals matched
pkg/cli/audit.go:1037-1042 5 separate strings.Contains calls ("Not Found", "404", "not found", "Could not resolve", err.Error() "404") Superset — could feed back into helper
pkg/cli/outcome_eval_comment.go:34 strings.Contains(err.Error(), "404") || strings.Contains(err.Error(), "Not Found") Case-sensitive, misses lowercase not found
pkg/cli/gateway_logs_mcp.go:28 strings.Contains(err.Error(), "not found") Misses 404 literal
pkg/cli/logs_download.go:316 strings.Contains(string(output), "not found") || strings.Contains(err.Error(), "410") Mixes 404+410, lowercase only

Why

  • Helper-underadoption failure mode, recurring theme across Sergo Runs 8–11 (HTTP timeout, file mode constants, GetWorkflowDir, etc.).
  • The duplicates already disagree on what counts as "not found":
    • outcome_eval_comment.go:34 is case-sensitive, while remote_fetch.go:610 lower-cases first.
    • audit.go:1037 adds "Could not resolve" (DNS-failure phrasing from git clone).
    • gateway_logs_mcp.go:28 skips the bare "404" substring entirely.
  • Single source of truth makes evolution (e.g. matching on *ghapi.HTTPError once gh CLI / go-gh exposes structured errors) a one-line change.

Suggested fix

  1. Move the helper to a shared location — options:
    • Export it: parser.IsNotFoundError(err error) bool (still in pkg/parser).
    • Or create pkg/ghapi/errors.go (or pkg/errorutil) and put both IsNotFoundError and an exported sibling of isPermissionError (#aw_sgcr1) there. Recommended — avoids pkg/cli and pkg/parser importing each other.
  2. Change the signature to func IsNotFoundError(err error) bool (and ignore nil err). The current string parameter forces callers to call err.Error() and loses wrapping.
  3. Replace the four duplicate sites to call it.
  4. Make the helper case-insensitive (lower-case once) — the canonical implementation in remote_fetch.go:610 already does this; the divergent outcome_eval_comment.go site is silently buggier today.

Validation

  • All four sites continue to return the same user-facing behavior.
  • go test ./pkg/cli/... ./pkg/parser/... passes.
  • No new occurrences of strings.Contains(err.Error(), "404") or "Not Found" outside the helper.

Severity

Medium. outcome_eval_comment.go is silently missing lower-cased "not found" matches today — a small but real false-negative footprint.

Context

Found by Sergo Run 12 — helper-adoption follow-up + error-comparison audit. Run §25981825933. Companion issue: helper-underadoption pair with isPermissionError consolidation.

Generated by 🤖 Sergo - Serena Go Expert · ● 23.5M ·

  • expires on May 24, 2026, 5:10 AM UTC

Metadata

Metadata

Labels

cookieIssue Monster Loves Cookies!sergo

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions