diff --git a/docs/adr/53018-centralize-error-classifiers-and-value-formatting.md b/docs/adr/53018-centralize-error-classifiers-and-value-formatting.md new file mode 100644 index 00000000000..408243dd372 --- /dev/null +++ b/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.* diff --git a/pkg/cli/audit.go b/pkg/cli/audit.go index 2af5836eed5..98c20089a9a 100644 --- a/pkg/cli/audit.go +++ b/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. diff --git a/pkg/cli/audit_test.go b/pkg/cli/audit_test.go index 11123e8f188..3c9f88ca720 100644 --- a/pkg/cli/audit_test.go +++ b/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: "", diff --git a/pkg/cli/download_workflow.go b/pkg/cli/download_workflow.go index a92cc0f4226..b48a9ea9ee1 100644 --- a/pkg/cli/download_workflow.go +++ b/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) diff --git a/pkg/cli/forecast_compute.go b/pkg/cli/forecast_compute.go index e435278cce0..29bd0e341c5 100644 --- a/pkg/cli/forecast_compute.go +++ b/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 diff --git a/pkg/cli/forecast_resolution.go b/pkg/cli/forecast_resolution.go index 4fb8e84cb8f..768bbaa03e4 100644 --- a/pkg/cli/forecast_resolution.go +++ b/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 } diff --git a/pkg/cli/health_command.go b/pkg/cli/health_command.go index 76d9933103b..4f98c859177 100644 --- a/pkg/cli/health_command.go +++ b/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")) diff --git a/pkg/cli/update_actions_release.go b/pkg/cli/update_actions_release.go index b65d6020f85..218f9050648 100644 --- a/pkg/cli/update_actions_release.go +++ b/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) diff --git a/pkg/cli/update_display.go b/pkg/cli/update_display.go index af339132adf..f6b3e283578 100644 --- a/pkg/cli/update_display.go +++ b/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" } diff --git a/pkg/cli/update_workflows.go b/pkg/cli/update_workflows.go index 194d29dd8ac..24ec8cc5589 100644 --- a/pkg/cli/update_workflows.go +++ b/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 { diff --git a/pkg/errorutil/README.md b/pkg/errorutil/README.md index 1c7934df992..99ec04ffe2c 100644 --- a/pkg/errorutil/README.md +++ b/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. --- diff --git a/pkg/errorutil/errors.go b/pkg/errorutil/errors.go index dd98130138b..c968002f82a 100644 --- a/pkg/errorutil/errors.go +++ b/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. diff --git a/pkg/errorutil/errors_test.go b/pkg/errorutil/errors_test.go index 193028e9841..961416fcbff 100644 --- a/pkg/errorutil/errors_test.go +++ b/pkg/errorutil/errors_test.go @@ -118,3 +118,47 @@ func TestIsGoneError(t *testing.T) { }) } } + +func TestIsRateLimitError(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + {name: "api rate limit exceeded", output: "API rate limit exceeded", want: true}, + {name: "rate limit exceeded", output: "rate limit exceeded", want: true}, + {name: "secondary rate limit", output: "secondary rate limit triggered", want: true}, + {name: "case-insensitive", output: "API RATE LIMIT EXCEEDED", want: true}, + {name: "non-rate-limit error", output: "HTTP 404: Not Found", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, errorutil.IsRateLimitError(tt.output)) + }) + } +} + +func TestIsAuthError(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + {name: "gh_token", output: "GH_TOKEN is not set", want: true}, + {name: "github_token", output: "GITHUB_TOKEN is invalid", want: true}, + {name: "authentication", output: "authentication required", want: true}, + {name: "not logged into", output: "not logged into any GitHub hosts", want: true}, + {name: "unauthorized", output: "HTTP 401: Unauthorized", want: true}, + {name: "forbidden is not inherently an auth failure", output: "HTTP 403: Forbidden", want: false}, + {name: "permission denied", output: "permission denied: insufficient scope", want: true}, + {name: "saml enforcement", output: "Resource protected by organization SAML enforcement", want: true}, + {name: "non-auth error", output: "API rate limit exceeded for installation", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, errorutil.IsAuthError(tt.output)) + }) + } +} diff --git a/pkg/errorutil/spec_test.go b/pkg/errorutil/spec_test.go index bf1465b7001..0dcc3265ee6 100644 --- a/pkg/errorutil/spec_test.go +++ b/pkg/errorutil/spec_test.go @@ -111,6 +111,51 @@ func TestSpec_PublicAPI_IsGoneError(t *testing.T) { } } +// TestSpec_PublicAPI_IsRateLimitError validates the documented behavior of +// IsRateLimitError as described in the errorutil README.md. +func TestSpec_PublicAPI_IsRateLimitError(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + {name: "documented phrase api rate limit exceeded", output: "403: API rate limit exceeded", want: true}, + {name: "documented phrase rate limit exceeded", output: "rate limit exceeded for installation", want: true}, + {name: "documented phrase secondary rate limit", output: "secondary rate limit triggered", want: true}, + {name: "case-insensitive", output: "API RATE LIMIT EXCEEDED", want: true}, + {name: "non-matching output", output: "404: not found", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, errorutil.IsRateLimitError(tt.output)) + }) + } +} + +// TestSpec_PublicAPI_IsAuthError validates the documented behavior of +// IsAuthError as described in the errorutil README.md. +func TestSpec_PublicAPI_IsAuthError(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + {name: "GH_TOKEN reference", output: "GH_TOKEN is invalid or expired", want: true}, + {name: "GITHUB_TOKEN reference", output: "GITHUB_TOKEN: authentication failed", want: true}, + {name: "unauthorized", output: "401: unauthorized", want: true}, + {name: "forbidden is not inherently an auth failure", output: "403: forbidden", want: false}, + {name: "saml enforcement", output: "Resource protected by organization SAML enforcement", want: true}, + {name: "non-auth output", output: "404: not found", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, errorutil.IsAuthError(tt.output)) + }) + } +} + // TestSpec_UsageExample_ErrorClassifiers validates that the documented usage // example pattern compiles and runs. // @@ -119,14 +164,20 @@ func TestSpec_PublicAPI_IsGoneError(t *testing.T) { // if errorutil.IsNotFoundError(err) { ... } // if errorutil.IsForbiddenError(err) { ... } // if errorutil.IsGoneError(err) { ... } +// if errorutil.IsRateLimitError(output) { ... } +// if errorutil.IsAuthError(output) { ... } func TestSpec_UsageExample_ErrorClassifiers(t *testing.T) { notFound := errors.New("HTTP 404: Not Found") forbidden := errors.New("HTTP 403: Forbidden") gone := errors.New("HTTP 410: Gone") + rateLimit := "API rate limit exceeded" + authOutput := "GH_TOKEN is missing" assert.True(t, errorutil.IsNotFoundError(notFound), "usage example: 404 path triggered") assert.True(t, errorutil.IsForbiddenError(forbidden), "usage example: 403 path triggered") assert.True(t, errorutil.IsGoneError(gone), "usage example: 410 path triggered") + assert.True(t, errorutil.IsRateLimitError(rateLimit), "usage example: rate-limit path triggered") + assert.True(t, errorutil.IsAuthError(authOutput), "usage example: auth path triggered") assert.False(t, errorutil.IsForbiddenError(notFound), "documented: classifiers are exclusive — 404 is not forbidden") assert.False(t, errorutil.IsGoneError(notFound), "documented: classifiers are exclusive — 404 is not gone") diff --git a/pkg/gitutil/README.md b/pkg/gitutil/README.md index db38c0de59a..9d56944f269 100644 --- a/pkg/gitutil/README.md +++ b/pkg/gitutil/README.md @@ -1,11 +1,10 @@ # gitutil Package -> Utility functions for Git repository operations and GitHub API error classification. +> Utility functions for Git repository operations and SHA/ref validation. ## Overview The `gitutil` package contains helpers for: -- Detecting rate-limit and authentication errors from GitHub API responses. - Validating hex strings (e.g. commit SHAs). - Extracting base repository slugs from action paths. - Finding the root directory of the current Git repository using pure Go filesystem traversal. @@ -23,10 +22,9 @@ The `gitutil` package contains helpers for: | Function | Signature | Description | |----------|-----------|-------------| -| `IsRateLimitError` | `func(errMsg string) bool` | Returns `true` when `errMsg` indicates a GitHub API rate-limit error (case-insensitive match against "api rate limit exceeded", "rate limit exceeded", or "secondary rate limit") | -| `IsAuthError` | `func(errMsg string) bool` | Returns `true` when `errMsg` indicates an authentication or authorization failure (case-insensitive match against `GH_TOKEN`, `GITHUB_TOKEN`, `authentication`, `not logged into`, `unauthorized`, `forbidden`, `permission denied`, or `SAML enforcement`) | | `IsHexString` | `func(s string) bool` | Returns `true` if `s` consists entirely of hexadecimal characters (`0–9`, `a–f`, `A–F`); returns `false` for the empty string | | `IsValidFullSHA` | `func(s string) bool` | Returns `true` if `s` is a valid 40-character lowercase hexadecimal SHA (matches `^[0-9a-f]{40}$`) | +| `IsValidFullSHACaseInsensitive` | `func(s string) bool` | Returns `true` if `s` is a valid 40-character hexadecimal SHA with either uppercase or lowercase letters | | `ValidateGitRef` | `func(ref string) error` | Returns an error if `ref` would be unsafe to pass as a positional argument to a `git` subprocess: rejects empty refs, refs starting with `-` (argument injection, CWE-88), refs containing NUL bytes, and refs containing `..` (object traversal expressions) | | `ValidateGitPath` | `func(path string) error` | Returns an error if `path` would be unsafe to pass as a positional argument to a `git` subprocess: rejects empty paths, paths starting with `-` (argument injection, CWE-88), absolute paths, and paths that resolve (after `path.Clean`) to `..` or contain a leading `../` traversal segment | | `ExtractBaseRepo` | `func(repoPath string) string` | Extracts the `owner/repo` portion from an action path that may include a sub-folder (e.g. `github/codeql-action/upload-sarif` → `github/codeql-action`) | @@ -38,10 +36,9 @@ The `gitutil` package contains helpers for: **Behavioral contracts**: -- `IsRateLimitError` and `IsAuthError` MUST perform case-insensitive string matching. -- `IsAuthError` MUST return `true` for messages containing any of: `gh_token`, `github_token`, `authentication`, `not logged into`, `unauthorized`, `forbidden`, `permission denied`, or `saml enforcement`. - `IsHexString` MUST return `false` for the empty string. - `IsValidFullSHA` MUST require exactly 40 lowercase hexadecimal characters; mixed-case or shorter strings MUST return `false`. +- `IsValidFullSHACaseInsensitive` MUST require exactly 40 hexadecimal characters and accept uppercase and lowercase letters. - `ValidateGitRef` MUST return an error for an empty ref, a ref starting with `-`, a ref containing a NUL byte, or a ref containing `..`. - `ValidateGitPath` MUST return an error for an empty path, a path starting with `-`, an absolute path, or a path that is `..` or starts with `../` after `path.Clean`. - `FindGitRoot` and `FindGitRootFrom` MUST return `ErrNotGitRepository` (not a wrapped error) when the filesystem root is reached without finding a `.git` entry. @@ -55,11 +52,6 @@ The `gitutil` package contains helpers for: ```go import "github.com/github/gh-aw/pkg/gitutil" -// Check for rate-limit errors from GitHub API -if gitutil.IsRateLimitError(err.Error()) { - // Back off and retry -} - // Validate a commit SHA if gitutil.IsValidFullSHA(commitSHA) { fmt.Println("Valid 40-character commit SHA") @@ -95,7 +87,7 @@ content, err := gitutil.ReadFileFromHEAD(filepath.Join(root, "go.mod"), root) ## Thread Safety -All exported functions are safe for concurrent use. The error-classification functions (`IsRateLimitError`, `IsAuthError`) and SHA-validation functions (`IsHexString`, `IsValidFullSHA`) are pure functions with no shared state. `FindGitRoot` and `FindGitRootFrom` read only the filesystem and the process working directory. `ReadFileFromHEAD` spawns a `git` subprocess per call with no shared state. +All exported functions are safe for concurrent use. The SHA-validation functions (`IsHexString`, `IsValidFullSHA`) are pure functions with no shared state. `FindGitRoot` and `FindGitRootFrom` read only the filesystem and the process working directory. `ReadFileFromHEAD` spawns a `git` subprocess per call with no shared state. ## Dependencies diff --git a/pkg/gitutil/gitutil.go b/pkg/gitutil/gitutil.go index ed9a675ce14..8657ada6a62 100644 --- a/pkg/gitutil/gitutil.go +++ b/pkg/gitutil/gitutil.go @@ -22,35 +22,6 @@ var osUserHomeDir = os.UserHomeDir var fullSHARegex = regexp.MustCompile(`^[0-9a-f]{40}$`) var gitObjectIDRegex = regexp.MustCompile(`^(?:[0-9a-f]{40}|[0-9a-f]{64})$`) -// IsRateLimitError checks if an error message indicates a GitHub API rate limit error. -// This is used to detect transient failures caused by hitting the GitHub API rate limit -// (HTTP 403 "API rate limit exceeded" or HTTP 429 responses). -func IsRateLimitError(errMsg string) bool { - lowerMsg := strings.ToLower(errMsg) - return strings.Contains(lowerMsg, "api rate limit exceeded") || - strings.Contains(lowerMsg, "rate limit exceeded") || - strings.Contains(lowerMsg, "secondary rate limit") -} - -// IsAuthError checks if an error message indicates an authentication issue. -// This is used to detect when GitHub API calls fail due to missing or invalid credentials. -func IsAuthError(errMsg string) bool { - gitutilLog.Printf("Checking if error is auth-related: %s", errMsg) - lowerMsg := strings.ToLower(errMsg) - isAuth := strings.Contains(lowerMsg, "gh_token") || - strings.Contains(lowerMsg, "github_token") || - strings.Contains(lowerMsg, "authentication") || - strings.Contains(lowerMsg, "not logged into") || - strings.Contains(lowerMsg, "unauthorized") || - strings.Contains(lowerMsg, "forbidden") || - strings.Contains(lowerMsg, "permission denied") || - strings.Contains(lowerMsg, "saml enforcement") - if isAuth { - gitutilLog.Print("Detected authentication error") - } - return isAuth -} - // IsHexString checks if a string contains only hexadecimal characters. // This is used to validate Git commit SHAs and other hexadecimal identifiers. func IsHexString(s string) bool { @@ -70,6 +41,11 @@ func IsValidFullSHA(s string) bool { return fullSHARegex.MatchString(s) } +// IsValidFullSHACaseInsensitive checks if s is a valid 40-character hexadecimal SHA. +func IsValidFullSHACaseInsensitive(s string) bool { + return len(s) == 40 && IsHexString(s) +} + // ValidateGitRef returns an error if ref would be unsafe to pass as a positional // argument to a git subprocess. A ref starting with '-' would be parsed as an // option flag rather than a value (argument injection, CWE-88). Refs containing diff --git a/pkg/gitutil/gitutil_test.go b/pkg/gitutil/gitutil_test.go index 0f6c15f03dd..7a529b298cf 100644 --- a/pkg/gitutil/gitutil_test.go +++ b/pkg/gitutil/gitutil_test.go @@ -12,133 +12,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestIsRateLimitError(t *testing.T) { - tests := []struct { - name string - errMsg string - expected bool - }{ - { - name: "GitHub API rate limit exceeded (HTTP 403)", - errMsg: "gh: API rate limit exceeded for installation. If you reach out to GitHub Support for help, please include the request ID (HTTP 403)", - expected: true, - }, - { - name: "rate limit exceeded lowercase", - errMsg: "rate limit exceeded", - expected: true, - }, - { - name: "HTTP 403 with API rate limit message", - errMsg: "HTTP 403: API rate limit exceeded for installation.", - expected: true, - }, - { - name: "secondary rate limit in GitHub error message", - errMsg: "gh: You have exceeded a secondary rate limit", - expected: true, - }, - { - name: "authentication error is not a rate limit error", - errMsg: "authentication required. Run 'gh auth login' first", - expected: false, - }, - { - name: "not found error is not a rate limit error", - errMsg: "HTTP 404: Not Found", - expected: false, - }, - { - name: "empty string", - errMsg: "", - expected: false, - }, - { - name: "unrelated error message", - errMsg: "failed to parse workflow runs: unexpected end of JSON input", - expected: false, - }, - { - name: "mixed case", - errMsg: "API Rate Limit Exceeded for installation", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsRateLimitError(tt.errMsg) - assert.Equal(t, tt.expected, result, "IsRateLimitError(%q) should return %v", tt.errMsg, tt.expected) - }) - } -} - -func TestIsAuthError(t *testing.T) { - tests := []struct { - name string - errMsg string - expected bool - }{ - { - name: "GH_TOKEN mention", - errMsg: "GH_TOKEN is not set", - expected: true, - }, - { - name: "GITHUB_TOKEN mention", - errMsg: "GITHUB_TOKEN is missing or invalid", - expected: true, - }, - { - name: "authentication error", - errMsg: "authentication required", - expected: true, - }, - { - name: "not logged in", - errMsg: "not logged into any GitHub hosts", - expected: true, - }, - { - name: "unauthorized", - errMsg: "HTTP 401: Unauthorized", - expected: true, - }, - { - name: "forbidden", - errMsg: "HTTP 403: Forbidden", - expected: true, - }, - { - name: "permission denied", - errMsg: "permission denied: insufficient scope", - expected: true, - }, - { - name: "saml enforcement", - errMsg: "Resource protected by organization SAML enforcement", - expected: true, - }, - { - name: "rate limit error is not an auth error", - errMsg: "API rate limit exceeded for installation", - expected: false, - }, - { - name: "empty string", - errMsg: "", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := IsAuthError(tt.errMsg) - assert.Equal(t, tt.expected, result, "IsAuthError(%q) should return %v", tt.errMsg, tt.expected) - }) - } -} - func TestIsHexString(t *testing.T) { tests := []struct { name string @@ -246,6 +119,25 @@ func TestIsValidFullSHA(t *testing.T) { } } +func TestIsValidFullSHACaseInsensitive(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {name: "valid lowercase full SHA", input: "abcdef0123456789abcdef0123456789abcdef01", expected: true}, + {name: "valid uppercase full SHA", input: "ABCDEF0123456789ABCDEF0123456789ABCDEF01", expected: true}, + {name: "invalid short SHA", input: "abcdef0", expected: false}, + {name: "invalid non-hex character", input: "abcdef0123456789abcdef0123456789abcdef0g", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, IsValidFullSHACaseInsensitive(tt.input)) + }) + } +} + func TestIsGitObjectID(t *testing.T) { tests := []struct { name string diff --git a/pkg/gitutil/spec_test.go b/pkg/gitutil/spec_test.go index a3b569580fa..0a44a0190a4 100644 --- a/pkg/gitutil/spec_test.go +++ b/pkg/gitutil/spec_test.go @@ -12,117 +12,6 @@ import ( "github.com/github/gh-aw/pkg/gitutil" ) -// TestSpec_PublicAPI_IsRateLimitError validates the documented behavior of -// IsRateLimitError as described in the package README.md. -// -// Specification: Returns true when errMsg indicates a GitHub API rate-limit -// error (case-insensitive match against "api rate limit exceeded", -// "rate limit exceeded", or "secondary rate limit"). -func TestSpec_PublicAPI_IsRateLimitError(t *testing.T) { - tests := []struct { - name string - errMsg string - expected bool - }{ - { - name: "documented phrase 'api rate limit exceeded' returns true", - errMsg: "403: API rate limit exceeded", - expected: true, - }, - { - name: "documented phrase 'rate limit exceeded' returns true", - errMsg: "rate limit exceeded for user ID 123", - expected: true, - }, - { - name: "documented phrase 'secondary rate limit' returns true", - errMsg: "secondary rate limit triggered", - expected: true, - }, - { - name: "case-insensitive match returns true (documented as case-insensitive)", - errMsg: "API RATE LIMIT EXCEEDED", - expected: true, - }, - { - name: "unrelated error message returns false", - errMsg: "404: not found", - expected: false, - }, - { - name: "empty string returns false", - errMsg: "", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := gitutil.IsRateLimitError(tt.errMsg) - assert.Equal(t, tt.expected, result, - "IsRateLimitError(%q) should match documented behavior", tt.errMsg) - }) - } -} - -// TestSpec_PublicAPI_IsAuthError validates the documented behavior of -// IsAuthError as described in the package README.md. -// -// Specification: Returns true when errMsg indicates an authentication or -// authorization failure (GH_TOKEN, GITHUB_TOKEN, unauthorized, forbidden, -// SAML enforcement, etc.). -func TestSpec_PublicAPI_IsAuthError(t *testing.T) { - tests := []struct { - name string - errMsg string - expected bool - }{ - { - name: "GH_TOKEN reference returns true", - errMsg: "GH_TOKEN is invalid or expired", - expected: true, - }, - { - name: "GITHUB_TOKEN reference returns true", - errMsg: "GITHUB_TOKEN: authentication failed", - expected: true, - }, - { - name: "unauthorized returns true", - errMsg: "401: unauthorized", - expected: true, - }, - { - name: "forbidden returns true", - errMsg: "403: forbidden", - expected: true, - }, - { - name: "SAML enforcement message returns true (documented)", - errMsg: "Resource protected by organization SAML enforcement", - expected: true, - }, - { - name: "unrelated error returns false", - errMsg: "404: not found", - expected: false, - }, - { - name: "empty string returns false", - errMsg: "", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := gitutil.IsAuthError(tt.errMsg) - assert.Equal(t, tt.expected, result, - "IsAuthError(%q) should match documented behavior", tt.errMsg) - }) - } -} - // TestSpec_PublicAPI_IsHexString validates the documented behavior of // IsHexString as described in the package README.md. // @@ -275,6 +164,27 @@ func TestSpec_PublicAPI_IsValidFullSHA(t *testing.T) { } } +// TestSpec_PublicAPI_IsValidFullSHACaseInsensitive validates the documented +// behavior of IsValidFullSHACaseInsensitive as described in the package README.md. +func TestSpec_PublicAPI_IsValidFullSHACaseInsensitive(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {name: "40-character lowercase hex returns true", input: "da39a3ee5e6b4b0d3255bfef95601890afd80709", expected: true}, + {name: "40-character uppercase hex returns true", input: "DA39A3EE5E6B4B0D3255BFEF95601890AFD80709", expected: true}, + {name: "39 characters returns false", input: "da39a3ee5e6b4b0d3255bfef95601890afd807", expected: false}, + {name: "non-hex character returns false", input: "za39a3ee5e6b4b0d3255bfef95601890afd80709", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, gitutil.IsValidFullSHACaseInsensitive(tt.input)) + }) + } +} + // TestSpec_PublicAPI_FindGitRoot validates the documented behavior of // FindGitRoot as described in the package README.md. // diff --git a/pkg/parser/remote_download_file.go b/pkg/parser/remote_download_file.go index 80d10fc9d95..12dfbd3f29c 100644 --- a/pkg/parser/remote_download_file.go +++ b/pkg/parser/remote_download_file.go @@ -56,7 +56,7 @@ var downloadFileViaGitFunc = downloadFileViaGit func downloadFileFromGitHubWithDepth(ctx context.Context, owner, repo, path, ref string, symlinkDepth int, host string) ([]byte, error) { client, err := createRESTClientForHostFunc(host) if err != nil { - if gitutil.IsAuthError(err.Error()) { + if errorutil.IsAuthError(err.Error()) { remoteLog.Printf("REST client creation failed due to auth error, attempting git fallback for %s/%s/%s@%s: %v", owner, repo, path, ref, err) content, gitErr := downloadFileViaGitFunc(ctx, owner, repo, path, ref, host) if gitErr != nil { @@ -76,7 +76,7 @@ func downloadFileFromGitHubWithDepth(ctx context.Context, owner, repo, path, ref err = fetchRemoteFileContentFunc(ctx, client, owner, repo, path, ref, &fileContent) if err != nil { - if gitutil.IsAuthError(err.Error()) { + if errorutil.IsAuthError(err.Error()) { remoteLog.Printf("GitHub API authentication failed, attempting git fallback for %s/%s/%s@%s", owner, repo, path, ref) content, gitErr := downloadFileViaGitFunc(ctx, owner, repo, path, ref, host) if gitErr != nil { @@ -399,7 +399,7 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s defer os.RemoveAll(tmpDir) repoURL := getRepoGitURL(owner, repo, host) - if len(ref) == 40 && gitutil.IsHexString(ref) { + if gitutil.IsValidFullSHACaseInsensitive(ref) { if err := cloneAndCheckoutSHA(ctx, repoURL, tmpDir, ref); err != nil { return nil, err } diff --git a/pkg/parser/remote_fetch_test.go b/pkg/parser/remote_fetch_test.go index 3f491ecafea..0b9c8b4941c 100644 --- a/pkg/parser/remote_fetch_test.go +++ b/pkg/parser/remote_fetch_test.go @@ -65,6 +65,18 @@ func TestBuildCommitLookupAPIPath(t *testing.T) { }) } +func TestResolveRefToSHAAcceptsUppercaseFullSHA(t *testing.T) { + const ref = "EA222E359276C0702A5F5203547FF9D88D0DDD76" + + sha, err := resolveRefToSHA(context.Background(), "owner", "repo", ref, "github.com") + if err != nil { + t.Fatalf("resolveRefToSHA() error = %v", err) + } + if sha != ref { + t.Fatalf("resolveRefToSHA() = %q, want %q", sha, ref) + } +} + func TestBuildContentsAPIPath(t *testing.T) { t.Run("escapes refs with reserved query chars", func(t *testing.T) { got := buildContentsAPIPath("owner", "repo", ".github/workflows/demo.md", "release+candidate#1") diff --git a/pkg/parser/remote_list_files.go b/pkg/parser/remote_list_files.go index c44a4131f6a..47b735000bd 100644 --- a/pkg/parser/remote_list_files.go +++ b/pkg/parser/remote_list_files.go @@ -15,7 +15,7 @@ import ( "sync" "github.com/cli/go-gh/v2/pkg/api" - "github.com/github/gh-aw/pkg/gitutil" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/stringutil" "golang.org/x/sync/singleflight" ) @@ -199,7 +199,7 @@ func listWorkflowFilesForHost(ctx context.Context, owner, repo, ref, workflowPat errStr := err.Error() // Check if this is an authentication error - if gitutil.IsAuthError(errStr) { + if errorutil.IsAuthError(errStr) { remoteLog.Printf("GitHub API authentication failed, attempting git fallback for %s/%s@%s", owner, repo, ref) // Try fallback using git commands for public repositories files, gitErr := listWorkflowFilesViaGitForHost(ctx, owner, repo, ref, workflowPath, host) @@ -321,7 +321,7 @@ func listDirAllFilesForHost(ctx context.Context, owner, repo, ref, dirPath, host err = client.DoWithContext(ctx, http.MethodGet, endpoint, nil, &contents) if err != nil { errStr := err.Error() - if gitutil.IsAuthError(errStr) { + if errorutil.IsAuthError(errStr) { remoteLog.Printf("GitHub API auth failed, attempting git fallback for %s/%s@%s", owner, repo, ref) files, gitErr := listDirAllFilesViaGitForHost(ctx, owner, repo, ref, dirPath, host) if gitErr != nil { @@ -427,7 +427,7 @@ func listDirAllFilesRecursivelyForHost(ctx context.Context, owner, repo, ref, di files, err := listContentsRecursively(ctx, client, owner, repo, ref, dirPath) if err != nil { errStr := err.Error() - if gitutil.IsAuthError(errStr) { + if errorutil.IsAuthError(errStr) { remoteLog.Printf("GitHub API auth failed, attempting git fallback for %s/%s@%s", owner, repo, ref) gitFiles, gitErr := listDirAllFilesRecursivelyViaGitForHost(ctx, owner, repo, ref, dirPath, host) if gitErr != nil { @@ -541,7 +541,7 @@ func listDirSubdirsForHost(ctx context.Context, owner, repo, ref, dirPath, host err = client.DoWithContext(ctx, http.MethodGet, endpoint, nil, &contents) if err != nil { errStr := err.Error() - if gitutil.IsAuthError(errStr) { + if errorutil.IsAuthError(errStr) { remoteLog.Printf("GitHub API auth failed, attempting git fallback for %s/%s@%s", owner, repo, ref) dirs, gitErr := listDirSubdirsViaGitForHost(ctx, owner, repo, ref, dirPath, host) if gitErr != nil { diff --git a/pkg/parser/remote_resolve_sha.go b/pkg/parser/remote_resolve_sha.go index 3efab65280f..7c90516a86f 100644 --- a/pkg/parser/remote_resolve_sha.go +++ b/pkg/parser/remote_resolve_sha.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/cli/go-gh/v2/pkg/api" + "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/gitutil" ) @@ -75,7 +76,7 @@ func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) ( sha := parts[0] // Validate it's a valid SHA - if len(sha) != 40 || !gitutil.IsHexString(sha) { + if !gitutil.IsValidFullSHACaseInsensitive(sha) { return "", fmt.Errorf("invalid SHA format from git ls-remote: %s", sha) } @@ -86,13 +87,13 @@ func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) ( // resolveRefToSHA resolves a git ref (branch, tag, or SHA) to its commit SHA func resolveRefToSHA(ctx context.Context, owner, repo, ref, host string) (string, error) { // If ref is already a full SHA (40 hex characters), return it as-is - if len(ref) == 40 && gitutil.IsHexString(ref) { + if gitutil.IsValidFullSHACaseInsensitive(ref) { return ref, nil } client, err := createRESTClientForHostFunc(host) if err != nil { - if gitutil.IsAuthError(err.Error()) { + if errorutil.IsAuthError(err.Error()) { remoteLog.Printf("REST client creation failed due to auth error, attempting git ls-remote fallback for %s/%s@%s: %v", owner, repo, ref, err) sha, gitErr := resolveRefToSHAViaGitFunc(ctx, owner, repo, ref, host) if gitErr != nil { @@ -152,7 +153,7 @@ func resolveRefToSHAWithFallbacks( } // Validate it's a valid SHA (40 hex characters) - if len(sha) != 40 || !gitutil.IsHexString(sha) { + if !gitutil.IsValidFullSHACaseInsensitive(sha) { return "", fmt.Errorf("invalid SHA format returned: %s", sha) } @@ -204,7 +205,7 @@ func resolveRefToSHAViaPublicAPI(ctx context.Context, owner, repo, ref string) ( if err := json.Unmarshal(body, &result); err != nil { return "", fmt.Errorf("failed to parse commit response: %w", err) } - if result.SHA == "" || len(result.SHA) != 40 || !gitutil.IsHexString(result.SHA) { + if !gitutil.IsValidFullSHACaseInsensitive(result.SHA) { return "", fmt.Errorf("invalid SHA returned from public API: %q", result.SHA) } return result.SHA, nil diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 975e7653d39..816042c9fed 100644 --- a/pkg/workflow/action_resolver.go +++ b/pkg/workflow/action_resolver.go @@ -184,7 +184,7 @@ func ParseTagRefTSV(line string) (sha, objType string, err error) { } sha = parts[0] objType = parts[1] - if len(sha) != 40 || !gitutil.IsHexString(sha) { + if !gitutil.IsValidFullSHACaseInsensitive(sha) { return "", "", fmt.Errorf("invalid SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) } return sha, objType, nil diff --git a/pkg/workflow/action_resolver_test.go b/pkg/workflow/action_resolver_test.go index 23b97e759e8..bfbf3e0b03b 100644 --- a/pkg/workflow/action_resolver_test.go +++ b/pkg/workflow/action_resolver_test.go @@ -204,6 +204,12 @@ func TestParseTagRefTSV(t *testing.T) { wantSHA: commitSHA, wantType: "commit", }, + { + name: "uppercase SHA is accepted", + input: "EA222E359276C0702A5F5203547FF9D88D0DDD76\tcommit", + wantSHA: "EA222E359276C0702A5F5203547FF9D88D0DDD76", + wantType: "commit", + }, { name: "empty input is rejected", input: "", diff --git a/pkg/workflow/step_types.go b/pkg/workflow/step_types.go index 13211b1485e..3223fed6c6b 100644 --- a/pkg/workflow/step_types.go +++ b/pkg/workflow/step_types.go @@ -1,14 +1,12 @@ package workflow import ( - "encoding/json" "errors" "fmt" "maps" - "reflect" - "sort" "strconv" + "github.com/github/gh-aw/pkg/importinpututil" "github.com/github/gh-aw/pkg/logger" ) @@ -238,48 +236,16 @@ func StepsToSlice(steps []*WorkflowStep) []any { // marshalEnvValue serializes a non-string env var value to a string suitable // for use in a GitHub Actions step env block. -// Arrays and maps are serialized as JSON (e.g. ["a","b"]) so that shell -// consumers such as `jq --argjson` receive valid JSON. -// Typed slices produced by goccy/go-yaml (e.g. []string instead of []any) -// are normalized via reflection before marshaling. -// Scalar values (int, bool, float64, etc.) fall back to fmt.Sprint. +// Arrays and maps are serialized as JSON (e.g. ["a","b"]) via +// importinpututil.FormatResolvedValue so import substitutions and env +// serialization stay aligned. Scalar values (int, bool, float64, etc.) +// fall back to fmt.Sprint. func marshalEnvValue(v any) string { - switch val := v.(type) { - case []any: - if b, err := json.Marshal(val); err == nil { - return string(b) - } - case map[string]any: - if b, err := json.Marshal(val); err == nil { - return string(b) - } - case nil: + if v == nil { return "" - default: - rv := reflect.ValueOf(v) - switch rv.Kind() { - case reflect.Slice: - normalized := make([]any, rv.Len()) - for i := range rv.Len() { - normalized[i] = rv.Index(i).Interface() - } - if b, err := json.Marshal(normalized); err == nil { - return string(b) - } - 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) - } - } + } + if s, ok := importinpututil.FormatResolvedValue(v); ok { + return s } return fmt.Sprint(v) }