Skip to content

github-tools: paginate PR diff and review-comment fetches - #437

Merged
TheGreatAxios merged 5 commits into
mainfrom
cl-7140-github-tools-pr-diff-and-review-comment-fetches-stop-at-page
Aug 29, 2026
Merged

TheGreatAxios merged 5 commits into
mainfrom
cl-7140-github-tools-pr-diff-and-review-comment-fetches-stop-at-page

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes CL-7140 — https://linear.app/abklabs/issue/CL-7140

Problem

fetchPullRequestDiff (packages/github-tools/src/pull-requests.ts) and
fetchPullRequestReviewComments set per_page=100 and issued a single
request, with no follow of the Link: rel="next" header and no page=
loop. A pull request with more than 100 changed files silently lost
files 101+; more than 100 review comments silently lost the rest, and
packages/code-review's dedupe then re-posted findings it had already
made because it never saw the earlier comments.

Change

  • Added fetchAllPages, a paginating GET helper that follows the
    Link: rel="next" header when GitHub sends one, and falls back to
    incrementing page= when it doesn't (some servers/tests omit it).
    Capped at 30 pages; hitting the cap sets truncated: true.
  • PullRequestDiff gains a truncated: boolean field.
    fetchPullRequestReviewComments now returns
    { comments, truncated } instead of a bare array.
  • Response parsing still goes through the existing arktype schemas
    (PullRequestFilesResponse, ReviewCommentsResponse), now validated
    against the merged page rather than a single page.
  • packages/code-review's aggregateReview now posts one plain note
    in the review body ("This review may be incomplete: ...") whenever
    either the diff or the already-posted-comments page was truncated —
    a truncated fetch was previously reviewed silently as if it were
    complete, which risked re-flagging a finding whose earlier post fell
    outside the read page. Threaded through
    CodeReviewGitHub.listPostedComments and runPullRequestReview.
  • Renamed MAX_FILES_PER_PAGE to MAX_ITEMS_PER_PAGE (it bounds both
    the files and comments endpoints) and had fetchAllPages reuse
    requestJSON's error formatting via a shared fetchJSON helper
    instead of duplicating it.
  • Bumped @corbits/github-tools to 0.0.8 (two src changes) and
    updated the { name, version } pin literals that reference it
    (workflows/code-review, workflows/last-30-days-research, and
    that workflow's test fixture).

Tests

packages/github-tools/src/pull-requests.test.ts: a fake fetch serving
two pages (merged via Link: rel="next" and via a page= fallback), a
single short page (unchanged behavior), and a page-bound case that
reports truncated: true — for both fetchPullRequestDiff's file list
and fetchPullRequestReviewComments.

packages/code-review/src/aggregate.test.ts and review-run.test.ts:
red/green coverage that a truncated diff and a truncated comments page
each surface the incompleteness note (at the aggregation layer and
end-to-end through runPullRequestReview), and that an untruncated
run posts no note.

Local gate note: the machine was under heavy load, so the full
monorepo-wide bun run check could not be run reliably (it was
getting OOM-killed partway through, on packages this change never
touches). Ran targeted gates instead — tsc --noEmit and bun test
for packages/github-tools and packages/code-review,
check:tool-package-pins, check:tool-package-freshness — all green.
CI's bun run check is the authoritative gate for this PR, and it is
green on every job.

Cover the >100-item case github-tools currently drops: a fake fetch
serving two pages (merged via Link: rel="next" and via a page=
fallback), a single short page (unchanged), and a page-bound case that
must report truncated: true. code-review's fixtures gain the new
truncated/comments shape.

Fixes CL-7140.
fetchPullRequestDiff and fetchPullRequestReviewComments issued one
request at per_page=100 and stopped, silently dropping files or
comments past the first page — code-review's dedupe then re-posted
findings it had already made. Both now follow the Link: rel="next"
header (falling back to page= when GitHub omits it), capped at 30
pages, and report truncated: true if the cap is hit.

Fixes CL-7140.
The 0.0.7 pin bump missed this test's hardcoded expectation.
Cover both truncation sources (diff.truncated and the posted-comments
page) at the aggregation layer and end to end through
runPullRequestReview, plus the untruncated case posting no note.
A truncated diff or already-posted-comments page was silently reviewed
as if it were complete, which could re-flag a finding whose earlier
post fell outside the read page. aggregateReview now posts one plain
note when either source is truncated.

Also: renamed MAX_FILES_PER_PAGE to MAX_ITEMS_PER_PAGE (it bounds both
the files and comments endpoints), and fetchAllPages now shares
requestJSON's error formatting via a common fetchJSON helper instead
of duplicating it. Bumped @corbits/github-tools to 0.0.8 for the src
change and updated its pin literals.
@TheGreatAxios
TheGreatAxios force-pushed the cl-7140-github-tools-pr-diff-and-review-comment-fetches-stop-at-page branch from 4a46ad1 to 0f9b41e Compare August 29, 2026 04:46
@TheGreatAxios
TheGreatAxios merged commit 5cf1acc into main Aug 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant