ci: comment-cop should scan the live base...head compare, not the PR's cached file list - #38127
ci: comment-cop should scan the live base...head compare, not the PR's cached file list#38127robobun wants to merge 1 commit into
Conversation
comment-cop took its file list from pulls.listFiles. Right after a rebase or a merge from main, that list can still be relative to the base the PR was opened against, so it contains every file main has touched since then and the workflow flags comments the PR never added. It posted one review comment per group with no bound, so each such run kept posting until the 5 minute job timeout cancelled it (about 120 comments per push). Take the diff from repos.compareCommitsWithBasehead(base.ref...head.sha), which GitHub computes from the current merge base at request time, and post at most 25 new comments per run, logging a warning when capped.
|
Status: reproduced and fixed.
|
|
Warning Review limit reached
Next review available in: 4 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted CI tooling fix that swaps the stale PR file cache for a live compare and caps per-run posts.
What was reviewed:
compareCommitsWithBaseheadresponse shape (files[].filename/status/patch) matches what the existing parser and dedup key consume, so downstream logic is unchanged.- Fork-PR head SHAs are reachable in the base repo via
refs/pull/N/head, so the compare againstoven-sh/bunresolves them; base refs containing/are path-encoded by octokit. per_page: 1only pages the commit list; the ≤300 files all arrive on the first page, and the 300-file ceiling is acceptable for a best-effort lint on claude-labeled PRs.- The
.slice(0, MAX_COMMENTS_PER_RUN)cap composes correctly with the existing content-hash dedup, so overflow groups are picked up on the next push rather than lost.
Extended reasoning...
Overview
Single-file change to .github/workflows/comment-cop.yml, the internal bot that flags multi-line code comments on claude-labeled PRs. Two functional edits: (1) source the changed-file list from repos.compareCommitsWithBasehead(${base.ref}...${head.sha}) instead of pulls.listFiles, because the latter can return a stale, massively inflated diff right after a rebase/merge-from-main; (2) cap new review comments to 25 per run with a core.warning when truncating. A diagnostic core.info line logs the compare's file count and merge base.
Security risks
None introduced. The workflow already runs on pull_request_target with pull-requests: write and never checks out PR code. The only new input read from the event payload is base.ref, which is controlled by the base repo, not the PR author. No shell interpolation, no new tokens, no widened permissions.
Level of scrutiny
Low. This is repo-internal CI tooling with a bounded, reversible effect (posting/resolving review comments). The failure mode being fixed is comment spam; the worst case for the new code is under-posting (300-file compare ceiling, 25-comment cap) or a failed API call, both of which degrade to "fewer nag comments" rather than anything user-facing or build-affecting. It cannot be exercised by this PR's own CI (pull_request_target runs base-branch code), and the PR description documents dry-runs against several live PRs — including one with a currently-stale cached diff — showing identical output on accurate PRs and correct output on the pathological ones.
Other factors
The response-shape compatibility (filename, status, patch) means groupsFromPatch, keyFor, and the auto-resolve logic are untouched and continue to work; compare.files ?? [] guards the one field that can be absent. The 25-per-run cap interacts safely with the existing seenKeys dedup: unposted groups remain in the diff and are posted on the next synchronize. No prior reviewer comments to address; the bug-hunting pass found nothing.
|
Updated 3:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 3259e6f has some failures in 🧪 To try this PR locally: bunx bun-pr 38127That installs a local version of the PR into your bun-38127 --bun |
2 participants
What does this PR do?
Problem
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrongreview threads on files a PR does not touch. On fetch: reject network errors as TypeError('fetch failed' / 'terminated') with the system error as cause #35988 two runs (31678330598, 31680070931) posted 240 threads across 67 files, none of them in the PR's 20-file diff. bundler: merge duplicate external ESM imports across the files of a chunk #35635 (6-file PR, 172 such threads), js_parser: lower undecorated auto-accessors to a native #-private storage field #35708 (2 files, 154), bundler: resolve require('bindings')(name) to the package's .node addon at build time #36713 (11 files, 123), watcher(windows): watch workspace packages outside the project root #35596 (8 files, 65) and transpiler: compile zod v4 schemas into lazy wrappers with flat validators #36956 (21 files, 59) got the same treatment..github/workflows/comment-cop.yml:92: the file list comes frompulls.listFiles. The workflow runs onsynchronize, and right after a rebase or a merge from main the PR's cached diff can still be relative to the base the PR was opened against, so it lists every file main touched since then. fetch: reject network errors as TypeError('fetch failed' / 'terminated') with the system error as cause #35988 reportedchanged_files: 3133against a base from July 25 (git diff 44f6469e..90ef0cdcis exactly 3133 files) for over an hour; bundler: resolve require('bindings')(name) to the package's .node addon at build time #36713 still reports 1946 files against an August 1 base while its live diff is 11 files..github/workflows/comment-cop.yml:170: onecreateReviewCommentper group with nothing bounding the count, so every such run posted until the job'stimeout-minutes: 5cancelled it (about 120 threads per push, both runs above endedcancelled). Because the inflated list is walked in path order, the run was usually cancelled before reaching the files the PR did change, so the comments it exists to post were not posted either.Fix
repos.compareCommitsWithBaseheadwithbase.ref...head.sha. GitHub computes that from the current merge base at request time, so it is the PR's actual diff no matter what the PR's cached diff says; the file entries have the same shape (filename,status,patch) aspulls.listFiles, so the parser and the dedup keys are unchanged. Requested withper_page: 1: the changed files all come on the first page (at most 300),per_pageonly pages the commit list.MAX_COMMENTS_PER_RUN), with a warning annotation when the cap is hit, so a run's write volume is bounded by design instead of by the job timeout. Groups left over are still in the diff on the next push and get posted then (dedup is per group, unchanged). Of about 30 claude PRs I scanned with the new script, all but one have 12 or fewer groups in their real diff; the exception, watcher(windows): watch workspace packages outside the project root #35596, has 40 (33 of them in one file) and would get 25 on the first run and the other 15 on the push after.presentKeysnow comes from the real diff, the existing auto-resolve step identifies the bogus threads as stale (240 on fetch: reject network errors as TypeError('fetch failed' / 'terminated') with the system error as cause #35988) and resolves them once ci: use a PAT to resolve stale comment-cop review threads #36959 gives it a token that can.How did you verify your code works?
The workflow runs on
pull_request_targetfrom the base branch, so CI on this PR does not exercise it. I dry-ran the embedded script (extracted from the YAML, real GitHub reads,createReviewCommentand the resolve mutation recorded instead of sent) against live PRs:changed_files: 1946, live compare 11 files): the script on main would post 1832 comments across 396 files; this branch's script posts 7 comments across 5 files, all in the PR's diff.repos.compareCommitsWithBaseheadis present in the pinnedactions/github-scriptbundle (GET /repos/{owner}/{repo}/compare/{basehead}); a fork PR's head SHA works as the head of the compare (checked against fix(types): correct FormData values()/entries() iterator types #34264 and install: serve lifecycle node-gyp from the install cache after EXDEV #38080), and octokit's percent-encoding of a base branch containing/is accepted by the endpoint. Prettier passes on the file.Does not overlap with #37948 (which groups count as a comment) or #36959 (token used to resolve threads); both touch other parts of the same script.
Per-PR audit of existing Comment Cop threads against each PR's live compare
For #35635, #35708 and #36713, 43 of 45, 53 of 54 and 37 of 40 of the commented-on paths were never touched by any non-merge commit on the branch (
git log --no-merges --name-only origin/main..<head>), and the threads were posted in one burst per push lasting until the 5 minute timeout (for example #35635: 160 threads between 03:08 and 03:13 UTC on Aug 13, right after a rebase).pr.changed_filesfor most of these has since been recomputed to the right number; the threads were posted while it was not.