[refactor] Duplicate value-formatting logic and split-brain error classifiers around pkg/gitutil and pkg/importinpututil

[github-actions]

🔧 Semantic Function Clustering Analysis

Scope: pkg/gitutil, pkg/importinpututil (2 non-test files, 20 functions)

Executive Summary

Both in-scope files are internally cohesive and correctly named — no outlier functions need to move out of them for naming reasons. However, semantic comparison against the rest of the repo surfaced one confirmed copy-paste duplicate, one split-brain error classifier, and one scattered inline idiom that re-derives a function that already exists.

#	Finding	Severity	Sites
1	marshalEnvValue duplicates importinpututil.FormatResolvedValue	High	2
2	Two divergent "canonical" auth-error classifiers	Medium	2
3	Full-SHA check hand-inlined instead of IsValidFullSHA	Medium	7
4	Error classifiers sit in gitutil rather than errorutil	Medium	2
5	Rate-limit substrings re-listed in isTransientSHAResolutionError	Low	1

---

1. Duplicate: marshalEnvValue re-implements importinpututil (High)

pkg/workflow/step_types.go:246 marshalEnvValue is a near-verbatim reimplementation of the entire public surface of the in-scope file pkg/importinpututil/import_input.go — FormatResolvedValue + formatReflectiveValue + marshalValue + normalizeSlice + normalizeMap, inlined into a single function.

The normalizeSlice and normalizeMap bodies are character-for-character identical, including the sorted-key ordering and the Go 1.22 for i := range rv.Len() idiom.

Side-by-side comparison

// pkg/importinpututil/import_input.go:84
func normalizeMap(rv reflect.Value) map[string]any {
	keys := make([]string, 0, rv.Len())
	for _, key := range rv.MapKeys() {
		keys = append(keys, key.String())
	}
	sort.Strings(keys)
	normalized := make(map[string]any, rv.Len())
	for _, k := range keys {
		normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface()
	}
	return normalized
}

// pkg/workflow/step_types.go:269 — inlined, identical
case reflect.Map:
	keys := make([]string, 0, rv.Len())
	for _, key := range rv.MapKeys() {
		keys = append(keys, key.String())
	}
	sort.Strings(keys)
	normalized := make(map[string]any, rv.Len())
	for _, k := range keys {
		normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface()
	}
	if b, err := json.Marshal(normalized); err == nil {
		return string(b)
	}

Behavioral deltas (all reconcilable at the call site):

Case	FormatResolvedValue	marshalEnvValue
nil	("", false)	""
marshal error	("", false)	fmt.Sprint(v)
scalar	fmt.Sprintf("%v", v), true	fmt.Sprint(v) — same output

Why this is safe to consolidate: pkg/workflow already imports importinpututil (pkg/workflow/expression_extraction.go:13), so there is no new dependency and no import cycle.

Recommendation: reduce marshalEnvValue to a delegation:

func marshalEnvValue(v any) string {
	if s, ok := importinpututil.FormatResolvedValue(v); ok {
		return s
	}
	return "" // nil and marshal-failure both fall here
}

Impact: removes ~35 duplicated lines; makes JSON env-var encoding and import-input substitution provably consistent — today they can silently drift.

---

2. Split-brain auth-error classification (Medium)

Two functions classify the same thing with different marker sets and different case sensitivity:

• pkg/gitutil/gitutil.go:37 IsAuthError — case-insensitive, 8 markers, used at 14+ call sites across pkg/parser and pkg/cli
• pkg/cli/audit.go:299 isPermissionErrorStr — case-sensitive, 8 different markers

isPermissionErrorStr carries this comment:

// This is the canonical union of all auth-error substrings used across the codebase; update here rather than adding new inline strings.Contains checks in callers.

That claim is not accurate — gitutil.IsAuthError predates it and is far more widely used.

Marker set divergence

