diff --git a/.agents/upstream-review.md b/.agents/upstream-review.md index e3845197d..c8f16ebb9 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, @@ -1592,20 +1597,34 @@ 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.** ## 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 +1635,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 | +| --- | -------- | ----------- | ------------ | ------------ | 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/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; } 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),