Refactor shared value formatting and unify GitHub error/SHA classification paths

docs/adr/53018-centralize-error-classifiers-and-value-formatting.md

@@ -0,0 +1,45 @@
+# ADR-53018: Centralize GitHub Error Classifiers and Value Formatting
+
+**Date**: 2026-08-16
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The codebase had duplicate implementations of two cross-cutting concerns scattered across multiple packages. `IsAuthError` and `IsRateLimitError` were defined in `pkg/gitutil` but called by `pkg/cli` and `pkg/parser` — packages that have no semantic dependency on git operations. Placing error-classification logic in a git utility package created an inappropriate coupling: callers that only needed to classify GitHub API responses had to import git infrastructure. Independently, `marshalEnvValue` in `pkg/workflow` contained inlined JSON/reflect normalization that duplicated the same logic already present in `importinpututil.FormatResolvedValue`, creating a split-brain risk where the two serialization paths could diverge silently. Additionally, full-SHA validation was written inline as `len(x)==40 && gitutil.IsHexString(x)` at seven separate call sites rather than using the already-exported `gitutil.IsValidFullSHA` predicate.
+
+### Decision
+
+We will move `IsAuthError` and `IsRateLimitError` out of `pkg/gitutil` and into `pkg/errorutil` as the canonical shared API for GitHub error classification. We will update all callers across `pkg/cli` and `pkg/parser` to import from `errorutil`. We will replace `marshalEnvValue`'s inlined JSON/reflect normalization with a delegation to `importinpututil.FormatResolvedValue`, keeping only a `fmt.Sprint` scalar fallback and a `nil`→`""` guard. We will replace all inline `len(x)==40 && IsHexString(x)` predicates with `gitutil.IsValidFullSHA`.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep classifiers in `gitutil`, add re-export shims in `errorutil`
+
+Re-export `gitutil.IsAuthError` and `gitutil.IsRateLimitError` from `errorutil` without moving the implementation. Callers can import from either package. This avoids touching the implementation and keeps `gitutil` as the authority, but it creates two public APIs for the same function, does not fix the semantic mismatch (error classification is not a git concern), and leaves the underlying coupling intact. It was rejected because it trades a clean break for ongoing confusion about which package owns the behavior.
+
+#### Alternative 2: Inline error-classification logic at each call site
+
+Remove shared classifiers entirely and duplicate the substring checks wherever they are needed. This eliminates the package-dependency question but defeats the goal of a single source of truth, making future changes to classification phrases error-prone and requiring updates across many files. It was rejected because the problem that motivated `gitutil.IsAuthError` in the first place — avoiding scattered inline checks — would recur immediately.
+
+### Consequences
+
+#### Positive
+- `pkg/gitutil` scope is now narrowly defined as git repository operations and SHA/ref validation, eliminating an inappropriate coupling to GitHub API error semantics.
+- `pkg/errorutil` becomes the single authoritative location for GitHub error classification, so future phrase changes need to be made in exactly one place.
+- `marshalEnvValue` and `importinpututil.FormatResolvedValue` are guaranteed to produce identical serialization for arrays and maps, eliminating the risk of silent divergence between the two code paths.
+- Inline SHA predicates are replaced by a named, tested, regex-backed function, reducing the chance of off-by-one errors (e.g., accepting mixed-case or 64-character SHAs).
+
+#### Negative
+- The change touches 21 files across `pkg/cli`, `pkg/parser`, `pkg/workflow`, `pkg/gitutil`, and `pkg/errorutil`, making it a wide-surface refactor that carries merge-conflict risk for any concurrent branches importing `gitutil.IsAuthError`.
+- Removing `IsRateLimitError` and `IsAuthError` from `pkg/gitutil`'s public API is a breaking change for any external consumers that imported those symbols directly (though this appears to be an internal-only codebase).
+
+#### Neutral
+- `isPermissionErrorStr` in `pkg/cli/audit.go` now delegates to `errorutil.IsAuthError` and augments with audit-specific markers (`exit status 4`, `permission`, `gh auth login`, workflow guidance) rather than maintaining its own canonical union — this preserves audit-command-specific behavior without duplicating shared logic.
+- Tests for the moved functions are migrated from `pkg/gitutil` to `pkg/errorutil`, and spec tests are updated to reflect the new package ownership.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/audit.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/fileutil"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/parser"
@@ -293,18 +294,18 @@ func runAuditMulti(ctx context.Context, args []string, repoFlag, outputDir strin
 	})
 }
 
