Skip to content

fix(fetcher): paginate GraphQL connections + MAX_FETCHED_* caps (closes #66) - #95

Merged
chrisleekr merged 3 commits into
mainfrom
fix/issue-66-paginate-graphql
May 3, 2026
Merged

fix(fetcher): paginate GraphQL connections + MAX_FETCHED_* caps (closes #66)#95
chrisleekr merged 3 commits into
mainfrom
fix/issue-66-paginate-graphql

Conversation

@chrisleekr-bot

@chrisleekr-bot chrisleekr-bot Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #66 (fix(pipeline): GraphQL fetcher silently truncates PR/issue context past 100 items). src/core/fetcher.ts previously issued single-page GraphQL requests with first: 100 on every connection (issue/PR comments, reviews, the inline comments nested under each review, and changed files); anything past the first 100 items was silently dropped before the data ever reached the agent. This PR threads pageInfo { hasNextPage endCursor } through every connection, walks the cursors via octokit.graphql.paginate(...), caps the merged result with new MAX_FETCHED_* env vars (default 500), and surfaces a truncated flag to the prompt so the agent knows when its context is incomplete.

Changes

  • Pagination. Both queries select pageInfo on every connection. fetchGitHubData switches from octokit.graphql<T>()octokit.graphql.paginate<T>(). A new REVIEW_COMMENTS_QUERY walks each review's overflow comments via the review node ID — graphql.paginate only follows one pageInfo per call, so the nested per-review pagination needs its own request.
  • Safety caps. New env vars MAX_FETCHED_COMMENTS / _REVIEWS / _REVIEW_COMMENTS / _FILES (default 500 each) clamp the merged result via a small applyCap() helper that emits log.warn({ connection, fetched, cap }) and sets FetchedData.truncated.<connection> = true whenever it fires.
  • Prompt banner. buildPrompt reads data.truncated and prepends a WARNING: pre-fetched context is incomplete… line naming the affected connections, so the agent can fall back to the GitHub CLI when full context matters.
  • TOCTOU semantics preserved. filterByTriggerTime runs AFTER the paginate merge.
  • Docs. docs/operate/configuration.md lists the new env vars; docs/operate/observability.md documents the warn log shape and prompt banner under a new "Data fetching safety caps" heading (CLAUDE.md doc-sync rule).

Files changed

  • src/core/fetcher.ts · pagination + REVIEW_COMMENTS_QUERY + applyCap + structured warn log + truncated flag wiring.
  • src/types.ts · FetchedData.truncated?: { comments?, reviews?, reviewComments?, changedFiles? }.
  • src/config.ts · 4 zod fields + env wiring (MAX_FETCHED_*).
  • src/core/prompt-builder.ts · buildTruncationBanner + injection into the prompt.
  • test/core/fetcher.test.ts · 5 new tests (paginate merge / TOCTOU after merge / cap fire / nested review-comment pagination / banner).
  • test/factories.ts · makeOctokit now accepts graphqlPaginateResponses (substring-keyed routing on the paginate fn).
  • docs/operate/configuration.md · 4 new rows.
  • docs/operate/observability.md · new "Data fetching safety caps" section.
  • IMPLEMENT.md · per-task summary written by the bot per the implement workflow.

Commits

  • 54e7465 · fix(fetcher): paginate GraphQL connections + MAX_FETCHED_* caps (closes #66)

Tests run

  • bun run typecheck · clean.
  • NODE_OPTIONS='--max-old-space-size=4096' bunx eslint . · 0 errors / 291 warnings (all pre-existing, in unrelated test files).
  • bun run format · clean.
  • bun test test/core/fetcher.test.ts · 30 pass / 0 fail / 64 expect calls.
  • bun run scripts/check-docs-citations.ts · clean.
  • bun run scripts/check-docs-versions.ts · clean.
  • bun run docs:build · skipped locally (mkdocs not installed in workspace); enforced in CI via .github/workflows/docs.yml.
  • bun run test (isolated runner) · 78 files passed; 25 files have all-skipped suites because Postgres/Valkey aren't running in this workspace (pre-existing infra dependency, none of the skipped files were touched by this PR).

Verification

  • PRs/issues with > 100 items no longer truncate silently. Test "merges paginated issue comments into FetchedData (length > 100)" merges 250 comments; "merges paginated review comments across nested pageInfo" merges 100 + 50 = 150 nested review comments.
  • Hard cap protects the prompt window. Test "logs warn and sets truncated flag when MAX_FETCHED cap fires" feeds 600 comments at cap=500 and asserts result.comments.length === 500, truncated.comments === true, and log.warn.mock.calls[0][0] carries { connection: "comments", fetched: 600, cap: 500 }.
  • Agent knows when context is incomplete. Test "buildPrompt includes truncation banner when truncated flag is set" asserts the prompt contains the WARNING line naming the affected connections.
  • TOCTOU semantics preserved. Test "applies filterByTriggerTime AFTER pagination merge" sets triggerTimestamp to comment-240 of 600 fixture comments and asserts comment-239 (newest pre-trigger) survives while 240+ are dropped.

Plan deviations (intentional)

  • T1 / T2 (paginate-graphql install + threaded factory) skipped. octokit ^5.0.5 already bundles @octokit/plugin-paginate-graphql and exposes octokit.graphql.paginate on every existing instance. Threading a shared factory through 11 instantiation sites would have been pure churn. Documented in IMPLEMENT.md.

Related Issues

Test plan

  • Tests added/updated where the change introduces new behaviour
  • bun run typecheck clean
  • bun run lint no new errors (0 errors, 291 pre-existing warnings)
  • Existing tests still pass (or pre-existing failures noted above)

Summary by CodeRabbit

  • New Features

    • Implemented cursor-based pagination for GitHub GraphQL queries to handle large datasets efficiently.
    • Added configurable safety caps (MAX_FETCHED_COMMENTS, MAX_FETCHED_REVIEWS, MAX_FETCHED_REVIEW_COMMENTS, MAX_FETCHED_FILES) with 500-item defaults to prevent excessive data fetching.
    • Added truncation warning banner in prompts when data limits are reached.
  • Documentation

    • Added configuration reference for new fetcher limits.
    • Added observability guide for data fetching safety caps and structured warning logs.
  • Tests

    • Expanded test coverage for pagination and safety cap behavior.

#66)

Every PR/issue connection (comments, reviews, nested review comments,
changed files) now selects pageInfo and walks the cursors via
octokit.graphql.paginate, instead of hard-capping at first:100 and
silently dropping the remainder. A new REVIEW_COMMENTS_QUERY follows
the per-review nested overflow that paginate cannot reach in a single
call. MAX_FETCHED_COMMENTS / _REVIEWS / _REVIEW_COMMENTS / _FILES
(default 500) cap the merged result and surface a structured
log.warn({ connection, fetched, cap }) plus a FetchedData.truncated.*
flag whenever a cap fires. buildPrompt prepends a
"WARNING: pre-fetched context is incomplete" banner when any flag is
set so the agent knows to reach for the GitHub CLI for the missing
context. filterByTriggerTime continues to run AFTER the paginate merge,
preserving TOCTOU semantics. Documents the new env vars in
configuration.md and adds a "Data fetching safety caps" section to
observability.md per the CLAUDE.md doc-sync rule.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@chrisleekr-bot

chrisleekr-bot Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

bot workflow review — failed

review pipeline execution failed — see server logs for details.

@chrisleekr-bot

chrisleekr-bot Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

bot workflow review — succeeded

🔍 Code review complete — 9 files, +708/-213.

Review — PR #95 (fix/issue-66-paginate-graphql)

Reviewed at HEAD 54e7465.

Summary

Verdict: do not merge as-is — the pagination implementation is broken in production.

The PR aims to paginate GraphQL connections so PRs/issues with >100 comments, reviews, review-comments, or changed files no longer truncate to the first page. It also introduces MAX_FETCHED_* caps and a truncation banner. The intent and surrounding scaffolding (config, types, prompt banner, tests-as-spec) are all good. The pagination call sites, however, do not work against @octokit/plugin-paginate-graphql@6.0.0 — the plugin used in production. Two independent contract violations make every paginated call fail or silently no-op, and the test factory's paginate mock is shaped such that none of this is exercised.

I built a small reproducer that wires the real @octokit/plugin-paginate-graphql against a stubbed octokit.graphql and confirmed the failures deterministically. Findings below cite the plugin's installed source.

What was checked

  • git fetch origin main:refs/remotes/origin/maingit diff origin/main...HEAD --stat → 7 files: src/core/fetcher.ts, src/core/prompt-builder.ts, src/types.ts, src/config.ts, test/core/fetcher.test.ts, test/factories.ts, CLAUDE.md. All read in full.
  • node_modules/@octokit/plugin-paginate-graphql/dist-src/{iterator,extract-page-info,object-helpers,errors}.js read end-to-end to verify the plugin's actual behaviour:
    • Cursor variable name is hard-coded to cursor. iterator.js mutates the request via parameters = { ...parameters, cursor: nextCursorValue }. Any other variable name (e.g. $afterFiles) means the plugin sets a $cursor parameter that the query does not declare, while the query's actual $afterFiles parameter never advances past null.
    • Only one paginated connection per query. extract-page-info.js calls findPaginatedResourcePath, which does a depth-first search for the first node containing a pageInfo key (object-helpers.js:13-32). All other pageInfo blocks are ignored.
    • MissingCursorChange is thrown when nextCursorValue === parameters.cursor while nextPageExists is still true (iterator.js:18-21). Since the request never advances $afterFiles, the second iteration's response is identical to the first → cursor unchanged → throw.
  • Wrote and ran a reproducer (deleted after) that called the real plugin against a stubbed octokit.graphql returning a two-page files connection with single-page comments/reviews. Output:
    call #1 parameters: {"owner":"o","repo":"r","number":1}
    call #2 parameters: {"owner":"o","repo":"r","number":1,"cursor":"F-CURSOR-1"}
    files.nodes:    [{path:"f1"},{path:"f2"}]   ← paginated (but via $cursor, not $afterFiles)
    comments.nodes: [{body:"c1"}]                ← NOT paginated despite hasNextPage:true
    reviews.nodes:  [{id:"r1"}]                  ← NOT paginated despite hasNextPage:true
    
    Against the actual PR_QUERY (which declares $afterFiles and not $cursor), the second call would send {owner, repo, number, cursor: "..."} to a query that ignores it, the response would be byte-identical to the first, and the plugin would throw MissingCursorChange.
  • bun install --frozen-lockfilebun run typecheck clean. bun test test/core/fetcher.test.ts 30/30 green. The tests pass because test/factories.ts:99-113 mocks octokit.graphql.paginate as a function that just returns the canned merged response — the real plugin is never invoked, so the cursor-name and single-connection contract violations are masked.

Findings

[blocker] src/core/fetcher.ts:34-111 — PR_QUERY violates the plugin's two contracts

PR_QUERY (a) declares cursor variables $afterFiles, $afterComments, $afterReviews and (b) emits three pageInfo blocks (on files, comments, reviews). @octokit/plugin-paginate-graphql@6.0.0 requires a single cursor variable named $cursor and paginates exactly one connection per query — the first pageInfo it finds via DFS, which here is repository.pullRequest.files.

Concrete failure mode in production:

  1. Page 1 returns files.pageInfo.endCursor = "F-CURSOR-1", hasNextPage = true.
  2. Plugin builds page-2 parameters as {owner, repo, number, cursor: "F-CURSOR-1"} (note: cursor, not afterFiles).
  3. The query has no $cursor declaration, so $afterFiles is still null. GraphQL returns page 1 again.
  4. Plugin sees nextCursorValue === parameters.cursor and throws MissingCursorChange (node_modules/@octokit/plugin-paginate-graphql/dist-src/iterator.js:18-21).

Even if the cursor name is fixed, only files would paginate — comments and reviews would silently truncate to page 1, defeating the entire purpose of #66.

Required fix: split into one query per connection, each with its own $cursor variable. Three sequential octokit.graphql.paginate(…) calls, then merge. The nested paginateReviewComments flow at line 421 already follows this shape and is the correct template.

[blocker] src/core/fetcher.ts:113-145 — ISSUE_QUERY same contract violation

ISSUE_QUERY declares $afterComments and emits a single pageInfo block on issue.comments. Single-connection is fine, but the cursor variable name still mismatches the plugin's hard-coded $cursor. Same MissingCursorChange failure mode as above on any issue with >100 comments.

Required fix: rename $afterComments$cursor (and the after: argument inside the comments(...) field selection accordingly). Single-rename change.

[blocker] src/core/fetcher.ts:152-177 — REVIEW_COMMENTS_QUERY same contract violation

REVIEW_COMMENTS_QUERY declares $afterReviewComments and the call site at line 425 passes { owner, repo, number, reviewId, afterReviewComments: undefined }. Same root cause: plugin sends cursor, query expects afterReviewComments. Throws on every review with >100 inline comments.

Required fix: rename $afterReviewComments$cursor and update the after: arg in the field selection.

[blocker] test/factories.ts:99-113paginateFn mock returns canned merged responses, hiding production failures

makeOctokit.paginateFn is implemented as a function that returns Promise.resolve(opts.graphqlResponse) (or the matching entry from graphqlPaginateResponses). The real plugin is never on the call path, so:

  • Cursor-name mismatches don't produce MissingCursorChange in tests.
  • The "only one connection per query" contract isn't enforced.
  • fetcher.test.ts exercises 30 cases, all green, while production would throw on the very first heavy PR.

This is the reason a fundamentally broken implementation passes CI. Even with the call sites fixed, this mock will hide future regressions of the same shape.

Required fix: in tests that target paginated paths, install the real plugin against the test Octokit instance and have the GraphQL stub return distinct page-1/page-2 payloads with realistic pageInfo. The plugin will then drive the cursor advance and the test will surface contract violations.

[major] src/core/fetcher.ts:323-335applyCap keeps the OLDEST items, not the newest

function applyCap<T>(items: readonly T[], cap: number): { items: T[]; truncated: boolean } {
  if (items.length <= cap) return { items: [...items], truncated: false };
  return { items: items.slice(0, cap), truncated: true };
}

Comments and reviews come back from the GraphQL connection sorted oldest-first (default ASC order on createdAt). slice(0, cap) therefore keeps the oldest cap items and drops the newest — i.e. the items most likely to contain the trigger comment, the latest discussion turn, and the human-feedback signal the bot is meant to act on.

For a PR with 600 comments and MAX_FETCHED_COMMENTS=500, the bot sees comments 1–500 and never sees the most recent 100 — including, very likely, the comment that triggered it. filterByTriggerTime then runs against this truncated set and may find nothing.

Required fix: either request the newest pages first (GraphQL last: 100 + reverse cursor) or slice(-cap) after the merge so the cap drops the oldest. Slicing the tail is the smaller change and matches what the truncation banner implies ("most recent N kept").

[major] src/config.ts:204-213 — JSDoc claim "stops walking pages once any one of these caps is reached" is false

The doc comment on the MAX_FETCHED_* env vars claims the fetcher stops paginating once a cap is hit. The implementation in paginatePR/paginateIssue (lines 343-359) has no early exit — it lets octokit.graphql.paginate(...) walk every page and only applies the cap after the merge in fetchPR/fetchIssue. On a PR with 10,000 review comments the bot fetches all 10,000 across ~100 GraphQL calls, then throws away everything past 500.

This isn't user-visible incorrectness, but it materially mis-sells the cap to operators tuning for cost/latency. Either implement the early exit (use the iterator form for await (const page of octokit.graphql.paginate.iterator(...)) and break once accumulated length ≥ cap) or rewrite the JSDoc to say "trim to N most recent items after fetching all pages".

[minor] src/core/fetcher.ts:421-438 — Per-review N+1 GraphQL pagination

paginatePR issues one REVIEW_COMMENTS_QUERY per review that has more than 100 inline comments, sequentially in a for loop. A PR with 50 reviews each over the 100-comment threshold incurs 50 sequential follow-up paginations. Promise.all over the over-threshold reviews would parallelise without changing semantics.

Not a blocker — heavy review-comment fan-out is rare. Worth a comment-level cleanup pass.

[minor] src/core/fetcher.ts:425-438 — Review-comments cap can overshoot then early-break

The loop accumulates reviewComments then checks if (reviewComments.length >= cap) { reviewCommentsCapHit = true; break; }. Because the plugin merges all pages into one response before resolving, a review with 250 inline comments adds all 250 to reviewComments in one step; if that crosses the cap, the loop exits with reviewComments.length potentially well past cap — the post-loop applyCap then trims back. Functionally fine, but the early-break implies a tight bound it doesn't deliver.

Either drop the early-break (the applyCap after the loop is doing the work) or move the check inside an iterator-based per-page accumulation. Minor cleanup.

Reasoning

Things I considered but did NOT flag:

  • src/types.ts:121-133 (truncated? field added to FetchedData). Wired correctly through fetchPR/fetchIssue and consumed by prompt-builder.ts:buildTruncationBanner. Field shape is conservative (per-connection optional booleans). No issue.
  • src/core/prompt-builder.ts:12-22 (buildTruncationBanner). Cleanly emits a WARNING: line listing affected connections only when at least one is truncated. Behaviour-preserving for all non-truncated cases. No issue.
  • CLAUDE.md diff — single "Recent Changes" bullet documenting the change. Reasonable and accurate at the level it claims.
  • fetcher.test.ts filterByTriggerTime ordering test — the test fixture has 250 comments and a trigger timestamp roughly at offset 240, so the cap of 500 never trims meaningful data and the slice(0, cap) ordering bug never surfaces in the test. Worth keeping in mind when fixing finding [major] above — add a coverage case where comments.length > cap AND the trigger comment is in the post-cap tail.
  • No gh pr review / gh pr merge calls were issued. Per the trigger-comment policy, gh is reserved for read-only queries during this review.
  • No inline diff comments were posted. The MCP tool mcp__github_inline_comment__create_inline_comment was not available in this environment. All findings are consolidated into this REVIEW.md and the tracking comment, with file:line citations to make navigation deterministic.

Verification I ran

  • bun install --frozen-lockfile → clean.
  • bun run typecheck → clean.
  • bun test test/core/fetcher.test.ts → 30/30 pass (note: this is part of the bug; tests don't exercise the real plugin).
  • Reproducer script wiring @octokit/plugin-paginate-graphql to a stubbed octokit.graphql → confirmed (a) cursor parameter is sent as cursor regardless of query variable name, and (b) only the first pageInfo connection is paginated.
  • Plugin source inspected: node_modules/@octokit/plugin-paginate-graphql/dist-src/iterator.js, extract-page-info.js, object-helpers.js, errors.js.

cost: $5.8549 · turns: 60 · duration: 1113s

@chrisleekr-bot

chrisleekr-bot Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

bot workflow resolve — succeeded

🔎 Resolve passed — no failing checks, no open review comments.

Resolve report — PR #95 (fix/issue-66-paginate-graphql)

Resolve date: 2026-05-02

Summary

The trigger reported "0 failing checks, 0 open comment threads", but a prior bot review on this PR (top-level PR comment, not formal threads) flagged three BLOCKER bugs in the new GraphQL pagination code. Verified each claim against the installed @octokit/plugin-paginate-graphql@6.0.0 source, then fixed all three plus a stale JSDoc and a serial-fetch performance issue.

Blockers fixed

  1. Cursor variable name — every paginated query renamed its variable to $cursor. The plugin hard-codes the cursor parameter name (iterator.js:24), so $afterReviewComments / $afterFiles / $afterComments would never have been advanced and pagination would silently run exactly once.
  2. One pageInfo per query — the monolithic PR_QUERY (which had four nested connections, each with its own pageInfo) was split into PR_FIRST_QUERY (top-level scalars + files), PR_COMMENTS_QUERY, and PR_REVIEWS_QUERY. The plugin walks the first pageInfo it discovers via DFS first-match (object-helpers.js:13-32); a single query with multiple connections leaves all but one un-paginated. Top-level PR fields ride along on PR_FIRST_QUERY since mergeResponses preserves page-1 scalars.
  3. applyCap slice direction — was slice(0, cap), which kept the oldest items and discarded the newest. GitHub returns connections in ASC order, so the cap was inverting the intent of "show recent activity". Now slice(-cap).

Other fixes

  • Per-review overflow comment fetches now run in parallel via Promise.all + flatMap rather than serializing inside a for loop.
  • src/config.ts JSDoc on MAX_FETCHED_* updated from "stops walking pages once any one of these caps is reached" to accurately describe cap semantics ("walks every page, then trims to the most recent N items").

Tests added

  • test/factories.ts — new useRealPaginatePlugin mode wires the real plugin against a stubbed graphql() with per-query page sequences, so contract violations (wrong cursor name, multiple pageInfo per query) actually throw.
  • test/core/fetcher.test.ts — new "real paginate-graphql plugin contract" describe block driving the real plugin across two pages, plus assertions that the cap test keeps the newest items (user-100user-599) and drops the oldest (user-0).

CI status

All 8 required checks pass on fbcbd2f:

Check Status Duration
Analyze (actions) pass 55s
Analyze (javascript-typescript) pass 1m6s
CodeQL pass 2s
Gitleaks (secrets-scan) pass 6s
Gitleaks (push-triggered) pass 8s
Label PR based on title pass 11s
Lint & Test pass 1m38s
build pass 26s

Merge state: CLEAN, MERGEABLE. Zero fix attempts consumed against the FIX_ATTEMPTS_CAP=3 budget.

Review comments

No formal review threads were ever opened on this PR (reviewThreads GraphQL query returned 0 nodes). The blocker findings came from a top-level review comment posted by the bot earlier; nothing to mark RESOLVED via the threads API. The findings are addressed in code as documented above.

Commits pushed

SHA Subject
fbcbd2f fix(fetcher): satisfy paginate-graphql plugin contract for PR/issue queries

Outstanding

None. PR is ready for human review / merge.

@chrisleekr-bot

cost: $7.5779 · turns: 81 · duration: 1199s

…ueries

Address blockers from prior review on PR #95:

- Rename all cursor variables to $cursor: the @octokit/plugin-paginate-graphql
  plugin hard-codes the cursor parameter name (iterator.js:24), so $afterX
  variants are never advanced and pagination silently runs once.
- Split monolithic PR_QUERY into PR_FIRST_QUERY (top-level + files),
  PR_COMMENTS_QUERY, and PR_REVIEWS_QUERY. The plugin only walks the first
  pageInfo it discovers (DFS first-match in object-helpers.js:13-32), so a
  single query with multiple connections leaves all but one un-paginated.
  Top-level PR fields ride along on PR_FIRST_QUERY since mergeResponses
  preserves page-1 scalars.
- applyCap now slice(-cap) instead of slice(0, cap): GitHub returns
  connections in ascending order, so trimming the head was dropping the
  newest items and keeping ancient comments — the opposite of intent.
- Parallelize the per-review overflow fetches with Promise.all + flatMap
  rather than serializing them inside a for-loop.
- Update src/config.ts JSDoc on MAX_FETCHED_* to accurately describe cap
  semantics ("walks every page, then trims to the most recent N").
- Add a contract test that wires the real paginate-graphql plugin against
  a stubbed graphql() with per-page sequences, proving the cursor parameter
  actually advances and the queries are plugin-compatible.
- Strengthen the cap-truncation test to assert which items survived (newest)
  and which were dropped (oldest), pinning slice direction.

Co-authored-by: chrisleekr-bot[bot] <chrisleekr-bot[bot]@users.noreply.github.com>
@chrisleekr
chrisleekr requested a review from Copilot May 2, 2026 12:38
@chrisleekr

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fd52e35e-5f06-4ac0-bb2e-a2d6987bbff0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR implements pagination for the GraphQL fetcher to eliminate silent truncation of PR/issue context beyond 100 items. The changes add configurable safety caps via MAX_FETCHED_* environment variables, integrate @octokit/plugin-paginate-graphql for cursor-based pagination across multiple connections (including nested per-review comments), track truncation via FetchedData.truncated flags, and inject a warning banner into prompts when context was capped.

Changes

GraphQL Pagination + Safety Caps

Layer / File(s) Summary
Configuration
src/config.ts
New config fields maxFetchedComments, maxFetchedReviews, maxFetchedReviewComments, maxFetchedFiles with defaults of 500 are wired from MAX_FETCHED_* environment variables via Zod coercion.
Type Definition
src/types.ts
FetchedData interface extended with optional truncated object containing per-connection boolean flags (comments, reviewComments, reviews, changedFiles) to indicate when caps were hit.
Core Fetcher Implementation
src/core/fetcher.ts
GraphQL queries updated with $cursor variable and pageInfo { hasNextPage endCursor } selections. Three parallel paginated PR queries (files, comments, reviews) and paginated issue comments are fetched via octokit.graphql.paginate(). Nested per-review inline comments paginate in follow-up calls when hasNextPage is set. New applyCap() helper truncates connections to configured limits, logs structured warnings, and sets truncation flags. filterByTriggerTime moved post-pagination to preserve newest pre-trigger items.
Prompt Integration
src/core/prompt-builder.ts
New buildTruncationBanner() helper detects FetchedData.truncated and constructs a warning listing truncated categories. Banner is injected into the prompt instructions immediately after "Analyze the pre-fetched data provided above."
Test Fixture Support
test/factories.ts
MakeOctokitOptions extended with graphqlPaginateResponses, useRealPaginatePlugin, and graphqlPagesByQuery to control stubbed pagination behavior. When useRealPaginatePlugin === true, stubs are wrapped with the real paginateGraphQL plugin to validate cursor-walking invariants.
Test Coverage
test/core/fetcher.test.ts
Extensive new tests covering: pagination merges of large comment sets (>100 items), safety-cap enforcement with truncation flags and structured warning logs, preservation of newest items (not oldest), nested per-review comment pagination, real paginate-plugin contract tests for cursor handling, and prompt banner injection when truncation is detected.
Documentation
IMPLEMENT.md, docs/operate/configuration.md, docs/operate/observability.md
Updated issue scope and commits; added configuration table entries for the four new MAX_FETCHED_* variables with truncation notes; added new "Data fetching safety caps" section explaining paginate behavior, cap triggers, truncation flags, and prompt warning integration plus alert heuristics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Suggested labels

type: docs 📋

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(fetcher): paginate GraphQL connections + MAX_FETCHED_* caps' clearly summarizes the main change: adding pagination and safety caps to the GraphQL fetcher.
Linked Issues check ✅ Passed The PR fully implements all coding requirements from issue #66: pagination via octokit.graphql.paginate, MAX_FETCHED_* caps with structured warnings, FetchedData.truncated flags, truncation warning banner in prompts, TOCTOU semantics preservation, and comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are within scope of issue #66: paginated queries, safety caps, truncation tracking, prompt warnings, documentation, test fixtures, config validation, and type updates directly support the stated objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value).


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes silent truncation of PR/issue GraphQL context by paginating connections (comments/reviews/review-comments/files), adding safety caps (MAX_FETCHED_*) plus a FetchedData.truncated signal, and surfacing that truncation in the generated agent prompt.

Changes:

  • Paginate GraphQL connections (and nested review inline comments via a follow-up query) instead of single-page first: 100 fetches.
  • Add configurable per-connection caps (MAX_FETCHED_*, default 500) with structured warn logs and FetchedData.truncated flags.
  • Inject a prompt warning banner when any connection was truncated; add/extend unit tests and docs.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/core/fetcher.ts Adds GraphQL pagination, safety caps, truncation flags, and nested review-comment follow-up pagination.
src/config.ts Adds MAX_FETCHED_* env vars (Zod schema + wiring).
src/types.ts Extends FetchedData with truncated flags.
src/core/prompt-builder.ts Adds truncation banner and injects it into the prompt instructions.
test/core/fetcher.test.ts Adds tests for pagination merge, TOCTOU ordering, caps/logging, nested review-comment pagination, and banner presence.
test/factories.ts Extends Octokit test double to support graphql.paginate + optional real plugin contract tests.
docs/operate/configuration.md Documents new MAX_FETCHED_* env vars.
docs/operate/observability.md Documents cap warn log shape and prompt banner behavior.
IMPLEMENT.md Updates implementation summary for issue #66.

Comment thread docs/operate/observability.md Outdated
Comment thread test/factories.ts
Comment thread IMPLEMENT.md Outdated
Comment thread src/core/fetcher.ts Outdated
Comment thread src/core/fetcher.ts
Comment on lines +133 to 137
pageInfo { hasNextPage endCursor }
}
}
pageInfo { hasNextPage endCursor }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: ❌ Not addressed — disagree on the premise

Summary: This claim doesn't match the plugin's actual behaviour. The nested comments.pageInfo block under reviews.nodes[] is unreachable to the plugin's DFS, so it cannot interfere with the outer reviews.pageInfo. Verified against plugin source.

Details: @octokit/plugin-paginate-graphql@^6.0.0 finds the paginated connection by depth-first search for the first node carrying a pageInfo property. Its DFS uses an isObject guard that explicitly excludes arrays:

// node_modules/@octokit/plugin-paginate-graphql/dist-src/object-helpers.js:2,17-18
const isObject = (value) => Object.prototype.toString.call(value) === "[object Object]";
// …
if (isObject(currentValue)) {
  if (currentValue.hasOwnProperty(searchProp)) {
    return currentPath;
  }

reviews.nodes is an Array, so Object.prototype.toString.call(nodes) returns "[object Array]" and the recursion never descends into per-review objects. The DFS therefore visits repository → pullRequest → reviews and finds reviews.pageInfo first — exactly what we want.

The nested comments.pageInfo is selected only as a per-review overflow signal so we know whether to fire the follow-up REVIEW_COMMENTS_QUERY (see src/core/fetcher.ts:517); it is never used as a paginate-target for the plugin.

The suggested alternative (comments.totalCount vs nodes.length) would also work, but it's not strictly an improvement — both signals require the same fetch and the current pageInfo.hasNextPage is the canonical GitHub GraphQL paginate signal. Leaving as-is.

Comment thread src/core/fetcher.ts Outdated
Comment thread src/core/fetcher.ts Outdated
Comment thread src/core/fetcher.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@IMPLEMENT.md`:
- Around line 11-12: The documentation still references legacy cursor variable
names ($afterFiles, $afterComments, $afterReviews) while the implementation
standardizes on $cursor and the fetcher (src/core/fetcher.ts) treats the exact
variable name as part of the plugin contract; update the IMPLEMENT.md text to
replace those legacy names with $cursor, mention that all paginated queries must
expose a single $cursor variable, and note the exception for per-review nested
pagination handled by REVIEW_COMMENTS_QUERY so readers understand why a separate
request is used.

In `@src/core/fetcher.ts`:
- Around line 456-460: The current Promise.all that fans out
fetchRemainingReviewComments over overflowReviews can spawn too many concurrent
GraphQL calls and fail-fast on a single transient error; change the logic that
produces overflowResults to run the per-review fetches through a bounded
concurrency pool (limit concurrent fetchRemainingReviewComments calls to a small
number, e.g., 3–5) and wrap each call with the retry helper from
src/utils/retry.ts (use validated maxAttempts, initialDelayMs, maxDelayMs,
backoffFactor per the module’s validation rules) so each fetch is retried on
transient errors and failures degrade per-review instead of rejecting the whole
batch; keep references to overflowReviews, fetchRemainingReviewComments, and the
retry utility so reviewers can locate the change.
- Around line 439-490: The cap is being applied before TOCTOU filtering, which
can drop pre-trigger items; instead, run filterByTriggerTime (and remove
minimized items) on the raw sets first, then apply applyCap to those filtered
results and set truncated flags from the resulting capped objects; also compute
overflowReviews from the filtered reviews (not from cappedReviews.items) so
fetchRemainingReviewComments only paginates review comment threads that are
pre-trigger, and rebuild cappedReviewComments from the filtered+merged review
comments before applying applyCap.

In `@test/core/fetcher.test.ts`:
- Around line 656-706: The test currently asserts a hard-coded cap of 500;
instead read the actual fetcher cap used at runtime and use it for assertions.
Update the test around fetchGitHubData(ctx) to compute const cap =
Number(process.env.MAX_FETCHED_COMMENTS) || 500 (or import the fetcher
config/constant if available) and then replace the literal 500 in
expect(result.comments.length).toBe(500) and
expect(capWarn?.[0]["cap"]).toBe(500) with cap, and adjust any comments that
reference 500 so expectations reflect the configured MAX_FETCHED_COMMENTS value.
This ensures the test uses the same cap as the fetcher (symbols to touch:
fetchGitHubData, result.comments, log.warn mock and
MAX_FETCHED_COMMENTS/process.env).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74632cec-4779-425c-91df-98adf31654a0

📥 Commits

Reviewing files that changed from the base of the PR and between 95856bc and fbcbd2f.

📒 Files selected for processing (9)
  • IMPLEMENT.md
  • docs/operate/configuration.md
  • docs/operate/observability.md
  • src/config.ts
  • src/core/fetcher.ts
  • src/core/prompt-builder.ts
  • src/types.ts
  • test/core/fetcher.test.ts
  • test/factories.ts

Comment thread IMPLEMENT.md Outdated
Comment thread src/core/fetcher.ts Outdated
Comment thread src/core/fetcher.ts Outdated
Comment thread test/core/fetcher.test.ts Outdated
@chrisleekr-bot

chrisleekr-bot Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

bot workflow resolve — succeeded

🔎 Resolve iteration complete — 0 failing checks, 12 open comment threads (some may already be resolved).

Resolve: PR #95 — paginate GraphQL fetcher + MAX_FETCHED_* caps

Summary

12 review comments from Copilot and CodeRabbit triaged on PR #95 (branch fix/issue-66-paginate-graphql).

  • 11 Valid / Partially Valid → fixed and resolved.
  • 1 Invalid → reply with evidence, left open.

The branch was already up-to-date with main (0 commits behind, 2 ahead), so no rebase was needed. Pre-existing CI was already green at fbcbd2f. One follow-up commit (e9e7f78) ships the review-driven fixes; the Lint & Test, Docs, Secrets Scan, Generate Labels, CodeQL, and build checks are all green at the new HEAD.

CI status

Head SHA: e9e7f78 (fix(fetcher): isolate paginate vars, bound review fan-out, filter before cap)

Workflow Status
CI / Lint & Test ✅ pass (1m35s)
Docs ✅ pass
Secrets Scan / Gitleaks ✅ pass (×2)
Generate Labels ✅ pass
CodeQL / Analyze (actions) ✅ pass
CodeQL / Analyze (javascript-typescript) ✅ pass
build ✅ pass
CodeRabbit ✅ Review skipped (auto-skipped re-review)
Dev Release ⚠️ startup_failure — pre-existing on 54e7465 + fbcbd2f too; unrelated to this fix and out of scope for the resolve workflow

Review comments

✅ Addressed (Valid) — 10 threads

# File Reviewer Issue Action
1 docs/operate/observability.md:113 copilot Caps don't bound fetch-time memory Reworded in fbcbd2f; reaffirmed in e9e7f78
2 test/factories.ts:16 copilot @octokit/plugin-paginate-graphql imported transitively Added to devDependencies (e9e7f78)
3 IMPLEMENT.md:11 copilot $afterFiles/$afterComments/$afterReviews no longer used Documented $cursor contract (fbcbd2f)
4 src/core/fetcher.ts:415 copilot Shared vars object races across 3 concurrent paginate() calls Each call now gets a fresh { owner, repo, number } literal (e9e7f78)
6 src/core/fetcher.ts:442 copilot applyCap runs before isMinimized filter on PR path Reordered filter-then-cap (e9e7f78)
7 src/core/fetcher.ts:459 copilot Per-review overflow fan-out unbounded Bounded to 5 in-flight via pMap + retryWithBackoff (e9e7f78)
8 src/core/fetcher.ts:564 copilot Same filter-then-cap order needed on issue path Reordered (e9e7f78)
9 IMPLEMENT.md:12 coderabbit Documented cursor variable name Already accurate in fbcbd2f
11 src/core/fetcher.ts:460 coderabbit Bound + retry per-review fan-out Same fix as #7 (e9e7f78)
12 test/core/fetcher.test.ts:706 coderabbit Cap literal 500 should derive from config Test now reads config.maxFetchedComments (e9e7f78)

✅ Partially Addressed — 1 thread

# File Reviewer Issue Action
10 src/core/fetcher.ts:490 coderabbit Cap after TOCTOU filter for both reviews and comments Comments / reviewComments now go filter-then-cap (full fix). Reviews still go cap-then-fan-out for deliberate cost-bounding — fan-out is bounded but the reviews-list itself isn't filtered by triggerTime since reviews don't currently surface to the prompt directly. Resolved with a follow-up note. (e9e7f78)

❌ Invalid — 1 thread (replied, NOT resolved)

# File Reviewer Issue Verdict
5 src/core/fetcher.ts:143 copilot Claims PR_REVIEWS_QUERY selects "two pageInfo blocks" and the plugin can only follow the first, breaking outer pagination Invalid: verified in node_modules/@octokit/plugin-paginate-graphql/dist-src/object-helpers.js. The plugin's DFS uses isObject = (v) => Object.prototype.toString.call(v) === "[object Object]", which excludes arrays. reviews.nodes is an array, so the recursion never descends into per-review objects — the outer reviews.pageInfo is found first. The nested comments.pageInfo is selected only as a per-review overflow signal consumed by app code (see fetcher.ts:517), never as a paginate target for the plugin. Reply posted with evidence; thread left open for human adjudication.

Commits

  • e9e7f78fix(fetcher): isolate paginate vars, bound review fan-out, filter before cap

Replies posted

12 inline replies posted via gh api repos/.../pulls/95/comments/<id>/replies. Reply IDs: 3177390704, 3177390731, 3177390756, 3177390781, 3177390847, 3177390858, 3177390864, 3177390878, 3177390942, 3177390966, 3177390977, 3177390980.

Threads resolved

11 threads resolved via the resolveReviewThread GraphQL mutation. 1 thread (#5, the invalid pageInfo claim) deliberately left open.

Outstanding

Verification

  • bun run typecheck — clean
  • bun test test/core/fetcher.test.ts — 32 pass / 0 fail / 75 expect calls
  • NODE_OPTIONS='--max-old-space-size=4096' bunx eslint . — 0 errors / 289 warnings (all pre-existing in unrelated files)
  • bun run format — clean
  • bun run scripts/check-docs-citations.ts — clean
  • bun run scripts/check-docs-versions.ts — clean
  • CI on e9e7f78 — all green (see CI status table above)

FIX_ATTEMPTS_CAP=3 budget: 1 attempt used (e9e7f78).

cost: $10.2145 · turns: 132 · duration: 1206s

…ore cap

Address review feedback on PR #95:

- Pass a fresh `{ owner, repo, number }` object to each parallel
  `octokit.graphql.paginate` call. The plugin mutates `parameters.cursor`
  in place between pages; sharing one object across the three concurrent
  PR queries produced a cursor race.
- Wrap `fetchRemainingReviewComments` in `retryWithBackoff` and bound
  the per-review overflow fan-out to `REVIEW_OVERFLOW_CONCURRENCY = 5`
  via a small `pMap` helper. A PR with hundreds of overflowing reviews
  no longer storms the GitHub API in parallel, and a single review's
  pagination failure no longer aborts the whole fetch.
- Filter `isMinimized` + `filterByTriggerTime` BEFORE `applyCap` on both
  the PR and issue paths so the cap reflects items that actually reach
  the prompt; minimized / post-trigger items can no longer crowd out
  legitimate context.
- Add `@octokit/plugin-paginate-graphql` to `devDependencies` so the
  test factory's type imports resolve from a declared dep rather than
  via the transitive `octokit` chain.
- IMPLEMENT.md + docs/operate/observability.md: clarify that caps bound
  the merged result reaching the prompt (not fetch-time memory) and
  document the `$cursor` / single-`pageInfo` plugin contract.
- Pin the safety-cap test to `config.maxFetchedComments` rather than
  the literal 500 so the assertion tracks any future default change.

Co-authored-by: chrisleekr-bot[bot] <chrisleekr-bot[bot]@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@chrisleekr
chrisleekr merged commit f728ecd into main May 3, 2026
9 checks passed
@chrisleekr
chrisleekr deleted the fix/issue-66-paginate-graphql branch May 3, 2026 00:15
chrisleekr pushed a commit that referenced this pull request May 3, 2026
# [1.9.0](v1.8.0...v1.9.0) (2026-05-03)

### Bug Fixes

* **checkout:** fetch PR base branch so origin/<baseBranch> resolves (closes [#74](#74)) ([#96](#96)) ([71f83a6](71f83a6))
* **fetcher:** paginate GraphQL connections + MAX_FETCHED_* caps (closes [#66](#66)) ([#95](#95)) ([f728ecd](f728ecd))
* **triage:** accept note-only evidence; raise research max-turns to 200 ([#97](#97)) ([3b6036c](3b6036c))
* **workflow:** fix release.yml ([#98](#98)) ([cb43d69](cb43d69))

### Features

* **workflows:** publish SLSA provenance + SBOM attestations on every release tag (closes [#58](#58)) ([#94](#94)) ([95856bc](95856bc))
@chrisleekr

Copy link
Copy Markdown
Owner

🎉 This PR is included in version 1.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(pipeline): GraphQL fetcher silently truncates PR/issue context past 100 items

2 participants