Repository navigation
Refactor shared value formatting and unify GitHub error/SHA classification paths #53018
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,38 @@ func IsGoneError(err error) bool { | |
| return matched | ||
| } | ||
|
|
||
| // IsRateLimitError reports whether output indicates a GitHub API rate-limit error. | ||
|
Contributor
There was a problem hiding this comment. [/improve-codebase-architecture] 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 💡 Why this mattersIf someone later adds a string-specific exclusion or a more restrictive matcher, the documented phrases should be independently testable. A deduplication comment in // "rate limit exceeded" is a suffix of "api rate limit exceeded" — one pattern covers both.@copilot please address this.
Contributor
Author
There was a problem hiding this comment. The redundant API-specific phrase remains covered by the canonical |
||
| // 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: %s", output) | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] Debug log may leak sensitive content: the full 💡 Suggested fixLog 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 @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Updated both shared classifiers to log only the output length in aaec1b1. |
||
| } | ||
| return matched | ||
| } | ||
|
|
||
| // IsAuthError reports whether output indicates an authentication or | ||
| // authorization issue from the GitHub API or gh CLI. | ||
| func IsAuthError(output string) bool { | ||
|
Contributor
There was a problem hiding this comment.
💡 Why this blocks mergeThe 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 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 |
||
| matched := containsSubstring(output, | ||
| "gh_token", | ||
| "github_token", | ||
| "authentication", | ||
| "not logged into", | ||
| "unauthorized", | ||
| "forbidden", | ||
| "permission denied", | ||
| "saml enforcement", | ||
| ) | ||
| if matched { | ||
| errorutilLog.Printf("Classified output as auth-related: %s", output) | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] Same sensitive-log concern applies to 💡 Suggested fixif matched {
errorutilLog.Printf("Classified output as auth-related (len=%d)", len(output))
}@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Updated both shared classifiers to log only the output length in aaec1b1. |
||
| } | ||
| return matched | ||
| } | ||
|
|
||
| // containsErrorSubstring reports whether err contains any of the provided | ||
| // substrings after lowercasing the full error message for case-insensitive | ||
| // matching. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
isPermissionErrorStrinaudit.gonow lowercases the input before matching, but the existingerrorutil.IsAuthError(which it delegates to first) also lowercases internally. This is correct and harmless, but the audit-specific branch that follows usesstrings.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 inerrorutil.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
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added coverage for the legacy GitHub CLI authentication marker in aaec1b1.