Repository navigation
ci: keep comment-cop working when GITHUB_TOKEN's GraphQL quota is exhausted - #39183
Conversation
…austed The scan step fetched the PR's review threads through one unguarded GraphQL query before doing anything else, so whenever the repo's shared GraphQL quota was used up the step died with an unhandled GraphqlResponseError and the check went red on every claude-labeled PR at once. Dedup now reads the PR's review comments over REST, which draws from a separate quota and is the same quota the rest of the step already needs. GraphQL is only used to find the unresolved threads whose flagged block left the diff; that lookup runs only when such a key exists and a failure is a warning, after which the scan still posts. source-lints runs the new test, which executes the embedded script against a fake github object, whenever comment-cop.yml changes.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: fixed, ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — CI-tooling reliability fix with a proper test.
What was reviewed:
- Dedup moved from GraphQL
reviewThreadsto RESTlistReviewComments— same marker keys, different quota; posting path now makes zero GraphQL calls. - The one remaining GraphQL lookup (thread ids for auto-resolve) is gated on
staleKeys.size > 0and wrapped in try/catch that warns instead of failing — auto-resolve is the auxiliary step, so degrading it is correct. - Test extracts the script from the YAML and runs it with the same
AsyncFunction(require, github, context, core, ...)shape asactions/github-script; traced the fixture patch throughgroupsFromPatchand the (2-4)/(6-8) line ranges and keys line up withexpectedPost. pull_request_targetsurface unchanged: no checkout, no permission or trigger changes, only which read endpoint is called.
Extended reasoning...
Overview
The PR touches three files: .github/workflows/comment-cop.yml (moves dedup from a GraphQL reviewThreads query to REST pulls.listReviewComments, gates the remaining GraphQL thread-id lookup on staleKeys.size > 0 and wraps it in a try/catch that emits a core.warning), .github/workflows/source-lints.yml (adds comment-cop.yml to the push/pull_request path filters so the new test runs when the script changes), and a new test/internal/source-lints/comment-cop.test.ts that parses the workflow YAML, evaluates the embedded script exactly as actions/github-script does, and asserts full {posted, resolved, graphqlRequests, restPaginated, warnings, info} objects across four scenarios.
Security risks
This is a pull_request_target workflow with pull-requests: write, so it warrants a careful look. The change does not add a checkout of PR code, does not alter permissions:, does not change the trigger or the if: gate, and does not read any attacker-controlled input beyond what it already read (the PR diff and existing review-comment bodies, which are only regex-matched for the <!-- comment-cop:KEY --> marker). It swaps one read-only GitHub API endpoint for another and adds a try/catch around a best-effort read. No new security surface. CODEOWNERS only covers /.github/CODEOWNERS itself, not .github/workflows/.
Level of scrutiny
Medium — it's CI tooling, not shipped runtime code, and the failure mode of a bug here is a mis-posted or un-resolved bot comment on a PR, not user-facing breakage. That said, I traced the logic carefully because pull_request_target workflows deserve it: seenKeys is now built from REST review-comment bodies instead of GraphQL thread roots (the marker is at the top of every bot comment body, so both endpoints surface the same keys); staleKeys = seenKeys \ presentKeys; GraphQL is only entered when there is at least one stale key and its failure produces a warning naming how many threads went unresolved. The posting path (fresh = groups.filter(g => !seenKeys.has(keyFor(g)))) is unchanged and now sits behind zero GraphQL calls. I also walked the test fixture's hunk through groupsFromPatch by hand — newLine starts at 1 from the @@ +1 header, the leading context line advances it to 2, so the two three-line groups land at 2-4 and 6-8, matching expectedPost(FIRST, 2, 4) / expectedPost(SECOND, 6, 8), and start < end holds so the unconditional start_line/start_side in expectedPost is correct for these fixtures.
Other factors
The test is strong by REVIEW.md standards: it asserts exact toEqual on the whole result object (not toContain), covers both quota states, covers the no-stale-keys path (asserts zero GraphQL requests), covers second-run idempotency by feeding the first run's posted bodies back as review comments, and covers pagination plus resolved/empty/human threads in the GraphQL-available case. The PR description documents that all four tests fail on main and pass here, and includes a dry-run against 9 live PRs showing identical post/resolve decisions vs main. The catch around staleThreadIds() is the correct application of "best-effort auxiliary steps degrade quietly" — auto-resolve is auxiliary; scan/dedup/post are required and are not caught. Bun.YAML.parse is available in the pinned BUN_VERSION: 1.3.14 used by source-lints.
Under the Actions GITHUB_TOKEN every resolveReviewThread mutation fails with "Resource not accessible by integration" (runs 31900717034 and 31900354924 today: 20/20 and 4/4 failed, each followed by a "Resolved N" log line), so keeping the thread lookup behind a try only preserved requests that fail and a misleading log line. With the lookup gone the step makes no GraphQL request at all; dedup comes from the REST review comment list. Resolving stale threads stays with #36959. The test shrinks to the dedup contract with GraphQL unavailable.
There was a problem hiding this comment.
LGTM — swaps the GraphQL thread lookup for REST listReviewComments and drops the auto-resolve loop that never worked under GITHUB_TOKEN.
What was reviewed
- Dedup equivalence:
listReviewCommentsreturns thread root bodies, so the same<!-- comment-cop:KEY -->markers are found;presentKeyswas only used by the removed resolve loop, so dropping it is fine. - Test harness: line ranges (2-4 / 6-8) match
groupsFromPatch's hunk-header parsing;start_lineis asserted and the workflow sets it sinceMIN_LINES=2guaranteesstart < end. source-lints.ymltrigger added on bothpushandpull_requestpaths; no CODEOWNERS on.github/workflows/.
Extended reasoning...
Overview
This PR touches CI tooling only: it replaces the GraphQL reviewThreads query in .github/workflows/comment-cop.yml with a REST pulls.listReviewComments call for deduplication, and deletes the auto-resolve loop (whose resolveReviewThread mutations always fail with Resource not accessible by integration under the Actions GITHUB_TOKEN). It adds test/internal/source-lints/comment-cop.test.ts, which parses the script out of the workflow YAML with Bun.YAML.parse, wraps it in an AsyncFunction the way actions/github-script does, and runs it against a fake github whose GraphQL always throws the observed rate-limit error. .github/workflows/source-lints.yml gains comment-cop.yml in both its push and pull_request path filters so the test runs whenever the script changes.
Security risks
None. This runs under pull_request_target but the change does not introduce any checkout of PR code, does not read new untrusted inputs, and does not change permissions. It strictly reduces API surface (removes GraphQL calls). The test is hermetic — no network, no subprocess, reads only the workflow file from the repo.
Level of scrutiny
Low-to-medium. Comment Cop is an advisory bot check on claude-labeled PRs, not a required merge gate and not part of the shipped runtime. The worst failure mode of a bug here is a duplicate or missed nag comment. The removed functionality (auto-resolve) is documented as non-functional today with linked runs showing every mutation failing, and #36959 is the tracked follow-up for doing it with a capable token.
Other factors
- Dedup correctness: the REST endpoint returns every review comment including thread roots, and every comment-cop comment starts with the marker, so
seenKeysis populated identically to before. The now-unusedpresentKeysset was only consumed by the deleted resolve loop. - The test asserts
graphqlRequests: [], which locks in that the step no longer depends on the GraphQL quota at all — the stated fix is structural, not a try/catch band-aid. - Verified the test's line-number expectations by walking
groupsFromPatchagainst the fixture patch: hunk header+1→ context line advances to 2 → first group at 2-4 →+fnflushes → second group at 6-8. TheexpectedPostshape includesstart_line/start_side, which the workflow sets becauseMIN_LINES = 2guarantees multi-line groups. - The fake
paginateignores the options arg, but both fake endpoints ignore their args too, so the test is self-consistent. - CODEOWNERS only covers
/.github/CODEOWNERSitself, not.github/workflows/. - The PR description includes a dry-run against 9 live PRs showing identical post decisions between main and this branch, plus a scenario matrix; combined with the hermetic test that fails on main and passes here, coverage is solid for a CI script change.
|
Heads-up from #40468: the resolve loop fails for a fixable reason. GitHub gates That changes the trade-off in this PR. Removing the resolve loop drops a feature that works once the permission is right. To keep the quota fix without that loss, keep the resolve loop and put the |
|
Retracting my suggestion above to keep the resolve loop. #40468 is closed. The review of it showed the loop clears nothing in practice: the stale threads get resolved by robobun's own review sweep (all 61 comment-cop threads on the 15 most recent merged |
|
#36959 (the PAT-based resolve step) is now closed in favor of this PR. This is the only open PR that changes how comment-cop deals with stale review threads. |
Problem
Comment Copcheck is red on every claude-labeled PR whenever the repo's GraphQL quota is used up. The step dies before it scans anything:x-ratelimit-resource: graphql,x-ratelimit-limit: 10000,x-ratelimit-remaining: 0(run 31898901053 on http2: resolve BatchSegment to its bytes in one place #39139). All 33 comment-cop failures on Aug 15 are this error (197 runs succeeded, 171 were skipped); they hit unrelated PRs at the same time because the quota is shared by every workflow run in the repo, and they come back whenever PR volume is high..github/workflows/comment-cop.yml:124on main: the step reads the PR's existing review threads withgithub.graphql(), outside anytry, and dedup and posting both sit behind that call. The REST call just before it (pulls.listFiles) had succeeded in each failing run, so the step had the diff; it only lacked the dedup data, which it was reading from the exhausted quota.GITHUB_TOKENeveryresolveReviewThreadmutation fails withResource not accessible by integration, after which the step still logsResolved N stale comment-cop thread(s).Two successful runs from today: 31900717034 (20 attempted, 20 failed, "Resolved 20") and 31900354924 (4 attempted, 4 failed, "Resolved 4"). ci: use a PAT to resolve stale comment-cop review threads #36959 documents the same thing and is the PR that moves resolution to a token that can do it. So the GraphQL query was paying for one thing REST can provide and one thing that does not happen.Fix
pulls.listReviewComments(REST) and collects the<!-- comment-cop:KEY -->markers from them. Every comment the step posts starts with that marker, so the review comments carry the same keys the thread roots did; REST is the quota the step already needs forlistFilesandcreateReviewComment, and it was available in every failing run.Resolved N stale comment-cop thread(s).after they had failed. Resolving stale threads stays with ci: use a PAT to resolve stale comment-cop review threads #36959, which will also need to move its thread lookup into its token step, since this PR removes theGITHUB_TOKENlookup it currently reuses (noted there).test/internal/source-lints/comment-cop.test.tsextracts the script from the workflow and runs it the wayactions/github-scriptdoes, against a fakegithubwhose GraphQL requests all fail with the rate limit error above. It checks that groups not yet flagged are posted with the right line ranges, that a group already flagged (and a stale marker) are left alone, that a second run recognizes the comments the first run posted and posts nothing, and that no GraphQL request is made. Both tests fail against the workflow on main (the script throws the error above) and pass with this change.source-lints.ymlnow also triggers on changes tocomment-cop.yml, so the test runs whenever the script is edited; it is excluded from the Buildkite shards like the rest of that directory.bun bd test test/internal/source-lints/comment-cop.test.ts,bun test test/internal/source-lints/(whole directory green), prettier on the three files. The comment-cop run on this PR itself still executes the script from main (pull_request_target), so theSource lintsjob is the one that exercises the change here.Background
.github/workflows/comment-cop.yml): on each push to a claude-labeled PR it reads the PR diff, finds multi-line comments added undersrc/, and posts one review comment per comment block. Each bot comment starts with<!-- comment-cop:KEY -->, KEY being the file path plus a hash of the block's text; a block whose KEY is already on the PR is not posted again. The check is advisory (not required for merge).x-ratelimit-resourceiscorefor REST andgraphqlfor GraphQL). For theGITHUB_TOKENActions hands out, each quota is per repository, so every workflow run in oven-sh/bun draws on the same two pools, and GraphQL-heavy automation (thegh pr/gh issue/gh searchcommands used by other workflows go through GraphQL) empties the GraphQL pool for everything else when PR volume is high.resolveReviewThreadtakes) and its resolved flag exist.GET /repos/{owner}/{repo}/pulls/{n}/commentsreturns every review comment on the PR, including each thread's root comment, so the markers are reachable from REST; only resolving needs GraphQL, and underGITHUB_TOKENGitHub refuses that mutation regardless of thepull-requests: writepermission.Script on main vs this branch against a fake github (the fake's resolve mutation fails the way GITHUB_TOKEN's does)
Dry-run against live PRs: comments the script on main would post vs the REST dedup (real reads through a user token, writes recorded)
Headers from the failing run