-// isPermissionErrorStr checks if a string contains any known permission/authentication error marker.
-// 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.
+// isPermissionErrorStr checks if a string contains known permission/authentication markers.
+// It delegates to the shared classifier and augments with gh CLI specific hints
+// that are only emitted in audit command contexts.
 func isPermissionErrorStr(s string) bool {
-	return strings.Contains(s, "authentication required") ||
-		strings.Contains(s, "exit status 4") ||
-		strings.Contains(s, "GitHub CLI authentication") ||
-		strings.Contains(s, "permission") ||
-		strings.Contains(s, "GH_TOKEN") ||
-		strings.Contains(s, "not logged into any GitHub hosts") ||
-		strings.Contains(s, "To use GitHub CLI in a GitHub Actions workflow") ||
-		strings.Contains(s, "gh auth login")
+	if errorutil.IsAuthError(s) {
+		return true
+	}
+	lower := strings.ToLower(s)
+	return strings.Contains(lower, "exit status 4") ||
+		strings.Contains(lower, "permission") ||
+		strings.Contains(lower, "gh auth login") ||
+		strings.Contains(lower, "to use github cli in a github actions workflow")
 }
 
 // isPermissionError checks if an error is related to permissions/authentication.

pkg/cli/audit_test.go

@@ -105,6 +105,11 @@ func TestIsPermissionErrorStr(t *testing.T) {
 			s:        "Run gh auth login to proceed",
 			expected: true,
 		},
+		{
+			name:     "GitHub CLI authentication marker",
+			s:        "GitHub CLI authentication token is missing",
+			expected: true,
+		},
 		{
 			name:     "Empty string",
 			s:        "",

pkg/cli/download_workflow.go

@@ -12,6 +12,7 @@ import (
 	"github.com/github/gh-aw/pkg/constants"
 
 	"github.com/github/gh-aw/pkg/console"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/fileutil"
 	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/github/gh-aw/pkg/logger"
@@ -129,7 +130,7 @@ func downloadWorkflowContentViaGitClone(ctx context.Context, repo, path, ref str
 	}
 
 	// Check if ref is a SHA (40 hex characters)
-	isSHA := len(ref) == 40 && gitutil.IsHexString(ref)
+	isSHA := gitutil.IsValidFullSHACaseInsensitive(ref)
 	downloadLog.Printf("Fetching ref via sparse checkout: is_sha=%t", isSHA)
 
 	if isSHA {
@@ -197,7 +198,7 @@ func downloadWorkflowContent(ctx context.Context, repo, path, ref string, verbos
 	if err != nil {
 		// Check if this is an authentication error
 		outputStr := string(output)
-		if gitutil.IsAuthError(outputStr) || gitutil.IsAuthError(err.Error()) {
+		if errorutil.IsAuthError(outputStr) || errorutil.IsAuthError(err.Error()) {
 			downloadLog.Printf("GitHub API authentication failed, attempting git fallback for %s/%s@%s", repo, path, ref)
 			// Try fallback using git commands
 			content, gitErr := downloadWorkflowContentViaGit(ctx, repo, path, ref, verbose)

pkg/cli/forecast_compute.go

@@ -15,8 +15,8 @@ import (
 
 	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/fileutil"
-	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
 
@@ -76,7 +76,7 @@ func forecastWorkflow(ctx context.Context, workflowName, startDate string, confi
 
 	runs, _, err := listRunsWithBackoff(ctx, opts, result.WorkflowID)
 	if err != nil {
-		if gitutil.IsRateLimitError(err.Error()) {
+		if errorutil.IsRateLimitError(err.Error()) {
 			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
 				fmt.Sprintf("Skipping %s: GitHub API rate limit exceeded", result.WorkflowID)))
 			return result, nil

pkg/cli/forecast_resolution.go

@@ -9,7 +9,7 @@ import (
 	"time"
 
 	"github.com/github/gh-aw/pkg/console"
-	"github.com/github/gh-aw/pkg/gitutil"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/workflow"
 )
@@ -116,7 +116,7 @@ func fetchWorkflowsWithBackoff(ctx context.Context, ids []string, repoOverride s
 		if err == nil {
 			return githubWorkflows, nil
 		}
-		if !gitutil.IsRateLimitError(err.Error()) {
+		if !errorutil.IsRateLimitError(err.Error()) {
 			return nil, err
 		}
 
@@ -160,7 +160,7 @@ func listRunsWithBackoff(ctx context.Context, opts ListWorkflowRunsOptions, work
 		if err == nil {
 			return runs, total, nil
 		}
-		if !gitutil.IsRateLimitError(err.Error()) {
+		if !errorutil.IsRateLimitError(err.Error()) {
 			return nil, 0, err
 		}
 

pkg/cli/health_command.go

@@ -10,7 +10,7 @@ import (
 
 	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/constants"
-	"github.com/github/gh-aw/pkg/gitutil"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/workflow"
 	"github.com/spf13/cobra"
@@ -133,7 +133,7 @@ func RunHealth(config HealthConfig) error {
 	// Fetch workflow runs from GitHub
 	runs, err := fetchWorkflowRuns(workflowAPIName, startDate, config.RepoOverride, config.Verbose)
 	if err != nil {
-		if gitutil.IsRateLimitError(err.Error()) {
+		if errorutil.IsRateLimitError(err.Error()) {
 			// Rate limiting is a transient infrastructure condition, not a code error.
 			// Warn and exit cleanly so CI jobs are not marked as failed.
 			fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Skipping health check: GitHub API rate limit exceeded"))

pkg/cli/update_actions_release.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"github.com/github/gh-aw/pkg/console"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/github/gh-aw/pkg/semverutil"
 	"github.com/github/gh-aw/pkg/workflow"
@@ -39,7 +40,7 @@ func getLatestActionReleaseWithDeps(ctx context.Context, deps actionUpdateDeps,
 	if err != nil {
 		// Check if this is an authentication error
 		outputStr := string(output)
-		if gitutil.IsAuthError(outputStr) || gitutil.IsAuthError(err.Error()) {
+		if errorutil.IsAuthError(outputStr) || errorutil.IsAuthError(err.Error()) {
 			updateLog.Printf("GitHub API authentication failed, attempting git ls-remote fallback for %s", repo)
 			// Try fallback using git ls-remote
 			latestRelease, latestSHA, gitErr := deps.getLatestReleaseViaGit(ctx, repo, currentVersion, allowMajor, verbose)

pkg/cli/update_display.go

@@ -7,7 +7,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw/pkg/console"
-	"github.com/github/gh-aw/pkg/gitutil"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/logger"
 )
 
@@ -68,11 +68,11 @@ func groupUpdateFailures(failures []updateFailure) []updateFailureGroup {
 
 func compactUpdateFailureReason(message string) string {
 	switch {
-	case gitutil.IsAuthError(message) && gitutil.IsRateLimitError(message):
+	case errorutil.IsAuthError(message) && errorutil.IsRateLimitError(message):
 		return "SAML-restricted authenticated access; anonymous GitHub API fallback is rate-limited"
-	case gitutil.IsAuthError(message):
+	case errorutil.IsAuthError(message):
 		return "GitHub API access is restricted by authentication or SAML"
-	case gitutil.IsRateLimitError(message):
+	case errorutil.IsRateLimitError(message):
 		return "GitHub API rate limit exceeded"
 	}
 

pkg/cli/update_workflows.go

@@ -14,10 +14,9 @@ import (
 	"sync"
 	"time"
 
-	"github.com/github/gh-aw/pkg/constants"
-
 	"github.com/github/gh-aw/pkg/console"
-	"github.com/github/gh-aw/pkg/gitutil"
+	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/errorutil"
 	"github.com/github/gh-aw/pkg/parser"
 	"github.com/github/gh-aw/pkg/semverutil"
 	"github.com/github/gh-aw/pkg/workflow"
@@ -187,7 +186,7 @@ func UpdateWorkflows(ctx context.Context, opts UpdateWorkflowsOptions) error {
 // (non-fatal) from genuine update failures (fatal).
 func allFailuresAreRateLimited(failures []updateFailure) bool {
 	for _, f := range failures {
-		if !gitutil.IsRateLimitError(f.Error) {
+		if !errorutil.IsRateLimitError(f.Error) {
 			return false
 		}
 	}
@@ -472,7 +471,7 @@ func fetchPublicReleaseTagsPaginated(ctx context.Context, repo string) ([]string
 // getRepoDefaultBranch fetches the default branch name for a repository.
 func getRepoDefaultBranch(ctx context.Context, repo string) (string, error) {
 	output, err := workflow.RunGHContext(ctx, "Fetching repo info...", "api", "/repos/"+repo, "--jq", ".default_branch")
-	if err != nil && gitutil.IsAuthError(err.Error()) {
+	if err != nil && errorutil.IsAuthError(err.Error()) {
 		updateLog.Printf("GitHub API auth failed for %s, retrying without token", repo)
 		body, fallbackErr := fetchPublicGitHubAPI(ctx, "/repos/"+repo)
 		if fallbackErr != nil {
@@ -506,7 +505,7 @@ func getLatestBranchCommitInfo(ctx context.Context, repo, branch string) (latest
 	// URL-encode the branch name since it may contain slashes (e.g. "feature/foo")
 	endpoint := fmt.Sprintf("/repos/%s/commits/%s", repo, url.PathEscape(branch))
 	output, err := workflow.RunGHContext(ctx, "Fetching commit info...", "api", endpoint)
-	if err != nil && gitutil.IsAuthError(err.Error()) {
+	if err != nil && errorutil.IsAuthError(err.Error()) {
 		updateLog.Printf("GitHub API auth failed for branch %s of %s, retrying without token", branch, repo)
 		body, fallbackErr := fetchPublicGitHubAPI(ctx, endpoint)
 		if fallbackErr != nil {
@@ -564,7 +563,7 @@ func defaultWorkflowUpdateDeps() workflowUpdateDeps {
 		runReleasesAPI: func(ctx context.Context, repo string) ([]byte, error) {
 			endpoint := fmt.Sprintf("/repos/%s/releases", repo)
 			output, err := workflow.RunGHContext(ctx, "Fetching releases...", "api", "--paginate", endpoint, "--jq", ".[].tag_name")
-			if err != nil && gitutil.IsAuthError(err.Error()) {
+			if err != nil && errorutil.IsAuthError(err.Error()) {
 				updateLog.Printf("GitHub API auth failed for releases of %s, retrying without token", repo)
 				tags, fallbackErr := fetchPublicReleaseTagsPaginated(ctx, repo)
 				if fallbackErr != nil {

pkg/errorutil/README.md

@@ -4,7 +4,7 @@ The `errorutil` package provides shared helpers for classifying and inspecting e
 
 ## Overview
 
-This package currently exposes focused helpers for identifying common error categories used across `pkg/cli` and `pkg/parser`, including "not found" (`404`), "forbidden" (`403`), and "gone" (`410`) responses.
+This package currently exposes focused helpers for identifying common error categories used across `pkg/cli` and `pkg/parser`, including "not found" (`404`), "forbidden" (`403`), "gone" (`410`), rate-limit, and authentication/authorization responses.
 
 ## Public API
 
@@ -15,6 +15,8 @@ This package currently exposes focused helpers for identifying common error cate
 | `IsNotFoundError` | `func(err error) bool` | Returns `true` when `err` indicates a "not found" condition by matching case-insensitive `404` or `not found` text; returns `false` for `nil` and non-matching errors |
 | `IsForbiddenError` | `func(err error) bool` | Returns `true` when `err` indicates an HTTP-style `403`/"forbidden" response by matching case-insensitive patterns like `HTTP 403` or `403 Forbidden`; returns `false` for `nil` and non-matching errors |
 | `IsGoneError` | `func(err error) bool` | Returns `true` when `err` indicates an HTTP-style `410`/"gone" response by matching case-insensitive patterns like `HTTP 410` or `410 Gone`; returns `false` for `nil` and non-matching errors |
+| `IsRateLimitError` | `func(output string) bool` | Returns `true` when `output` indicates GitHub API rate limiting by matching case-insensitive `rate limit exceeded` (including `API rate limit exceeded`) or `secondary rate limit` text |
+| `IsAuthError` | `func(output string) bool` | Returns `true` when `output` indicates authentication or authorization failures by matching case-insensitive credential-specific markers including `GH_TOKEN`, `GITHUB_TOKEN`, `authentication`, `not logged into`, `unauthorized`, `permission denied`, or `SAML enforcement` |
 
 ## Usage Examples
 
@@ -32,6 +34,14 @@ if errorutil.IsForbiddenError(err) {
 if errorutil.IsGoneError(err) {
     // Handle expired or deleted resource
 }
+
+if errorutil.IsRateLimitError(output) {
+    // Back off and retry
+}
+
+if errorutil.IsAuthError(output) {
+    // Surface credential guidance
+}
 ```
 
 ## Dependencies
@@ -45,6 +55,7 @@ if errorutil.IsGoneError(err) {
 ## Design Notes
 
 - `IsNotFoundError`, `IsForbiddenError`, and `IsGoneError` intentionally accept multiple message formats to cover errors produced by GitHub API responses, `gh` CLI output, and `go-gh` wrappers.
+- `IsRateLimitError` and `IsAuthError` provide shared case-insensitive string classifiers for GitHub API and `gh` CLI output so callers avoid duplicating inline substring checks.
 - `IsForbiddenError` and `IsGoneError` intentionally require HTTP-style status context so unrelated phrases like `forbidden character` or `gone away` are not misclassified.
 
 ---

pkg/errorutil/errors.go

@@ -60,6 +60,37 @@ func IsGoneError(err error) bool {
 	return matched
 }
 
+// IsRateLimitError reports whether output indicates a GitHub API rate-limit error.
+// The check is case-insensitive and matches known API phrases.
+func IsRateLimitError(output string) bool {
+	matched := containsSubstring(output,
+		"rate limit exceeded",
+		"secondary rate limit",
+	)
+	if matched {
+		errorutilLog.Printf("Classified output as rate-limit related (len=%d)", len(output))
+	}
+	return matched
+}
+
+// IsAuthError reports whether output indicates an authentication or
+// authorization issue from the GitHub API or gh CLI.
+func IsAuthError(output string) bool {
+	matched := containsSubstring(output,
+		"gh_token",
+		"github_token",
+		"authentication",
+		"not logged into",
+		"unauthorized",
+		"permission denied",
+		"saml enforcement",
+	)
+	if matched {
+		errorutilLog.Printf("Classified output as auth-related (len=%d)", len(output))
+	}
+	return matched
+}
+
 // containsErrorSubstring reports whether err contains any of the provided
 // substrings after lowercasing the full error message for case-insensitive
 // matching.