Marker	IsAuthError	isPermissionErrorStr
not logged into	✅	✅ (full phrase)
gh_token / GH_TOKEN	✅ (lowercased)	✅ (uppercase only)
authentication	✅	⚠️ authentication required only
unauthorized	✅	❌
forbidden	✅	❌
permission denied	✅	⚠️ bare permission
saml enforcement	✅	❌
exit status 4	❌	✅
gh auth login	❌	✅

Consequence: an error reading "SAML enforcement blocked this request" is an auth error to gitutil but not to audit.go. Conversely "exit status 4" is auth to audit.go only. Because isPermissionErrorStr is case-sensitive, a lowercase gh_token in an error string slips past it.

Recommendation: make isPermissionErrorStr delegate to gitutil.IsAuthError and add only the two gh-CLI-specific markers it uniquely needs, or merge both marker sets into one classifier. Either way, correct the misleading "canonical union" comment.

---

3. Full-SHA predicate inlined at 7 sites (Medium)

gitutil.IsValidFullSHA exists for exactly this purpose, but 7 call sites hand-write len(x) == 40 && gitutil.IsHexString(x) instead:

pkg/parser/remote_resolve_sha.go:78    if len(sha) != 40 || !gitutil.IsHexString(sha)
pkg/parser/remote_resolve_sha.go:89    if len(ref) == 40 && gitutil.IsHexString(ref)
pkg/parser/remote_resolve_sha.go:155   if len(sha) != 40 || !gitutil.IsHexString(sha)
pkg/parser/remote_resolve_sha.go:207   if result.SHA == "" || len(result.SHA) != 40 || !gitutil.IsHexString(result.SHA)
pkg/parser/remote_download_file.go:402 if len(ref) == 40 && gitutil.IsHexString(ref)
pkg/workflow/action_resolver.go:187    if len(sha) != 40 || !gitutil.IsHexString(sha)
pkg/cli/download_workflow.go:132       isSHA := len(ref) == 40 && gitutil.IsHexString(ref)

⚠️ Not a pure find-and-replace. IsValidFullSHA uses ^[0-9a-f]{40}$ (lowercase only), whereas IsHexString accepts A-F. Swapping in IsValidFullSHA tightens behavior by rejecting uppercase SHAs. GitHub API responses are lowercase, so this is almost certainly the intended semantics — but each site should be reviewed rather than bulk-edited.

Recommendation: replace all 7 with gitutil.IsValidFullSHA, confirming per-site that uppercase SHAs are not a supported input. Note that pkg/workflow/skills_frontmatter.go:35 already combines both correctly for the partial-SHA case, so it should stay as-is.

---

4. Outlier: GitHub API error classifiers live in gitutil (Medium)

pkg/gitutil clusters into 7 clean groups — hex/SHA validation, git-argument safety, repo-root discovery, HEAD blob reading, repo-path parsing, environment lookup — plus one group that does not belong:

• IsRateLimitError (line 28)
• IsAuthError (line 37)

Neither touches git. They classify GitHub API / gh CLI error strings — which is the stated charter of a package that already exists:

// Package errorutil provides shared helpers for classifying and inspecting errors returned by the GitHub API and gh CLI.

pkg/errorutil already houses IsNotFoundError, IsForbiddenError, and IsGoneError, and already has the exact private primitive both gitutil functions hand-roll:

// pkg/errorutil/errors.go:73 — identical semantics to the
// strings.ToLower + chained strings.Contains in both gitutil functions
func containsSubstring(value string, substrings ...string) bool {
	msg := strings.ToLower(value)
	for _, substring := range substrings {
		if strings.Contains(msg, substring) { return true }
	}
	return false
}

Recommendation: move both to pkg/errorutil and reimplement over containsSubstring. This also creates the natural home for resolving finding #2. pkg/errorutil has no dependency on pkg/gitutil, so there is no cycle risk.

---

5. Rate-limit substrings re-listed (Low)

pkg/cli/fetch.go:269 isTransientSHAResolutionError inlines "rate limit" and "http 429" alongside its network/timeout markers, overlapping gitutil.IsRateLimitError.

This one is largely acceptable — the transient check is intentionally broader (timeouts, connection resets, 5xx) and its "rate limit" prefix match is deliberately looser than the three specific phrases in IsRateLimitError. Flagged only so the overlap is a conscious choice.

Recommendation: optionally delegate the rate-limit clause to gitutil.IsRateLimitError (soon errorutil) and keep the network markers local. Low priority.

---

Function inventory & cluster analysis

pkg/gitutil/gitutil.go — 14 functions

Cluster	Functions	Verdict
GitHub API error classification	IsRateLimitError, IsAuthError	❌ outlier — see #4
Hex / SHA validation	IsHexString, IsValidFullSHA, isGitObjectID	✅ cohesive
Git argument safety (CWE-88)	ValidateGitRef, ValidateGitPath	✅ cohesive
Repo root discovery	FindGitRoot, FindGitRootFrom	✅ cohesive
HEAD blob reading	ReadFileFromHEAD, resolveHEADBlobID	✅ cohesive
Repo path parsing	ExtractBaseRepo	✅
Environment lookup	Getwd, UserHomeDir	✅ thin, well-justified wrappers

At 307 lines across 6 concerns in a single-file package, gitutil.go is near the threshold where splitting (sha.go, validate.go, root.go) would help — but this is a judgment call, not a defect. Removing the error classifiers (#4) is the higher-value change.

pkg/importinpututil/import_input.go — 6 functions

Single cohesive cluster: resolve an import-input path, then format the value for textual substitution. Well-factored — FormatResolvedValue → formatReflectiveValue → marshalValue / normalizeSlice / normalizeMap is a clean decomposition. This file is the correct home; finding #1 is the duplicate, not this.

Checked and cleared

• pkg/workflow/git_helpers.go:48 findGitRoot — not a duplicate of gitutil.FindGitRoot; it delegates and swallows the error to return "". Intentional, documented.
• pkg/parser/import_input_substitution.go:49-59 — one-line pass-throughs to importinpututil. Trivial indirection, below the reporting threshold.

Implementation Checklist

• rejig docs #1 Reduce marshalEnvValue to delegate to importinpututil.FormatResolvedValue (highest value, zero new deps)
• Add workflow: githubnext/agentics/weekly-research #4 Move IsRateLimitError / IsAuthError to pkg/errorutil, reimplement over containsSubstring
• Add workflow: githubnext/agentics/weekly-research #2 Merge isPermissionErrorStr into the relocated IsAuthError; fix the inaccurate "canonical union" comment
• Add workflow: githubnext/agentics/weekly-research #3 Replace 7 inline len==40 && IsHexString sites with IsValidFullSHA, verifying uppercase-SHA handling per site
• Add workflow: githubnext/agentics/weekly-research #5 (optional) Delegate the rate-limit clause of isTransientSHAResolutionError
• Run go test ./... — findings Add workflow: githubnext/agentics/weekly-research #2 and Add workflow: githubnext/agentics/weekly-research #3 change classification behavior at the edges

Analysis metadata

• Files in scope: 2 (pkg/gitutil/gitutil.go, pkg/importinpututil/import_input.go)
• Functions cataloged: 20
• Clusters identified: 8
• Outliers found: 2 (IsRateLimitError, IsAuthError)
• Duplicates confirmed: 1 exact-structure, 1 functional, 1 scattered idiom (7 sites)
• Method: Serena semantic analysis (gopls) + repo-wide reference and pattern search
• Import-cycle safety: verified for all consolidation recommendations

Generated by 🔧 Semantic Function Refactoring · sonnet46 · 207.3 AIC · ⌖ 22.7 AIC · ⊞ 10K · ◷

• expires on Aug 17, 2026, 6:49 PM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue 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.17 AIC · ⌖ 6.06 AIC · ⊞ 9.5K · ◷