Report GitHub API rate limits in logs output - #58793
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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.
🟡 Changes recommended
Empty-host resolution can incorrectly query and label GitHub.com instead of the configured GHES host.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds GitHub API quota snapshots and low-quota warnings to logs reporting.
Changes:
- Captures start and end core API quotas per host.
- Includes quota data across JSON output paths.
- Warns non-JSON users when 20% or less remains.
Required change: Resolve the effective configured or current-repository host when no host is provided instead of defaulting directly to github.com, which misreports GHES usage.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_report.go |
Adds rate-limit report fields. |
pkg/cli/logs_rate_limit.go |
Implements quota capture, host handling, and warnings. |
pkg/cli/logs_rate_limit_test.go |
Tests reports, warnings, and host deduplication. |
pkg/cli/logs_orchestrator.go |
Wires reporting into standard downloads. |
pkg/cli/logs_orchestrator_types.go |
Carries reports through rendering. |
pkg/cli/logs_orchestrator_stdin.go |
Adds stdin and multi-host reporting. |
pkg/cli/logs_orchestrator_render.go |
Includes quotas in rendered output. |
pkg/cli/logs_orchestrator_download.go |
Includes quotas in empty results. |
pkg/cli/logs_multi.go |
Tracks quotas across workflow targets. |
pkg/cli/logs_models.go |
Defines the warning threshold. |
pkg/cli/audit_cross_run.go |
Adds quotas to cross-run reports. |
Review details
Suppressed comments (2)
pkg/cli/logs_rate_limit.go:199
- Omitting
--hostnameforgithub.comstill lets an inheritedGH_HOSTredirect this supposedly public-host snapshot to GHES. In a mixed-host report, that can duplicate the enterprise quota under thegithub.comlabel. Pin every non-empty host here;fetchRateLimit(ctx)continues to pass an empty host for callers that intentionally use the configured default.
args := []string{"api", "rate_limit"}
if host != "" && host != "github.com" {
args = append(args, "--hostname", host)
}
pkg/cli/logs_rate_limit.go:153
- The
--repocontract is[HOST/]owner/repoand does not require the host to contain a dot (seepkg/cli/flags.go:27and the stdin parser above). For a valid single-label GHES host such asgithub/owner/repo, this falls back togithub.com, so snapshots and low-quota warnings report the wrong account. Treat any three-part repo override as host-qualified.
func logsRateLimitHost(repoOverride string) string {
parts := strings.SplitN(repoOverride, "/", 3)
if len(parts) == 3 && strings.Contains(parts[0], ".") {
return parts[0]
}
return ""
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
| func normalizedGitHubAPIHost(host string) string { | ||
| if host == "" { | ||
| return "github.com" | ||
| } | ||
| return host |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd to the new rate-limit reporting code. There's already an existing review comment from Copilot on the host-resolution logic (logs_rate_limit.go:145) that I agree is worth addressing. My two additional comments are minor and non-blocking.
📋 Key Themes & Highlights
Key Themes
- Naming duplication:
rateLimitResourceis kept as a type alias for the newly-introducedGitHubAPIRateLimitState, giving one concept two names in the same package. - Partition boundary untested:
partitionGitHubAPIRateLimitReports's single-vs-multi-host branching isn't directly unit tested.
Positive Highlights
- ✅ Good test coverage overall for warn/suppress/omit-on-error behavior (
logs_rate_limit_test.go). - ✅ Sensible JSON shape (
start/endsnapshots) and clean threshold constant with a guarding unit test. - ✅ Host normalization and multi-host support are handled consistently across the three logs entry points.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 33.3 AIC · ⌖ 17.4 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/logs_rate_limit.go:304
[/codebase-design] Introducing type rateLimitResource = GitHubAPIRateLimitState gives the same concept two public-facing names in one package, which adds navigation friction (readers must learn both rateLimitResource and GitHubAPIRateLimitState refer to the same struct). This looks like it exists only to avoid touching call sites that still use rateLimitResource.
<details>
<summary>💡 Suggestion</summary>
Since this PR already touches every call site of fetchRateLimit/`fetchRate…
pkg/cli/logs_rate_limit.go:398
[/tdd] partitionGitHubAPIRateLimitReports (which decides whether a single-host report goes on GitHubAPIRateLimit or falls back to the GitHubAPIRateLimits array) has no direct test — only its callers are exercised indirectly through multi-host fixtures. The 2-vs-1 boundary is exactly the kind of edge case that regresses silently.
<details>
<summary>💡 Suggested test</summary>
func TestPartitionGitHubAPIRateLimitReports(t *testing.T) {
one := populatedGitHubAPIRateLimitReport(&…
</details>
🏗️ ADR RequiredI inferred an architectural decision from this PR and added a draft ADR at Evidence used
Gate result
Next action
|
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.
|
There was a problem hiding this comment.
Verdict
No blocking issues jumped out in the changed lines.
Why this stays non-blocking
The new rate-limit reporting is wired through the single-run, stdin, multi-target, and cross-run JSON paths consistently, and the one candidate issue from the sub-agent does not hold up: the all-invalid-stdin branch already emits the JSON payload before returning. I did not find a changed-line correctness, crash, or quota-handling regression that clearly warrants blocking the PR.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.9 AIC · ⌖ 7.34 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
The new rate-limit reporting adds a couple of helper layers that can be simplified without changing behavior. net: -16 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58793 · codex · mai10 · 7.97 AIC · ⌖ 13.3 AIC · ⊞ 13.5K
Comment /ponytail to run again
| int64(state.Remaining)*100 <= int64(state.Limit)*int64(RateLimitWarningThresholdPercent) | ||
| } | ||
|
|
||
| func populatedGitHubAPIRateLimitReport(report *GitHubAPIRateLimitReport) *GitHubAPIRateLimitReport { |
There was a problem hiding this comment.
pkg/cli/logs_rate_limit.go:L161: yagni: three tiny helpers that only filter nil reports and choose a single report. Inline the nil handling at the call sites and drop this mini-API.
| return fmt.Errorf("invalid repository format '%s': expected '[HOST/]owner/repo'", opts.RepoOverride) | ||
| } | ||
| } | ||
| rateLimitHosts := make([]string, 0, len(opts.RunURLs)) |
There was a problem hiding this comment.
pkg/cli/logs_orchestrator_stdin.go:L80: yagni: a host-dedup loop with a map just to build a short list of rate-limit hosts. A single pass or a direct fallback would keep this branch simpler.
|
🎉 This pull request is included in a new release. Release: |
Logs commands did not expose their API quota consumption or warn when quota was running low.
Changes
{ "github_api_rate_limit": { "host": "github.com", "start": { "limit": 5000, "remaining": 4200, "used": 800 }, "end": { "limit": 5000, "remaining": 4100, "used": 900 } } }