Skip to content

fix(pull-requests): correct two quota and truncation defects from the 2026-08-18 batch - #47

Merged
rynfar merged 4 commits into
pylonfrom
fix/pull-request-quota-followups
Aug 18, 2026
Merged

fix(pull-requests): correct two quota and truncation defects from the 2026-08-18 batch#47
rynfar merged 4 commits into
pylonfrom
fix/pull-request-quota-followups

Conversation

@rynfar

@rynfar rynfar commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Two defects the xhigh review of #46 found in the upstream work that batch adopted. Both exist in upstream main; #46 shipped without them so the adoption stayed faithful, and they were tracked as DEF-3 and DEF-4 in the review ledger.

Every test here was run against the unfixed code first and fails there.

A pageable review thread is not a truncated one

#6466 cut review threads to their first ten comments and left the rest behind a cursor the client reads on demand — but kept marking the whole conversation truncated when any thread had an eleventh reply. Before that change the flag meant "we walked ten pages of a hundred and GitHub still had more", so it was effectively never set. Three callers were written against that older meaning:

  • The header hides #7077's approval count on it. That is the worse of the two, because the verdicts it counts come from gh pr view --json reviews, which the provider's own comment says is never truncated — the gate keys on a signal unrelated to the data it guards. One long thread and the headline of #7077 disappears.
  • The handoff prompt warns that comments may be missing.
  • Both now fire on ordinary pull requests.

The flag now covers only the thread walk itself, which is the part no cursor reaches. What a page really costs is said where it is actually true: a thread handed to an agent notes that later replies went unread, since the agent cannot press Load more and the resolution of an argument is usually its point.

Worth noting the shared gate is right for GitLab, Bitbucket, and Azure DevOps, whose commentsTruncated comes from conversation reads that do carry verdicts. The fix belongs at GitHub's truncated, not at the gate.

The GraphQL cost estimate only ratcheted upward

Reserving a query's cost pushes the local remaining below what GitHub last reported, and the out-of-order guard compared incoming answers against that reserved figure. So every honest response looked like quota going up and was discarded along with its cost, limit, and remaining. The estimate could only move when a read cost more than the one before it: one 12-point review-threads read priced every later 1-point read at 12 for the rest of the window, and automatic reads could pause on a floor the host was nowhere near.

Staleness is now judged against the last figure GitHub itself reported, which is what the guard meant all along — a genuinely stale out-of-order answer is still rejected, and the existing test for that still passes. Accepting an answer also resets the running total to the host's own number, so a reservation left behind by a call that failed before it could report no longer accumulates.

This one failed safe, which is why it was queued rather than blocking: reads paused early instead of quota being overspent, and the hourly reset cleared it.

Verification

Server 30 files / 628 tests, web pull request suite 15 files / 282 tests. Typecheck clean across contracts, client-runtime, web, and server. Lint 0 errors.

.agents/upstream-review.md retires DEF-3 and DEF-4; the deferred register is empty again.

Built with Claude Opus 5 (1M context) in Claude Code.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.

rynfar added 3 commits August 18, 2026 12:16
Reserving a query's cost pushes the local remaining below what GitHub last
reported, and the out-of-order guard compared incoming answers against that
reserved figure. Every honest response therefore looked like quota going up
and was discarded along with its cost, so the estimate only moved when a read
cost more than the one before it: one 12-point review-threads read priced
every later 1-point read at 12 for the rest of the window, and reads could
pause on a floor the host was nowhere near.

Staleness is now judged against the last figure GitHub itself reported, which
is what the guard meant all along. Accepting an answer also resets the running
total to the host's own number, so a reservation left behind by a call that
failed before it could report no longer accumulates.

Both tests were run against the unfixed code first and fail there.
#6466 cut review threads to their first ten comments and left the rest behind
a cursor the client reads on demand, but kept marking the conversation
truncated when any thread had an eleventh reply. Three callers read that flag
as "this is all the host will give you": the header hides #7077's approval
count on it, the handoff prompt warns that comments may be missing, and both
now fire on ordinary pull requests. The approval count is the worse of the
two, since the verdicts it counts come from `gh pr view --json reviews`,
which is never truncated at all.

The flag now covers only the thread walk itself, which is the part no cursor
reaches. What a page really costs is said where it is true: a thread handed to
an agent notes that later replies went unread, because the agent cannot press
Load more and the resolution of an argument is usually its point.

Regression tests on both halves fail against the unfixed code.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Aug 18, 2026
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Thread transfer impact

⚠️ The latest CI run did not produce a thread transfer result for 2e806b9.

This comment will update automatically after the next completed run.

@rynfar

rynfar commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Live verification, and a correction on severity

#7219 verified with an A/B on the merged batch: the usage breakdown shows 29 daily rows (Aug 18 → Jul 20). Restoring the old .slice(0, 8) and reloading with the cache disabled drops it to 8 (Aug 18 → Aug 11), so the cap really was hiding three weeks of the window.

The truncation defect could not be reproduced live, and the reason matters. Firing it needs a single review thread with more than ten comments. Across upstream's most-reviewed pull requests the largest thread anywhere was five:

PR review comments largest single thread
#4849 100 5
#7077 61 5
#6466 24 3
#7107 14 3

So the suppressed approval count is real but rarer than I described when I opened this PR — it takes an unusually long single thread, not merely a busy pull request. The code is still wrong and the fix still stands: the flag says something untrue, and the reader that acts on it drops a feature. But it is not the everyday occurrence my earlier wording implied, and the ledger has been corrected to match.

Neither the approval badge nor Load more comments could be exercised in a client: no repository in reach carries a single approved review, GitHub does not allow approving one's own pull request, and no thread anywhere reaches eleven comments. Coverage rests on the four regression tests, each of which fails against the unfixed code.

@rynfar
rynfar merged commit c2dc08e into pylon Aug 18, 2026
11 checks passed
@rynfar
rynfar deleted the fix/pull-request-quota-followups branch August 18, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant