From 7e50ef1ab3f775a88bdd528d35f8154baaf6f4f6 Mon Sep 17 00:00:00 2001 From: Trevor Walker Date: Tue, 18 Aug 2026 12:16:36 -0600 Subject: [PATCH 1/4] fix(pull-requests): stop the GitHub GraphQL cost estimate ratcheting 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. --- .../sourceControl/githubGraphQlBudget.test.ts | 56 +++++++++++++++++++ .../src/sourceControl/githubGraphQlBudget.ts | 19 ++++++- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts index a166bf0db..bcdc583ca 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.test.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.test.ts @@ -18,6 +18,13 @@ function rateLimit(remaining: number, limit = 5_000, resetAt = RESET_AT): string }); } +/** The same answer with its price named, for the tests that turn on how much a read cost. */ +function rateLimitCosting(cost: number, remaining: number, limit = 5_000): string { + return JSON.stringify({ + data: { rateLimit: { cost, limit, remaining, resetAt: RESET_AT } }, + }); +} + describe("GitHub GraphQL budget", () => { it.effect("adds rate metadata to a read query", () => Effect.gen(function* () { @@ -160,6 +167,55 @@ describe("GitHub GraphQL budget", () => { }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), ); + // The reserve is deducted from what GitHub last said, so an honest answer always looks + // higher than the running figure. Judging staleness against it discarded every truthful + // response, leaving the estimate to climb only when a read cost more than the one before — + // one 100-point read then priced every 1-point read at 100 for the rest of the window. + it.effect("takes a cheaper answer that lands above the reserved figure", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + yield* budget.observe("github.com", rateLimitCosting(100, 600)); + + // Reserves 100 against 600, which leaves exactly the floor. + expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( + "rateLimit", + ); + // GitHub spent one point, not another hundred. 599 is above the reserved 500 and below + // the 600 it last reported, so it is news and not an answer that arrived out of order. + yield* budget.observe("github.com", rateLimitCosting(1, 599)); + + expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( + "rateLimit", + ); + }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), + ); + + // A read that never answers — a network flap, a `gh` failure — keeps its reserve, because + // only the success path observes. The next answer carries the host's own figure, so it puts + // the estimate back rather than leaving the leak to accumulate until the window resets. + it.effect("clears reserves left behind by reads that never answered", () => + Effect.gen(function* () { + yield* TestClock.setTime(BEFORE_RESET); + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + yield* budget.observe("github.com", rateLimitCosting(100, 900)); + + for (let attempt = 0; attempt < 4; attempt += 1) { + expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( + "rateLimit", + ); + } + // Four reserves took the estimate to the floor while GitHub barely moved. + yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); + + yield* budget.observe("github.com", rateLimitCosting(1, 899)); + + expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( + "rateLimit", + ); + }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), + ); + it.effect("ignores malformed or partial rate metadata", () => Effect.gen(function* () { yield* TestClock.setTime(BEFORE_RESET); diff --git a/apps/server/src/sourceControl/githubGraphQlBudget.ts b/apps/server/src/sourceControl/githubGraphQlBudget.ts index 9c43de8e0..e4cebd43f 100644 --- a/apps/server/src/sourceControl/githubGraphQlBudget.ts +++ b/apps/server/src/sourceControl/githubGraphQlBudget.ts @@ -13,7 +13,19 @@ const RATE_LIMIT_SELECTION = "rateLimit { cost limit remaining resetAt }"; interface GraphQlBudgetSnapshot { readonly cost: number; readonly limit: number; + /** + * What is left after subtracting the reads issued since the last answer. Reserving keeps + * concurrent reads from each spending the same points, so this runs below what GitHub last + * said and is the figure the pause is judged against. + */ readonly remaining: number; + /** + * The last figure GitHub itself reported, untouched by reservations. Staleness is judged + * against this and not against `remaining`: a reservation always pushes `remaining` below + * the truth, so comparing an honest answer with it would read as "quota went up" and throw + * the answer away. + */ + readonly observedRemaining: number; readonly resetAtMs: number; } @@ -59,7 +71,9 @@ function snapshotFrom(raw: string): GraphQlBudgetSnapshot | null { return null; } const resetAtMs = Date.parse(resetAt); - return Number.isFinite(resetAtMs) ? { cost, limit, remaining, resetAtMs } : null; + return Number.isFinite(resetAtMs) + ? { cost, limit, remaining, observedRemaining: remaining, resetAtMs } + : null; } catch { return null; } @@ -125,7 +139,8 @@ export const make = Effect.gen(function* () { if ( previous !== undefined && (snapshot.resetAtMs < previous.resetAtMs || - (snapshot.resetAtMs === previous.resetAtMs && snapshot.remaining >= previous.remaining)) + (snapshot.resetAtMs === previous.resetAtMs && + snapshot.remaining >= previous.observedRemaining)) ) { return current; } From 0b4bb24010a94560abdbb31e1c2bf03b24a548fd Mon Sep 17 00:00:00 2001 From: Trevor Walker Date: Tue, 18 Aug 2026 12:16:37 -0600 Subject: [PATCH 2/4] fix(pull-requests): 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 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. --- .../pullRequest/GitHubPullRequestCli.test.ts | 6 ++++- .../src/pullRequest/GitHubPullRequestCli.ts | 6 ++++- .../pullRequestDetail.logic.test.ts | 27 +++++++++++++++++++ .../pullRequest/pullRequestDetail.logic.ts | 16 ++++++++--- 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 33d0d120c..f3bb2941b 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -2216,7 +2216,11 @@ layer("GitHubPullRequestCli.layer", (it) => { commentCount: 3, nextCommentsCursor: "Y3Vyc29yOjI", }); - assert.isTrue(conversation.truncated); + // Paged, as the name says, and not truncated: the cursor travels with the thread and the + // reader takes the rest whenever they want it. Callers treat the flag as "this is all the + // host will give you" and hide the header's approval count on it, which a reply that is + // one press away must not cost. + assert.isFalse(conversation.truncated); }), ); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 6e09de769..8cb312a7e 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -1586,7 +1586,11 @@ export const make = Effect.gen(function* () { // GitHub's own count of each thread, so the number the page shows is the host's even // where a bound kept some of the words on GitHub. commentCount: entries.reduce((total, entry) => total + entry.commentCount, 0), - truncated: cursor !== null || entries.some((entry) => entry.nextCommentCursor !== null), + // Only the thread walk itself. A thread whose replies stop short carries its cursor + // to the client, which reads the rest on demand, so it is paged and not truncated — + // and readers of this flag drop a whole feature when it is set (the header's approval + // count goes away), which a reachable reply must not cost. + truncated: cursor !== null, reactions, reactionsById, reviewers, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index c2108835a..f581f3071 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -782,6 +782,33 @@ describe("one finding handed over on its own", () => { expect(handoff.prompt).not.toContain("rename the helper"); }); + it("says so when the thread was still going past what was read", () => { + // Replies past the first page arrive only when someone presses Load more, and an agent + // cannot. Handing over the opening of an argument as though it were the whole of one hides + // exactly the part that settled it. + const handoff = buildFixFindingHandoff({ + ...base, + finding: { + kind: "thread", + thread: { ...reviewThread, nextCommentsCursor: "Y3Vyc29yOjI" }, + }, + }); + + expect(handoff.reviewComments[0]?.text).toContain("reviewer: rename the helper"); + expect(handoff.reviewComments[0]?.text).toContain( + "(Later replies in this thread were not read.)", + ); + }); + + it("says nothing of the kind about a thread that was read whole", () => { + const handoff = buildFixFindingHandoff({ + ...base, + finding: { kind: "thread", thread: reviewThread }, + }); + + expect(handoff.reviewComments[0]?.text).toBe("reviewer: rename the helper"); + }); + it("quotes a review remark, which has no line to attach it to", () => { const handoff = buildFixFindingHandoff({ ...base, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 6b83681e1..4c805cb5a 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -446,13 +446,21 @@ function reviewThreadContext( thread.line === null ? "file" : `L${thread.line}${thread.side === "left" ? " (before)" : ""}`, // Bot bookkeeping lives in HTML comments and would otherwise eat the length bound before // the finding itself got any of it. + // + // A long thread arrives as its opening replies plus a cursor, and the rest is read only + // when someone presses Load more. The agent cannot press it, so a thread that was still + // going says so — otherwise the handoff reads as a settled discussion when what it holds + // is the start of one, and the resolution is the part that matters. text: bounded( - thread.comments - .flatMap((comment) => { + [ + ...thread.comments.flatMap((comment) => { const body = visibleBody(comment.body); return body === null ? [] : [`${comment.author?.login ?? "ghost"}: ${body}`]; - }) - .join("\n"), + }), + ...(thread.nextCommentsCursor === undefined + ? [] + : ["(Later replies in this thread were not read.)"]), + ].join("\n"), ), diff: "", fenceLanguage: inferReviewCommentFenceLanguage(thread.path), From 2267ade1763a9c7b34ac2384bae7723e26a5d00c Mon Sep 17 00:00:00 2001 From: Trevor Walker Date: Tue, 18 Aug 2026 12:16:50 -0600 Subject: [PATCH 3/4] chore(upstream): retire DEF-3 and DEF-4 from the deferred register --- .agents/upstream-review.md | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/.agents/upstream-review.md b/.agents/upstream-review.md index e3845197d..209d2c5f7 100644 --- a/.agents/upstream-review.md +++ b/.agents/upstream-review.md @@ -1511,7 +1511,12 @@ included. `pnpm-lock.yaml` is untouched by this batch — no change set adds a d | N10 | `13458e651` / `#7296` | adopted | `05c123968` | One `mx-0!` class centering the context usage meter's SVG. | | N11 | `3723722f7` / `#7364` | adopted | `1d1851a91` | Bot cleanup removing a second `expect(only()).toBe(true)`. Verified genuinely redundant — `claimWorkspaceBasenameLookup` returns a pure comparison, so the repeat asserts nothing new. Adopted only to keep the file aligned with upstream and conflict-free later. | -### Inherited defects found by review, deferred to a follow-up +### Inherited defects found by review, fixed in a follow-up + +> **Closed.** Both were fixed on `fix/pull-request-quota-followups` the same day, +> with regression tests that were checked against the unfixed code first. DEF-3 +> and DEF-4 have been retired from the register. The account below is kept +> because it explains why the batch shipped without them. An `xhigh` review of the integration branch confirmed all five Pylon-first resolutions are clean — including the `BrowserDeviceToolbar` equivalence claim, @@ -1602,10 +1607,11 @@ visible symptom could not be reproduced live and rests on source reading. ## Deferred register -_DEF-1 and DEF-2 were adopted on 2026-08-11 (see the sixth batch above). The -register was empty from then through 2026-08-18, when the batch review added -DEF-3 and DEF-4 — both inherited upstream defects, not upstream work awaiting a -decision. Entries are removed once adopted or skipped._ +_The register is currently empty. DEF-1 and DEF-2 were adopted on 2026-08-11 +(see the sixth batch above). DEF-3 and DEF-4 were opened and closed on +2026-08-18: the 2026-08-18 batch review found them, the batch shipped without +them so the adoption stayed faithful, and `fix/pull-request-quota-followups` +fixed both the same day. Entries are removed once adopted, skipped, or fixed._ Upstream work that has been reviewed and consciously _not_ adopted yet, with the condition that should trigger a fresh look. Entries stay here until they @@ -1616,7 +1622,5 @@ Every review must read this register before reporting new candidates, re-evaluate each `Revisit when` against the current upstream head, and report the outcome. See Phase 2.5 of the `review-t3-upstream` skill. -| ID | Upstream | Deferred on | Revisit when | Why deferred | -| ----- | --------------------------------------------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| DEF-3 | `ba46f922a` / `#6466` + `c7e6d711d` / `#7077` | 2026-08-18 | Immediately — this is queued Pylon work, not a wait on upstream. Close it when a Pylon branch narrows GitHub's `truncated` to the thread-list cursor and feeds the full thread to the prompt builder, or when `git log bab4b6f02..t3code-upstream/main -- apps/server/src/pullRequest/GitHubPullRequestCli.ts apps/web/src/components/pullRequest/pullRequestDetail.logic.ts` shows upstream fixed it first. | `truncated` narrowed to "a thread has an 11th reply" while three readers still treat it as "the conversation is short of the host": the approval badge is suppressed, the handoff prompt gains a spurious notice, and the agent receives only a thread's oldest 10 comments. Deferred to keep the adoption PR faithful; see the 2026-08-18 batch notes. | -| DEF-4 | `ba46f922a` / `#6466` | 2026-08-18 | Same follow-up branch as DEF-3. Also re-check on any upstream change to `apps/server/src/sourceControl/githubGraphQlBudget.ts`. | `observe` compares GitHub's true `remaining` against a locally-reserved figure, so honest responses are always discarded and the cost estimate only ratchets upward; a failed call leaks its reservation. Fails safe and resets hourly, which is why it is queued rather than blocking. | +| ID | Upstream | Deferred on | Revisit when | Why deferred | +| --- | -------- | ----------- | ------------ | ------------ | From 2e806b9bab726a1daf313ac3146e5307f932f805 Mon Sep 17 00:00:00 2001 From: Trevor Walker Date: Tue, 18 Aug 2026 12:37:51 -0600 Subject: [PATCH 4/4] chore(upstream): record what the client pass verified, and right-size DEF-3 --- .agents/upstream-review.md | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/.agents/upstream-review.md b/.agents/upstream-review.md index 209d2c5f7..c8f16ebb9 100644 --- a/.agents/upstream-review.md +++ b/.agents/upstream-review.md @@ -1597,11 +1597,24 @@ at runtime rather than by lint alone: **zero** native `title` attributes remain elements in the rendered DOM. `#7077`'s Reviewers row renders above the conversation and the Summary/Timeline/Code tabs load. -**Two items were not reachable.** `#7219`'s usage breakdown never finished loading against -the seeded 334 MB database (the page reported one request waiting longer than 15s), and no -pull request in reach carried either an approval or an 11-comment thread, so `#6466`'s **Load -more comments** and `#7077`'s verdict rows were not exercised — which is also why DEF-3's -visible symptom could not be reproduced live and rests on source reading. +**`#7219` was verified on a second pass**, once the usage query was given long enough to +finish against the seeded 334 MB database. The breakdown shows 29 daily rows, Aug 18 back to +Jul 20; restoring the old `.slice(0, 8)` and reloading with the browser cache disabled drops +it to 8, ending at Aug 11. Note the first attempt at that negative control reported 29 rows +for both sides — Vite had reused a warm module graph, so an A/B against a running dev server +needs the cache disabled to mean anything. + +**`#6466`'s Load more comments and `#7077`'s verdict rows remain unexercised.** No repository +in reach carries a single approved review, GitHub does not allow approving one's own pull +request, and a disposable clone of `pingdotgg/t3code` added as a project did not surface in +the pull request list, whose repository discovery did not pick it up. + +**That search right-sized DEF-3.** Its symptom needs one review thread longer than ten +comments, and across upstream's most-reviewed pull requests the largest thread anywhere was +**five** — `#4849` (100 review comments, largest thread 5), `#7077` (61, 5), `#6466` (24, 3), +`#7107` (14, 3). The defect is real and the fix stands, since the flag asserts something +untrue and its reader drops a feature on it, but it fires on unusually long single threads +rather than on busy pull requests generally. **Mobile (N7, N8) was not exercised at all.**