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

packages/code-review/src/aggregate.test.ts

@@ -23,6 +23,7 @@ const DIFF: PullRequestDiff = {
       changedLines: [1, 2, 3],
     },
   ],
+  truncated: false,
 };
 
 function pass(id: string, report: unknown): ReviewerPass {
@@ -279,3 +280,32 @@ test("a fingerprint already posted is skipped on a re-run", () => {
   expect(review.body).not.toContain("already flagged");
   expect(review.comments).toEqual([]);
 });
+
+test("an untruncated diff and comment page post no incompleteness note", () => {
+  const review = aggregateReview(
+    [pass("correctness", { summary: "read it", findings: [] })],
+    DIFF,
+  );
+  expect(review.body).not.toContain("may be incomplete");
+});
+
+test("a truncated diff surfaces an incompleteness note in the review body", () => {
+  const review = aggregateReview(
+    [pass("correctness", { summary: "read it", findings: [] })],
+    { ...DIFF, truncated: true },
+  );
+  expect(review.body).toContain(
+    "This review may be incomplete: the pull request has more changed " +
+      "files or already-posted comments than one review pass reads",
+  );
+});
+
+test("a truncated already-posted-comments page surfaces the same note", () => {
+  const review = aggregateReview(
+    [pass("correctness", { summary: "read it", findings: [] })],
+    DIFF,
+    new Set(),
+    true,
+  );
+  expect(review.body).toContain("This review may be incomplete");
+});

packages/code-review/src/aggregate.ts

@@ -195,6 +195,16 @@ function commentBody(entry: AggregatedFinding, diff: PullRequestDiff): string {
   );
 }
 
+// GitHub caps a paginated fetch at `fetchAllPages`' page bound; a pull
+// request past it reads back partial. Silence would let this review
+// read as complete when it saw only part of the change, or re-flag a
+// finding whose earlier post fell outside the read page — both worse
+// than saying so plainly.
+const TRUNCATED_NOTE =
+  "_This review may be incomplete: the pull request has more changed " +
+  "files or already-posted comments than one review pass reads, so " +
+  "some results may be partial or repeated._";
+
 function countLine(findings: readonly AggregatedFinding[]): string {
   if (findings.length === 0) {
     return "No findings — the reviewers read the change and had nothing to raise.";
@@ -219,6 +229,7 @@ export function aggregateReview(
   passes: readonly ReviewerPass[],
   diff: PullRequestDiff,
   alreadyPosted: ReadonlySet<string> = new Set(),
+  commentsTruncated = false,
 ): PullRequestReviewDraft {
   const collected = collect(passes, alreadyPosted);
   const sections: string[] = [
@@ -227,6 +238,10 @@ export function aggregateReview(
     countLine(collected.findings),
   ];
 
+  if (diff.truncated || commentsTruncated) {
+    sections.push("", TRUNCATED_NOTE);
+  }
+
   for (const severity of SEVERITY_ORDER) {
     const forSeverity = collected.findings.filter(
       (entry) => entry.finding.severity === severity,

packages/code-review/src/prompt.test.ts

@@ -27,6 +27,7 @@ function diffOf(files: readonly PullRequestFileDiff[]): PullRequestDiff {
     headSha: "headsha",
     baseSha: "basesha",
     files,
+    truncated: false,
   };
 }
 

packages/code-review/src/review-run.test.ts

@@ -31,6 +31,7 @@ const DIFF: PullRequestDiff = {
       changedLines: [1, 2],
     },
   ],
+  truncated: false,
 };
 
 interface FakeGitHub {
@@ -47,6 +48,7 @@ interface FakeGitHub {
 function fakeGitHub(
   diff: PullRequestDiff = DIFF,
   postedComments: readonly string[] = [],
+  commentsTruncated = false,
 ): FakeGitHub {
   const posted: FakeGitHub["posted"][number][] = [];
   const diffReads: PullRequestRef[] = [];
@@ -69,7 +71,10 @@ function fakeGitHub(
       },
       listPostedComments: (ref) => {
         listedComments.push(ref);
-        return Promise.resolve(postedComments);
+        return Promise.resolve({
+          comments: postedComments,
+          truncated: commentsTruncated,
+        });
       },
     },
   };
@@ -221,3 +226,35 @@ test("a finding whose fingerprint was already posted is not raised again", async
   expect(result.review.body).toContain("release-risk finding");
   expect(result.review.body).not.toContain("architecture finding");
 });
+
+test("a truncated diff surfaces an incompleteness note in the posted review", async () => {
+  const github = fakeGitHub({ ...DIFF, truncated: true });
+
+  const result = await runPullRequestReview(
+    {
+      github: github.client,
+      runReviewerTurn: ({ reviewer }) =>
+        Promise.resolve(reportFor(reviewer.id)),
+    },
+    REF,
+  );
+
+  if (result.skipped) throw new Error("expected the review to run");
+  expect(result.review.body).toContain("This review may be incomplete");
+});
+
+test("a truncated already-posted-comments page surfaces the same note", async () => {
+  const github = fakeGitHub(DIFF, [], true);
+
+  const result = await runPullRequestReview(
+    {
+      github: github.client,
+      runReviewerTurn: ({ reviewer }) =>
+        Promise.resolve(reportFor(reviewer.id)),
+    },
+    REF,
+  );
+
+  if (result.skipped) throw new Error("expected the review to run");
+  expect(result.review.body).toContain("This review may be incomplete");
+});

packages/code-review/src/review-run.ts

@@ -21,6 +21,7 @@ import type {
   PostedPullRequestReview,
   PullRequestDiff,
   PullRequestRef,
+  PullRequestReviewCommentsPage,
   PullRequestReviewDraft,
 } from "@corbits/github-tools";
 
@@ -39,7 +40,9 @@ export interface CodeReviewGitHub {
     review: PullRequestReviewDraft,
   ): Promise<PostedPullRequestReview>;
   /** Bodies of every review comment already posted, for the fingerprint scan. */
-  listPostedComments(ref: PullRequestRef): Promise<readonly string[]>;
+  listPostedComments(
+    ref: PullRequestRef,
+  ): Promise<PullRequestReviewCommentsPage>;
 }
 
 /** Runs one reviewer's turn and returns its raw reply. */
@@ -111,13 +114,17 @@ export async function runPullRequestReview(
     };
   }
   const prompt = renderReviewPrompt(diff);
-  const alreadyPosted = fingerprintsIn(
-    await deps.github.listPostedComments(ref),
-  );
+  const postedComments = await deps.github.listPostedComments(ref);
+  const alreadyPosted = fingerprintsIn(postedComments.comments);
   const passes = await Promise.all(
     reviewers.map((reviewer) => runOne(deps, reviewer, prompt)),
   );
-  const review = aggregateReview(passes, diff, alreadyPosted);
+  const review = aggregateReview(
+    passes,
+    diff,
+    alreadyPosted,
+    postedComments.truncated,
+  );
   const posted = await deps.github.postReview(ref, diff.headSha, review);
   return { skipped: false, diff, passes, review, posted };
 }

packages/github-tools/package.json

@@ -2,7 +2,7 @@
   "name": "@corbits/github-tools",
   "private": true,
   "description": "GitHub search integration: a minimal REST client and an @intx/agent tool bundle exposing github_activity",
-  "version": "0.0.6",
+  "version": "0.0.8",
   "license": "LGPL-2.1-or-later",
   "type": "module",
   "exports": {

packages/github-tools/src/index.ts

@@ -15,6 +15,7 @@ export type {
   PullRequestFileDiff,
   PullRequestRef,
   PullRequestReviewComment,
+  PullRequestReviewCommentsPage,
   PullRequestReviewDraft,
 } from "./pull-requests";
 export {

packages/github-tools/src/pull-requests.test.ts

@@ -176,7 +176,7 @@ test("postPullRequestReview posts one comment-only review at the head sha", asyn
 });
 
 test("fetchPullRequestReviewComments returns each comment's body", async () => {
-  const bodies = await fetchPullRequestReviewComments(
+  const page = await fetchPullRequestReviewComments(
     {
       apiKey: "token",
       baseUrl: BASE,
@@ -186,7 +186,173 @@ test("fetchPullRequestReviewComments returns each comment's body", async () => {
     },
     { owner: "acme", repo: "widgets", number: 7 },
   );
-  expect(bodies).toEqual(["first", "second"]);
+  expect(page).toEqual({ comments: ["first", "second"], truncated: false });
+});
+
+/** A page short enough to end pagination whichever way the caller detects it. */
+function shortPage(bodies: string[], link?: string): Response {
+  return new Response(JSON.stringify(bodies.map((body) => ({ body }))), {
+    status: 200,
+    headers: link === undefined ? {} : { link },
+  });
+}
+
+/** A full (100-item) page, to drive the page-number fallback when no `Link` header is sent. */
+function fullPage(prefix: string): Response {
+  const bodies = Array.from({ length: 100 }, (_, i) => ({
+    body: `${prefix}-${String(i)}`,
+  }));
+  return new Response(JSON.stringify(bodies), { status: 200 });
+}
+
+test('fetchPullRequestReviewComments follows a Link: rel="next" header across pages', async () => {
+  const requestedUrls: string[] = [];
+  const page = await fetchPullRequestReviewComments(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch((input) => {
+        const url = String(input);
+        requestedUrls.push(url);
+        if (url.includes("page=2")) {
+          return Promise.resolve(shortPage(["third", "fourth"]));
+        }
+        return Promise.resolve(
+          shortPage(["first", "second"], `<${BASE}/next?page=2>; rel="next"`),
+        );
+      }),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(page).toEqual({
+    comments: ["first", "second", "third", "fourth"],
+    truncated: false,
+  });
+  expect(requestedUrls).toHaveLength(2);
+});
+
+test("fetchPullRequestReviewComments falls back to page= when no Link header is sent", async () => {
+  let calls = 0;
+  const page = await fetchPullRequestReviewComments(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch(() => {
+        calls += 1;
+        return Promise.resolve(
+          calls === 1 ? fullPage("p1") : shortPage(["last"]),
+        );
+      }),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(page.comments).toHaveLength(101);
+  expect(page.comments[100]).toBe("last");
+  expect(page.truncated).toBe(false);
+  expect(calls).toBe(2);
+});
+
+test("fetchPullRequestReviewComments reports truncated when the page bound is hit", async () => {
+  const page = await fetchPullRequestReviewComments(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch(() => Promise.resolve(fullPage("p"))),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(page.truncated).toBe(true);
+  expect(page.comments).toHaveLength(3000);
+});
+
+test("fetchPullRequestDiff follows pagination for files and reports it unset for one page", async () => {
+  const diff = await fetchPullRequestDiff(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch((input) => {
+        const url = String(input);
+        if (url.includes("/files")) {
+          return Promise.resolve(
+            jsonResponse([
+              {
+                filename: "src/loop.ts",
+                status: "modified",
+                additions: 2,
+                deletions: 1,
+                patch: PATCH,
+              },
+            ]),
+          );
+        }
+        return Promise.resolve(jsonResponse(PULL_BODY));
+      }),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(diff.truncated).toBe(false);
+  expect(diff.files).toHaveLength(1);
+});
+
+function fileAt(index: number): Record<string, unknown> {
+  return {
+    filename: `src/file-${String(index)}.ts`,
+    status: "modified",
+    additions: 1,
+    deletions: 0,
+  };
+}
+
+test("fetchPullRequestDiff merges every page of changed files", async () => {
+  const diff = await fetchPullRequestDiff(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch((input) => {
+        const url = String(input);
+        if (url.includes("page=2")) {
+          return Promise.resolve(
+            new Response(JSON.stringify([fileAt(100)]), { status: 200 }),
+          );
+        }
+        if (url.includes("/files")) {
+          const files = Array.from({ length: 100 }, (_, i) => fileAt(i));
+          return Promise.resolve(
+            new Response(JSON.stringify(files), {
+              status: 200,
+              headers: {
+                link: `<${BASE}/repos/acme/widgets/pulls/7/files?page=2>; rel="next"`,
+              },
+            }),
+          );
+        }
+        return Promise.resolve(jsonResponse(PULL_BODY));
+      }),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(diff.files).toHaveLength(101);
+  expect(diff.truncated).toBe(false);
+});
+
+test("fetchPullRequestDiff reports truncated when the file-page bound is hit", async () => {
+  const diff = await fetchPullRequestDiff(
+    {
+      apiKey: "token",
+      baseUrl: BASE,
+      fetchImpl: fakeFetch((input) => {
+        const url = String(input);
+        if (url.includes("/files")) {
+          const files = Array.from({ length: 100 }, (_, i) => fileAt(i));
+          return Promise.resolve(jsonResponse(files));
+        }
+        return Promise.resolve(jsonResponse(PULL_BODY));
+      }),
+    },
+    { owner: "acme", repo: "widgets", number: 7 },
+  );
+  expect(diff.truncated).toBe(true);
+  expect(diff.files).toHaveLength(3000);
 });
 
 test("postPullRequestReview refuses an empty body", async () => {