Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions .agents/upstream-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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 |
| --- | -------- | ----------- | ------------ | ------------ |
6 changes: 5 additions & 1 deletion apps/server/src/pullRequest/GitHubPullRequestCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}),
);

Expand Down
6 changes: 5 additions & 1 deletion apps/server/src/pullRequest/GitHubPullRequestCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
56 changes: 56 additions & 0 deletions apps/server/src/sourceControl/githubGraphQlBudget.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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* () {
Expand Down Expand Up @@ -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);
Expand Down
19 changes: 17 additions & 2 deletions apps/server/src/sourceControl/githubGraphQlBudget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 12 additions & 4 deletions apps/web/src/components/pullRequest/pullRequestDetail.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading