Refactor shared value formatting and unify GitHub error/SHA classification paths - #53018
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors shared formatting, error classification, and SHA validation to use centralized utilities.
Changes:
- Delegates environment-value formatting to
importinpututil. - Moves GitHub error classifiers into
errorutiland updates callers. - Standardizes full-SHA checks and updates related tests/docs.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/step_types.go |
Reuses shared value formatting. |
pkg/workflow/action_resolver.go |
Uses canonical SHA validation. |
pkg/parser/remote_resolve_sha.go |
Centralizes error and SHA checks. |
pkg/parser/remote_list_files.go |
Uses shared auth classification. |
pkg/parser/remote_download_file.go |
Updates auth and SHA checks. |
pkg/gitutil/spec_test.go |
Removes relocated classifier specs. |
pkg/gitutil/README.md |
Updates package responsibilities. |
pkg/gitutil/gitutil.go |
Removes error classifiers. |
pkg/gitutil/gitutil_test.go |
Removes relocated classifier tests. |
pkg/errorutil/spec_test.go |
Adds classifier contract tests. |
pkg/errorutil/README.md |
Documents new classifier APIs. |
pkg/errorutil/errors.go |
Adds shared classifiers. |
pkg/errorutil/errors_test.go |
Tests classifier behavior. |
pkg/cli/update_workflows.go |
Migrates classifier calls. |
pkg/cli/update_display.go |
Migrates failure grouping. |
pkg/cli/update_actions_release.go |
Migrates auth fallback detection. |
pkg/cli/health_command.go |
Migrates rate-limit detection. |
pkg/cli/forecast_resolution.go |
Migrates retry classification. |
pkg/cli/forecast_compute.go |
Migrates rate-limit handling. |
pkg/cli/download_workflow.go |
Centralizes auth and SHA checks. |
pkg/cli/audit.go |
Aligns permission classification. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| // 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 { |
| if !gitutil.IsValidFullSHA(sha) { | ||
| return "", "", fmt.Errorf("invalid SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR is a net simplification (-369/+199 lines): it deduplicates value-formatting logic into importinpututil.FormatResolvedValue and consolidates error classifiers into errorutil, deleting redundant code from gitutil/workflow. No new abstractions, dependencies, or speculative flexibility were introduced. Existing review comments already cover the two correctness concerns (removed public API, error message wording), which are out of scope for this over-engineering-only pass.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This cleanup is not behavior-preserving: it narrows accepted SHA inputs and broadens auth-error detection enough to send unrelated 403s down the wrong fallback paths.
Blocking themes
- Replacing
len(x) == 40 && IsHexString(x)withIsValidFullSHAchanges user-visible behavior by rejecting uppercase full SHAs that previously worked. - The new shared
IsAuthErrorclassifier treats genericforbiddenresponses as auth failures, which changes control flow in parser/CLI fallback code and can mask the real error.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11.3 AIC · ⌖ 6.6 AIC · ⊞ 6.9K
Comment /review to run again
| 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.IsValidFullSHA(ref) { |
There was a problem hiding this comment.
This refactor silently changes the accepted meaning of a “full SHA”: the old code accepted any 40-character hex string, but IsValidFullSHA only accepts lowercase, so callers like resolveRefToSHA now reject uppercase commit IDs that previously worked.
💡 Why this blocks merge
Several changed paths (download_workflow, remote_download_file, remote_resolve_sha, action_resolver) used to gate on len(x) == 40 && gitutil.IsHexString(x), and IsHexString explicitly accepts both uppercase and lowercase hex. Replacing those checks with gitutil.IsValidFullSHA is not a mechanical cleanup — it narrows behavior.
That means an input like ABCDEF... now stops being treated as a SHA and falls into branch/tag/API resolution paths, which can produce wrong lookups or hard failures for a value that previously succeeded.
A safe fix is to preserve the old predicate for user-supplied refs, or normalize with strings.ToLower before validating only in places where lowercase-only storage is actually required.
|
|
||
| // IsAuthError reports whether output indicates an authentication or | ||
| // authorization issue from the GitHub API or gh CLI. | ||
| func IsAuthError(output string) bool { |
There was a problem hiding this comment.
IsAuthError now treats every 403 Forbidden as an authentication problem, which will misroute ordinary authorization failures into auth/git-fallback paths and hide the real cause from users.
💡 Why this blocks merge
The old audit helper had extra command-specific heuristics, but the shared classifier is now used in parser and CLI fallback logic to decide whether to retry with git or unauthenticated API calls. Broadening that shared predicate to match bare forbidden means any non-auth 403 — for example repo policy restrictions, disabled endpoints, or feature gating — gets mislabeled as an auth failure.
That changes control flow, not just messaging: callers will take fallback branches intended only for missing/invalid credentials, and the final error becomes misleading when the fallback also fails.
Please tighten the classifier so it only matches credential-specific markers, or require stronger context than a generic forbidden substring before triggering auth recovery.
Add draft Architecture Decision Record for the refactor that moves IsAuthError and IsRateLimitError from pkg/gitutil to pkg/errorutil and delegates marshalEnvValue serialization to importinpututil. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (199 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Summary
This PR refactors error/SHA classification to shared packages, reducing duplication across the codebase. The intent is good, but two issues need attention:
-
Breaking API change (
pkg/gitutil):IsRateLimitErrorandIsAuthErrorare deleted fromgitutiland moved toerrorutil. This breaks any external consumers of thegitutilpublic API without a deprecation shim or compatibility wrapper. The existing review comment onpkg/errorutil/errors.go:65captures this. -
Stale error message (
pkg/workflow/action_resolver.go:189): After switching togitutil.IsValidFullSHA(which uses^[0-9a-f]{40}$— lowercase only), the error message still says "40 hex characters" without clarifying the lowercase constraint, making the message misleading for uppercase SHA input. The existing review comment onpkg/workflow/action_resolver.go:189captures this.
The marshalEnvValue delegation to importinpututil.FormatResolvedValue is a clean unification, and the isPermissionErrorStr refactor in audit.go correctly preserves the original matching behaviour through the shared classifier.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 52.9 AIC · ⌖ 7.93 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /improve-codebase-architecture, and /tdd to this refactor. Requesting changes on two correctness issues (misleading error message, sensitive debug logs) and flagging a dead-code structure and a coverage gap.
📋 Key Themes & Findings
Key Findings
-
Sensitive content in debug logs (
pkg/errorutil/errors.golines 71 & 90):IsRateLimitErrorandIsAuthErrorlog the fulloutputstring on a positive match. Unlike the oldgitutil.IsAuthError(which logged only"Detected authentication error"), these logs may echo credential error messages verbatim into debug output. Should be bounded tolen(output)or a redacted marker. -
Misleading error message (
pkg/workflow/action_resolver.goline 188):IsValidFullSHArequires lowercase hex, but the error message says "expected 40 hex characters" — an uppercase valid-looking SHA will produce a confusing diagnostic. -
Dead-code structure (
pkg/workflow/step_types.goline 243): Theif v == nilguard is logically unreachable afterFormatResolvedValuereturns("", false)for nil. The guard works correctly now but will silently misbehave ifFormatResolvedValueever returns false for non-nil values, since the fallback then skips nil-checking and callsfmt.Sprint(v)on a non-nil value. -
Removed marker without a regression test (
pkg/cli/audit.goline 302):"GitHub CLI authentication"was removed as a literal and is now covered by the"authentication"substring match insideerrorutil.IsAuthError. This is correct, but there's no test inaudit_test.goasserting the legacy phrase still triggersisPermissionErrorStr. -
Redundant rate-limit pattern (
pkg/errorutil/errors.goline 63):"api rate limit exceeded"is a superstring of"rate limit exceeded", making the former pattern unreachable. Minor, but the README documents both as independent phrases.
Positive Highlights
- ✅ Clean elimination of split-brain
IsAuthError/IsRateLimitErrorduplication betweengitutilandparser/cli - ✅ Full spec tests and unit tests added for the new
errorutilpublic API - ✅
IsValidFullSHAconsolidation is thorough — all five call sites replaced consistently - ✅
marshalEnvValuedelegation toimportinpututil.FormatResolvedValuekeeps serialization logic in one place
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 90.6 AIC · ⌖ 9.13 AIC · ⊞ 7.7K
Comment /matt to run again
| "secondary rate limit", | ||
| ) | ||
| if matched { | ||
| errorutilLog.Printf("Classified output as rate-limit related: %s", output) |
There was a problem hiding this comment.
[/codebase-design] Debug log may leak sensitive content: the full output string is written to the log on a positive match, but that string can contain credential fragments from CLI error messages. The original gitutil.IsAuthError logged only a fixed sentinel — not the payload.
💡 Suggested fix
Log a bounded indicator instead of the raw value:
errorutilLog.Printf("Classified output as rate-limit related (len=%d)", len(output))This is consistent with how gitutil.IsAuthError handled it and avoids echoing potentially sensitive error payloads into debug logs.
@copilot please address this.
There was a problem hiding this comment.
Updated both shared classifiers to log only the output length in aaec1b1.
| "saml enforcement", | ||
| ) | ||
| if matched { | ||
| errorutilLog.Printf("Classified output as auth-related: %s", output) |
There was a problem hiding this comment.
[/codebase-design] Same sensitive-log concern applies to IsAuthError: the full output (which often contains the exact credential error message) is logged verbatim when the function returns true. Consider logging only len(output) or a fixed marker here too.
💡 Suggested fix
if matched {
errorutilLog.Printf("Classified output as auth-related (len=%d)", len(output))
}@copilot please address this.
There was a problem hiding this comment.
Updated both shared classifiers to log only the output length in aaec1b1.
| objType = parts[1] | ||
| if len(sha) != 40 || !gitutil.IsHexString(sha) { | ||
| if !gitutil.IsValidFullSHA(sha) { | ||
| return "", "", fmt.Errorf("invalid SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) |
There was a problem hiding this comment.
[/codebase-design] The error message at line 188 says "expected 40 hex characters" but IsValidFullSHA now requires lowercase hex (^[0-9a-f]{40}$). An uppercase 40-character SHA like ABCDEF... would produce a misleading message reporting it got 40 characters yet was still rejected.
💡 Suggested fix
Update the error to reflect the actual constraint:
return "", "", fmt.Errorf("invalid SHA format: expected 40 lowercase hex characters, got %q", sha)Alternatively, accept uppercase and call strings.ToLower before matching, but that changes the semantics so the error message fix is safer.
@copilot please address this.
There was a problem hiding this comment.
Uppercase 40-character SHA values are now accepted in the affected ref-resolution paths via gitutil.IsValidFullSHACaseInsensitive, with regression coverage in aaec1b1.
| // 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 { |
There was a problem hiding this comment.
[/codebase-design] marshalEnvValue now silently returns "" when FormatResolvedValue returns (_, false) and v == nil is false — i.e. it falls through to fmt.Sprint(v). But FormatResolvedValue returns false only for nil; all other types return true. The double-nil guard (if s, ok := ...; ok then if v == nil) therefore creates a dead-code branch: the second if v == nil is unreachable because FormatResolvedValue(nil) returns ("", false) so the first branch doesn't return, but then v == nil is true.
This means nil returns "" correctly, but the structure is confusing — a future FormatResolvedValue change that returns false for non-nil values (e.g. a marshalling error) would silently fall into fmt.Sprint.
💡 Suggested fix
Either log/surface the marshalling failure, or be explicit:
func marshalEnvValue(v any) string {
if v == nil {
return ""
}
if s, ok := importinpututil.FormatResolvedValue(v); ok {
return s
}
return fmt.Sprint(v)
}This makes the nil guard first-class and the fallback readable.
@copilot please address this.
There was a problem hiding this comment.
Moved the nil guard before FormatResolvedValue in aaec1b1.
| strings.Contains(s, "To use GitHub CLI in a GitHub Actions workflow") || | ||
| strings.Contains(s, "gh auth login") | ||
| if errorutil.IsAuthError(s) { | ||
| return true |
There was a problem hiding this comment.
[/tdd] isPermissionErrorStr in audit.go now lowercases the input before matching, but the existing errorutil.IsAuthError (which it delegates to first) also lowercases internally. This is correct and harmless, but the audit-specific branch that follows uses strings.ToLower(s) on a different variable (lower). One subtle gap: the original "GitHub CLI authentication" marker is no longer present — it was removed and not mapped to any matching pattern in errorutil.IsAuthError ("authentication" would match it, but "GitHub CLI authentication" is a distinct phrase). A targeted test for this removed literal would confirm it's still covered.
💡 Suggested test
// In pkg/cli/audit_test.go
assert.True(t, isPermissionErrorStr("GitHub CLI authentication token is missing"), "legacy marker should still match via 'authentication'")@copilot please address this.
There was a problem hiding this comment.
Added coverage for the legacy GitHub CLI authentication marker in aaec1b1.
| return matched | ||
| } | ||
|
|
||
| // IsRateLimitError reports whether output indicates a GitHub API rate-limit error. |
There was a problem hiding this comment.
[/improve-codebase-architecture] IsRateLimitError is documented as matching "api rate limit exceeded", "rate limit exceeded", or "secondary rate limit". The implementation uses containsSubstring which matches "rate limit exceeded" — this is a substring of "api rate limit exceeded", so the two patterns collapse into one. The "api rate limit exceeded" entry in errors_test.go passes because "rate limit exceeded" matches it too.
This redundancy isn't harmful, but it means the first documented phrase has no independent coverage. Consider either removing the redundant literal or documenting that "rate limit exceeded" is the canonical form.
💡 Why this matters
If someone later adds a string-specific exclusion or a more restrictive matcher, the documented phrases should be independently testable. A deduplication comment in errors.go would make the intent explicit:
// "rate limit exceeded" is a suffix of "api rate limit exceeded" — one pattern covers both.@copilot please address this.
There was a problem hiding this comment.
The redundant API-specific phrase remains covered by the canonical rate limit exceeded substring; the documentation now lists that canonical matcher in aaec1b1.
|
@copilot Please address the unresolved review feedback below, refresh the branch if GitHub does not do it automatically, and run the
|
|
Great work on this refactor! 🎯 This PR successfully consolidates duplicated value-formatting logic, unifies error classification across Highlights:
Tests look good — both The diff is well-scoped for a cross-package refactor touching 22 files. Ready for review!
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please address the unresolved review feedback below, refresh the branch if GitHub does not do it automatically, and run the
|
Addressed the SHA and auth-classification feedback in aaec1b1, then refreshed the branch with the latest |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
This refactor removes duplicated value-formatting logic and resolves split-brain GitHub error classification between
gitutil,cli, andparser. It also standardizes full-SHA validation by replacing hand-inlined predicates withgitutil.IsValidFullSHA.Value formatting deduplication (
workflow→importinpututil)marshalEnvValue’s inlined JSON/reflect normalization with delegation toimportinpututil.FormatResolvedValue.fmt.Sprintfallback andnil→"".Error classifier consolidation (
gitutil→errorutil)pkg/errorutilas shared APIs:IsAuthError(output string)IsRateLimitError(output string)pkg/cliandpkg/parserto useerrorutil.pkg/gitutil.Audit permission classifier alignment
isPermissionErrorStrto delegate to shared auth classification and keep audit-only markers (exit status 4,gh auth login, workflow-specific guidance, genericpermission).Full SHA predicate normalization
len(x)==40 && IsHexString(x)checks withgitutil.IsValidFullSHAin resolver/download/action paths.Spec/docs updates
errorutilandgitutilREADMEs and tests to reflect ownership changes and public API contracts.Run: https://github.com/github/gh-aw/actions/runs/31928605740> Generated by 👨🍳 PR Sous Chef · gpt54 · 5.23 AIC · ⌖ 5.69 AIC · ⊞ 6.3K · ◷