From c3f4877d64a500aafb5c33f9e25280e457f344f0 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 08:51:40 +0200 Subject: [PATCH 1/4] feat(inbox): a review you skipped comes back when someone asks again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A skip settles a PR for good: `SETTLED_BY_YOU` keeps the poll from dragging the row back on every push, which is right — cerber does not get to overrule a decision you made. But it treated every later review request as the same one you had already answered, and a request that was withdrawn and re-added is not: your skip answered the request that was open when you made it, and cannot have answered one that came after. The row stayed invisible while someone waited. The poll now does filing's mirror image. Settling stamps `settledAt` — the decision has to be dated as it is made, because `updatedAt` moves every time you merely open a settled review and it refreshes. For rows GitHub is asking about right now, `reopenIfAskedAgain` reads the last `REVIEW_REQUESTED_EVENT` naming you (one GraphQL call, on the same 30-minute-per-artifact leash as the state checks) and, when it is newer than the stamp, puts the row back: `ready` if it has a draft, `awaiting` if it does not, with `settledAt` and cerber's own `filed` note cleared. From there the ordinary rules apply — the freshness guard re-drafts it if the head moved and leaves the draft alone if it did not. Team requests never count: `stillRequested` honours them because refusing to file work away is the safe side of that question, and this one undoes a decision of yours, where the safe side is doing nothing unless somebody named you. Rows settled before the field existed fall back to the latest moment cerber can prove the decision came after — `filed.at`, or `run.finishedAt`, since you cannot skip a draft that does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 8 +- docs/lifecycle.md | 70 ++++++++-- src/core/artifact.ts | 12 ++ src/core/autosend.test.ts | 1 + src/core/gh.test.ts | 37 ++++- src/core/gh.ts | 61 +++++++++ src/core/refresh.test.ts | 1 + src/core/refresh.ts | 4 + src/core/revise.test.ts | 1 + src/core/send.test.ts | 1 + src/core/state.test.ts | 1 + src/runner/chat.test.ts | 1 + src/runner/review-merge.test.ts | 26 ++++ src/runner/review.test.ts | 1 + src/runner/review.ts | 3 +- src/server/chat.test.ts | 1 + src/server/daemon.test.ts | 231 ++++++++++++++++++++++++++++++++ src/server/daemon.ts | 124 +++++++++++++++-- src/server/guards.test.ts | 16 +++ src/server/index.ts | 5 + 20 files changed, 586 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 738a75b..864b432 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,7 +98,13 @@ any of them — it names the files each rule lives in. `run.trigger` spares a draft you asked for after having spoken; the withdrawal reason is stricter still (poll-written drafts only, and it confirms against the PR with `fetchReviewRequests` before acting, because - there its only evidence is a search index's silence). Config's `daemon` + there its only evidence is a search index's silence). The poll also does the + reverse (`reopenIfAskedAgain`): a settled row comes back when someone + requested your review *again* after you settled it — the one thing that + reopens a `reviewed`/`skipped` row, since your skip answered the request that + was open at the time and cannot have answered a later one. It turns on + `settledAt` dating the decision and the last `REVIEW_REQUESTED_EVENT` naming + you; team requests never count, because this undoes a decision of yours. Config's `daemon` block is re-read every poll, so cockpit toggles apply without a restart. Each poll also publishes what it found (`status.awaiting`): the queue filters on what you settled locally, which stops matching what GitHub asks you for the diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 5f6ec78..0b64061 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -24,7 +24,7 @@ Four things write the artifact, and most questions in this document are really | Writer | Where | What it may do | | --- | --- | --- | -| **the poll** | `src/server/daemon.ts` | create stubs, archive, file, delete stubs, start drafts — and, with `--auto-send`, submit and mark `sent` | +| **the poll** | `src/server/daemon.ts` | create stubs, archive, file, reopen a settled row someone asked you about again, delete stubs, start drafts — and, with `--auto-send`, submit and mark `sent` | | **startup** | `reconcileRunning`, `src/core/state.ts` | on boot, turn a leftover `running` into `failed` and error a pending chat turn | | **the runner** | `src/runner/review.ts`, `chat.ts` | fill in summary / chapters / comments / verdict | | **you** | the cockpit → `src/server/index.ts` | edit, mark reviewed/skipped, send, re-review, chat — and, just by opening a review, the automatic refresh that rewrites `pr`, `diff`, the comment anchors and `refresh` | @@ -55,7 +55,13 @@ Two derived groupings drive most behaviour: - **`SETTLED = [sent, reviewed, skipped]`** (`web/src/inbox.ts`) — out of the live queue. - **`SETTLED_BY_YOU = [reviewed, skipped]`** (`src/runner/review.ts`) — a new - push must not drag these back. + push must not drag these back. A new *request* is the one thing that can + (§5, "Asked again"). + +Settling one of those stamps `settledAt`. That field exists for exactly one +question — did this review request come before or after your decision — and +`updatedAt` cannot answer it, because opening a settled review refreshes it and +moves that field forward long afterwards. `archived` is *not* a status. It is `pr.state !== "OPEN"` — merged or closed. It moves a row out of the live tabs and out of **settled** and **sent**, and @@ -103,11 +109,17 @@ running ──cerber restarted─► failed (reconcileRunning) does not: ``` -any unsent row ──you mark it────────────────► reviewed | skipped -ready ──the poll finds GitHub moved past it─► reviewed (+ `filed`) +any unsent row ──you mark it────────────────► reviewed | skipped (+ `settledAt`) +ready ──the poll finds GitHub moved past it─► reviewed (+ `filed`, `settledAt`) ready ──Send / `cerber send` / auto-send────► sent ``` +And the one way back out of settled without you pressing anything: + +``` +reviewed | skipped ──asked for again since `settledAt`──► ready | awaiting +``` + Settling is the one that is not restricted to a finished draft: the cockpit offers skip on `awaiting`, `running` and `failed` rows too, so a PR you have decided about needs no draft first. @@ -150,7 +162,10 @@ a token it checks the artifact already on disk: 1. `force`? → run. (The cockpit's **re-review** button always forces; `cerber review --force` too.) 2. Status in `SETTLED_BY_YOU` (`reviewed`, `skipped`)? → **skip**, log - "use --force". *This is why marking a PR reviewed survives a push.* + "use --force". *This is why marking a PR reviewed survives a push.* The poll + can still take such a row out of settled first — see "Asked again" in §5 — + but that is a decision about the queue, made before this guard is reached, + and never about spending a run on a row that is still settled. 3. Status in `HEAD_SENSITIVE` (`ready`, `sent`) **and** the head the last run *read* is still the PR's head? → **skip** as up to date. (Which sha that is, and why it is not `pr.headSha`, is the paragraph below.) @@ -253,6 +268,10 @@ walk `walkable()`: open, unsettled rows only. | GitHub moved past a finished draft | status → `reviewed`, `filed` set | `fileIfSettledElsewhere` | | Auto-send is on and the draft qualified | status → `sent` (the **sent** tab) | `handleAutoSend` | +And the one that *adds* a row back: your review requested again after you +settled → status → `ready`/`awaiting`, `settledAt` and `filed` cleared +(`reopenIfAskedAgain`; see "Asked again" below). + A **pure stub** is `awaiting`, with no comments and **`run === null`** — it does not look at chapters. The `run` block is what makes this safe: the create endpoint persists one before it answers, so a review you pulled in by hand is @@ -280,6 +299,37 @@ Someone *answering* your comment files nothing — that reply is addressed to you. State checks are leashed to one per artifact per 30 minutes and capped per poll. +### Asked again: the way back out of settled + +Filing's mirror image, and the only thing that reopens a settled row on its own +(`reopenIfAskedAgain`, `src/server/daemon.ts`). A skip says "I am done with +this PR", and a push does not undo that. Somebody asking you *again* is a +different event: your skip answered the request that was open when you made it +and cannot have answered one that came later — which is what a request +withdrawn and then re-added is. Nothing else in GitHub's API distinguishes the +two, so this reads the one thing that does: the timestamp of the last +`REVIEW_REQUESTED_EVENT` naming you (`fetchLastReviewRequest`, one GraphQL call). + +Conditions, all of them: the row is `reviewed` or `skipped`, it was never sent, +GitHub is asking about it *now* (only rows in the awaiting search are checked +at all), and the request is newer than `settledAt`. Then the row goes back to +`ready` — or `awaiting` if it has no finished draft to show — `settledAt` and +`filed` are cleared, and the ordinary rules take it from there: the freshness +guard in §4 re-drafts it if the head has moved since the run read it, and +leaves the existing draft alone if it has not. Same leash as the state checks +above: one call per artifact per 30 minutes, capped per poll. + +Team requests are ignored here on purpose. `stillRequested` counts them because +refusing to *file work away* is the safe side of that question; this decides to +undo a decision of yours, where the safe side is doing nothing unless somebody +named you. + +Rows settled before `settledAt` existed are dated by the latest moment cerber +can prove the decision came after — cerber's own `filed.at`, or `run.finishedAt` +(you cannot have skipped a draft before it existed). Both are lower bounds, so +the cost of being wrong is one row coming back once; the alternative is that +every row settled before this shipped stays unreachable forever. + ### "Whose move is it" Independent of status. Each poll reads the PR conversation per awaiting PR @@ -381,7 +431,9 @@ freshness guard skipped is not re-judged or re-logged on later polls. (the **sent** tab — `sent` is in `SETTLED` too), it's settled (`reviewed` / `skipped` — the **settled** tab, and check **open requests**), the PR is merged/closed (**archived**), the poll is off, or GitHub isn't requesting your -review. +review. A settled row that GitHub *has* asked you about again since you settled +it is meant to come back on its own — if it hasn't, the poll is off, or the +request is older than your decision. **"Why did this PR come back?"** — The poll re-drafted it, which needs polling and auto-review both on and the PR still in the awaiting search. Given that: @@ -389,8 +441,10 @@ its status was `awaiting` or `failed`, which are retried with no push involved at all; or the author pushed and it was `ready` or `sent`, both head-sensitive — though a `sent` row also needs someone to have asked you again. Whether you have opened the draft since the push makes no difference: the guard compares -against the head the run *read*, not the one the artifact mentions. Only -`reviewed` and `skipped` never come back under any of it. +against the head the run *read*, not the one the artifact mentions. A +`reviewed` or `skipped` row comes back for one reason only, and it is not a +push: somebody requested your review *again* after you settled it — see "Asked +again" in §5. **"Why won't it re-review?"** — Both causes are the freshness guard in §4, so they only bind the callers that obey it (the poll, plain `cerber review`): diff --git a/src/core/artifact.ts b/src/core/artifact.ts index fb36d4f..0cfce6c 100644 --- a/src/core/artifact.ts +++ b/src/core/artifact.ts @@ -316,6 +316,18 @@ export const ArtifactSchema = z.object({ sent: SentInfoSchema.nullable().default(null), /** Set when cerber filed this draft away itself, and why. Never on a sent one. */ filed: FiledInfoSchema.nullable().default(null), + /** + * When this review was settled — marked `reviewed` or `skipped`, by you or by + * cerber filing it. Null on anything not settled. + * + * A settle answers the request that was open at the time. It cannot answer + * one that came afterwards, so the poll needs to know which side of it a + * review request falls on before deciding a settled row stays settled + * (`askedAgainAfterSettling` in `src/server/daemon.ts`). `updatedAt` cannot + * stand in for this: opening a settled review refreshes it, which moves that + * field forward long after the decision it is meant to date. + */ + settledAt: z.string().nullable().default(null), /** Last time this review was pulled forward onto a newer head commit. */ refresh: RefreshInfoSchema.nullable().default(null), calibration: CalibrationSchema.nullable().default(null), diff --git a/src/core/autosend.test.ts b/src/core/autosend.test.ts index 93e564d..af092f6 100644 --- a/src/core/autosend.test.ts +++ b/src/core/autosend.test.ts @@ -36,6 +36,7 @@ function makeArtifact(overrides: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/core/gh.test.ts b/src/core/gh.test.ts index 458ae1c..b344dbe 100644 --- a/src/core/gh.test.ts +++ b/src/core/gh.test.ts @@ -1,6 +1,6 @@ import { execFile } from "node:child_process"; import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; -import { classifyReply, currentLogin, latestOwnReview, resetLoginCache } from "./gh.js"; +import { classifyReply, currentLogin, lastRequestOf, latestOwnReview, resetLoginCache } from "./gh.js"; // gh.ts calls `promisify(execFile)`, which honours this symbol — so the mock // resolves to the `{ stdout }` shape the real one does, while still recording @@ -148,6 +148,41 @@ describe("latestOwnReview", () => { }); }); +describe("lastRequestOf", () => { + const asked = (login: string, createdAt: string, typename = "User") => ({ + createdAt, + requestedReviewer: { __typename: typename, login }, + }); + + it("finds nothing when nobody has asked you", () => { + expect(lastRequestOf([], "me")).toBeNull(); + expect(lastRequestOf([asked("them", "2026-08-20T10:00:00Z")], "me")).toBeNull(); + }); + + // The whole point: a request that was withdrawn and made again is a second + // ask, and only its timestamp can tell it from the first. + it("takes the most recent ask, not the first", () => { + expect( + lastRequestOf( + [ + asked("me", "2026-08-21T10:43:27Z"), + asked("them", "2026-08-24T11:00:00Z"), + asked("me", "2026-08-24T12:41:22Z"), + ], + "me", + ), + ).toBe("2026-08-24T12:41:22Z"); + }); + + // A team request names the team, never you. This answer decides whether to + // undo a decision of yours, so it acts only on somebody naming you. + it("ignores requests that did not name you", () => { + expect(lastRequestOf([{ createdAt: "2026-08-24T12:41:22Z", requestedReviewer: { __typename: "Team" } }], "me")).toBeNull(); + expect(lastRequestOf([asked("me", "2026-08-24T12:41:22Z", "Bot")], "me")).toBeNull(); + expect(lastRequestOf([{ createdAt: "2026-08-24T12:41:22Z", requestedReviewer: null }], "me")).toBeNull(); + }); +}); + describe("parsePrRef", () => { it("parses a full PR URL", () => { expect(parsePrRef("https://github.com/fullstackhouse/skills/pull/22")).toEqual({ diff --git a/src/core/gh.ts b/src/core/gh.ts index 29fe456..d990b71 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -356,6 +356,67 @@ export function stillRequested(requests: ReviewRequests, login: string): boolean return requests.teams.length > 0 || requests.users.includes(login); } +/** + * When someone last asked *you*, by name, for a review — and nothing else. + * + * `fetchReviewRequests` answers whether a request is open; this answers when it + * was made, which is the only way to tell a request you already dealt with from + * a second one asking again. A withdrawn-then-re-added request looks identical + * to the original in every other read GitHub offers. + * + * Team requests are deliberately ignored. `stillRequested` counts them because + * refusing to file work away is the safe side of that question; this one decides + * to *undo* a decision of yours, where the safe side is doing nothing unless + * somebody named you. + */ +export interface RequestedReviewEvent { + createdAt: string; + requestedReviewer: { __typename?: string; login?: string } | null; +} + +export function lastRequestOf(events: RequestedReviewEvent[], login: string): string | null { + const mine = events.filter((e) => e.requestedReviewer?.__typename === "User" && e.requestedReviewer.login === login); + if (mine.length === 0) return null; + return mine.reduce((a, b) => (b.createdAt > a.createdAt ? b : a)).createdAt; +} + +const LAST_REQUEST_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$number){ + timelineItems(last:20,itemTypes:[REVIEW_REQUESTED_EVENT]){ + nodes{... on ReviewRequestedEvent{createdAt requestedReviewer{__typename ... on User{login}}}} + } + } + } +}`; + +/** + * Ask GitHub when your review was last requested on a PR. + * + * GraphQL rather than the REST timeline on purpose: this runs on rows the queue + * already holds, poll after poll, and the timeline of a busy PR is hundreds of + * events across several pages. `timelineItems(last:20, itemTypes:[…])` is one + * call whatever the PR's history looks like. + */ +export async function fetchLastReviewRequest(ref: PrRef, login: string): Promise { + const out = await gh([ + "api", + "graphql", + "-f", + `query=${LAST_REQUEST_QUERY}`, + "-F", + `owner=${ref.owner}`, + "-F", + `repo=${ref.repo}`, + "-F", + `number=${ref.number}`, + ]); + const raw = JSON.parse(out) as { + data?: { repository?: { pullRequest?: { timelineItems?: { nodes?: RequestedReviewEvent[] } } } }; + }; + return lastRequestOf(raw.data?.repository?.pullRequest?.timelineItems?.nodes ?? [], login); +} + export function searchAwaitingArgs(repoFilter?: string, limit = 50): string[] { const args = [ "search", diff --git a/src/core/refresh.test.ts b/src/core/refresh.test.ts index 27b942a..ed428db 100644 --- a/src/core/refresh.test.ts +++ b/src/core/refresh.test.ts @@ -70,6 +70,7 @@ function makeArtifact(overrides: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/core/refresh.ts b/src/core/refresh.ts index 2dce76a..4f1e1eb 100644 --- a/src/core/refresh.ts +++ b/src/core/refresh.ts @@ -86,6 +86,10 @@ export function mergeRunResult(fresh: Artifact, current: Artifact): Artifact { return { ...fresh, status: userOwnsStatus(current) ? current.status : fresh.status, + // Travels with the status it dates. A settle that landed while the run + // worked keeps both halves of itself, and a run that reopens the row takes + // the stamp away with the status. + settledAt: userOwnsStatus(current) ? current.settledAt : fresh.settledAt, sent: current.sent, calibration: current.calibration, filed: current.filed, diff --git a/src/core/revise.test.ts b/src/core/revise.test.ts index eff561b..b1febc4 100644 --- a/src/core/revise.test.ts +++ b/src/core/revise.test.ts @@ -52,6 +52,7 @@ function artifact(over: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/core/send.test.ts b/src/core/send.test.ts index b35958f..b562233 100644 --- a/src/core/send.test.ts +++ b/src/core/send.test.ts @@ -48,6 +48,7 @@ function makeArtifact(overrides: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/core/state.test.ts b/src/core/state.test.ts index 62cc30a..12741f3 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -42,6 +42,7 @@ function artifact(over: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/runner/chat.test.ts b/src/runner/chat.test.ts index dbffaf1..4c852fe 100644 --- a/src/runner/chat.test.ts +++ b/src/runner/chat.test.ts @@ -69,6 +69,7 @@ function artifact(over: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/runner/review-merge.test.ts b/src/runner/review-merge.test.ts index 1ed2b1b..9b86179 100644 --- a/src/runner/review-merge.test.ts +++ b/src/runner/review-merge.test.ts @@ -85,6 +85,7 @@ function ready(comments: Comment[], headSha = "old-sha"): Artifact { run: null, sent: null, filed: null, + settledAt: null, refresh: null, calibration: null, chat: [], @@ -180,6 +181,31 @@ describe("what a re-review does to a decision you made while it ran", () => { } }); + it("keeps the settle whole — the stamp with the status", async () => { + // The stamp is what the poll compares a review request against. Half a + // settle would leave the row settled with nothing to date the decision by, + // and a re-request would then read as older than a skip that came after it. + await saveArtifact(ready([])); + const settledAt = "2026-08-21T10:02:00.000Z"; + claudeThat(async () => { + await updateArtifactByKey(KEY, (a) => ({ ...a, status: "skipped" as const, settledAt })); + }); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.status).toBe("skipped"); + expect(artifact.settledAt).toBe(settledAt); + }); + + // A forced re-review is you taking the row back, so the settle goes entirely. + it("clears the stamp when it does reopen a row", async () => { + await saveArtifact({ ...ready([]), status: "skipped" as const, settledAt: "2026-08-20T09:00:00Z" }); + claudeThat(async () => {}); + + const { artifact } = await reviewPr(REF, { withSource: false, force: true }); + expect(artifact.status).toBe("ready"); + expect(artifact.settledAt).toBeNull(); + }); + it("does not reopen one you settled when the run fails either", async () => { // A failure is still just a fact about the code; it does not overrule a // decision about the PR any more than a success does. diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index f274eb3..47c9d30 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -58,6 +58,7 @@ function artifact(status: ArtifactStatus, headSha: string): Artifact { run: null, sent: null, filed: null, + settledAt: null, refresh: null, calibration: null, chat: [], diff --git a/src/runner/review.ts b/src/runner/review.ts index afceb0d..d227484 100644 --- a/src/runner/review.ts +++ b/src/runner/review.ts @@ -70,7 +70,7 @@ const HEAD_SENSITIVE = new Set(["ready", "sent"]); * as cerber overruling you — and on a busy PR it happens every few minutes. * The way back in is the way you got out: press re-review, which forces. */ -const SETTLED_BY_YOU = new Set(["reviewed", "skipped"]); +export const SETTLED_BY_YOU = new Set(["reviewed", "skipped"]); /** * Fetch a PR, run the AI review, persist the artifact at each stage. @@ -213,6 +213,7 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, // The conversation is the user's writing, so a re-review keeps it — the // chat prompt replays the transcript, which is what makes it survive the diff --git a/src/server/chat.test.ts b/src/server/chat.test.ts index 61b6a28..8452e85 100644 --- a/src/server/chat.test.ts +++ b/src/server/chat.test.ts @@ -53,6 +53,7 @@ function artifact(over: Partial = {}): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, diff --git a/src/server/daemon.test.ts b/src/server/daemon.test.ts index 807da4a..fb01e82 100644 --- a/src/server/daemon.test.ts +++ b/src/server/daemon.test.ts @@ -7,6 +7,7 @@ import { loadConfig } from "../core/config.js"; import { currentLogin, fetchConversation, + fetchLastReviewRequest, fetchOwnReview, fetchPrInfo, fetchReviewRequests, @@ -17,9 +18,12 @@ import { notify } from "../core/notify.js"; import { loadArtifact, saveArtifact } from "../core/state.js"; import { DaemonHandle, + askedAgainAfterSettling, filedByWithdrawnRequest, filedByYourAct, isPureStub, + reopenedStatus, + settledAtOf, startDaemon, stubArtifact, } from "./daemon.js"; @@ -32,6 +36,7 @@ vi.mock("../core/gh.js", async (orig) => ({ searchAwaitingMe: vi.fn(), fetchPrInfo: vi.fn(), fetchConversation: vi.fn(), + fetchLastReviewRequest: vi.fn(), fetchOwnReview: vi.fn(), fetchReviewRequests: vi.fn(), currentLogin: vi.fn(), @@ -52,6 +57,7 @@ const prInfo = fetchPrInfo as Mock; const config = loadConfig as Mock; const conversation = fetchConversation as Mock; const ownReview = fetchOwnReview as Mock; +const lastRequest = fetchLastReviewRequest as Mock; const requests = fetchReviewRequests as Mock; const login = currentLogin as Mock; const notified = notify as Mock; @@ -819,3 +825,228 @@ describe("the tap on the machine when a PR lands", () => { expect((await pollTimes(1)).notify).toBe(false); }); }); + +describe("a review you settled, and were asked for again", () => { + const daemonKnobs = { + poll: true, + autoReview: true, + notify: true, + intervalMinutes: 5, + parallel: 1, + repos: [], + }; + // autoReview off: what's under test is the queue's bookkeeping, not the runner. + const options = { + repos: [], + intervalMs: 10, + parallel: 1, + autoReview: false, + notify: false, + autoSend: "shadow" as const, + autoSendThreshold: 90, + }; + + const RUN = { + model: "claude", + startedAt: "2026-08-24T09:10:00Z", + finishedAt: "2026-08-24T09:17:00Z", + costUsd: 6.8, + error: null, + withSource: true, + trusted: true, + sessionId: null, + trigger: "daemon" as const, + reviewedSha: "abc1234", + }; + + /** A draft you skipped at a known moment — the row this whole change is about. */ + const settled = (over: Record = {}) => ({ + ...stubArtifact(DISCOVERED), + status: "skipped" as const, + run: RUN, + settledAt: "2026-08-24T10:00:00Z", + ...over, + }); + + beforeEach(() => { + vi.clearAllMocks(); + process.env.CERBER_HOME = mkdtempSync(path.join(os.tmpdir(), "cerber-daemon-reopen-")); + config.mockResolvedValue({ trust: [], daemon: daemonKnobs }); + login.mockResolvedValue("me"); + conversation.mockResolvedValue([]); + lastRequest.mockResolvedValue(null); + // GitHub is asking for this one right now — every case here starts there. + search.mockResolvedValue([DISCOVERED]); + }); + + async function pollOnce() { + const handle = startDaemon(options); + try { + await vi.waitFor(() => expect(handle.status().polls).toBeGreaterThanOrEqual(1)); + } finally { + await stopAndDrain(handle); + } + return loadArtifact("acme/widgets#7"); + } + + it("puts the row back in the inbox when the ask came after your skip", async () => { + await saveArtifact(settled()); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + + const after = await pollOnce(); + expect(after?.status).toBe("ready"); + // The stamp goes with the decision it dated: this row is not settled now. + expect(after?.settledAt).toBeNull(); + // The draft itself is untouched — it is the same review, back on your desk. + expect(after?.run?.reviewedSha).toBe("abc1234"); + }); + + it("leaves your skip standing when the ask is the one you already answered", async () => { + await saveArtifact(settled()); + lastRequest.mockResolvedValue("2026-08-21T10:43:27Z"); + + const after = await pollOnce(); + expect(after?.status).toBe("skipped"); + expect(after?.settledAt).toBe("2026-08-24T10:00:00Z"); + }); + + // The request left open by a skip is the ordinary case — a row you dealt with + // that GitHub never stopped asking about. Nothing to undo, nothing to say. + it("says nothing about a request that predates nothing", async () => { + await saveArtifact(settled()); + lastRequest.mockResolvedValue(null); + + expect((await pollOnce())?.status).toBe("skipped"); + }); + + it("drops cerber's own account of why the row was settled", async () => { + // Filed by the poll, not by you: the note explains a status that is going. + await saveArtifact( + settled({ + status: "reviewed" as const, + settledAt: "2026-08-24T10:00:00Z", + filed: { at: "2026-08-24T10:00:00Z", reason: "request-withdrawn" as const, review: null, reply: null }, + }), + ); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + + const after = await pollOnce(); + expect(after?.status).toBe("ready"); + expect(after?.filed).toBeNull(); + }); + + it("reopens a row with no draft as awaiting one", async () => { + await saveArtifact(settled({ run: null })); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + + expect((await pollOnce())?.status).toBe("awaiting"); + }); + + it("never reopens a review that was sent", async () => { + await saveArtifact( + settled({ + status: "sent" as const, + sent: { at: "2026-08-24T10:00:00Z", event: "COMMENT" as const, url: null, auto: false }, + }), + ); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + + const after = await pollOnce(); + expect(after?.status).toBe("sent"); + expect(lastRequest).not.toHaveBeenCalled(); + }); + + // One GitHub call per poll per settled row is the whole cost of this; a row + // that could never be reopened must not cost even that. + it("asks GitHub nothing about a row that isn't settled", async () => { + await saveArtifact({ ...stubArtifact(DISCOVERED), status: "ready" as const, run: RUN }); + + expect((await pollOnce())?.status).toBe("ready"); + expect(lastRequest).not.toHaveBeenCalled(); + }); + + it("leaves the row alone when GitHub cannot be reached", async () => { + await saveArtifact(settled()); + lastRequest.mockRejectedValue(new Error("gh api graphql failed: offline")); + + expect((await pollOnce())?.status).toBe("skipped"); + }); +}); + +describe("askedAgainAfterSettling", () => { + const RUN = { + model: null, + startedAt: "2026-08-24T09:10:00Z", + finishedAt: "2026-08-24T09:17:00Z", + costUsd: null, + error: null, + withSource: false, + trusted: false, + sessionId: null, + trigger: "daemon" as const, + reviewedSha: null, + }; + const skipped = { + ...stubArtifact(DISCOVERED), + status: "skipped" as const, + run: RUN, + settledAt: "2026-08-24T10:00:00Z", + }; + + it("only counts an ask that came after the decision", () => { + expect(askedAgainAfterSettling(skipped, "2026-08-24T12:41:22Z")).toBe(true); + expect(askedAgainAfterSettling(skipped, "2026-08-24T09:00:00Z")).toBe(false); + expect(askedAgainAfterSettling(skipped, null)).toBe(false); + }); + + // Same second on both sides: our stamps carry milliseconds and GitHub's do + // not, and "…:00.500Z" sorts before "…:00Z" as text. + it("compares moments, not strings", () => { + const settledAt = "2026-08-24T10:00:26.500Z"; + expect(askedAgainAfterSettling({ ...skipped, settledAt }, "2026-08-24T10:00:26Z")).toBe(false); + expect(askedAgainAfterSettling({ ...skipped, settledAt }, "2026-08-24T10:00:27Z")).toBe(true); + }); + + it("has nothing to undo on a row you never settled", () => { + expect(askedAgainAfterSettling({ ...skipped, status: "ready" }, "2026-08-24T12:41:22Z")).toBe(false); + expect(askedAgainAfterSettling({ ...skipped, status: "running" }, "2026-08-24T12:41:22Z")).toBe(false); + }); + + it("never speaks for a review that was sent", () => { + const sent = { + ...skipped, + status: "reviewed" as const, + sent: { at: "2026-08-24T10:00:00Z", event: "COMMENT" as const, url: null, auto: false }, + }; + expect(askedAgainAfterSettling(sent, "2026-08-24T12:41:22Z")).toBe(false); + }); + + // Rows settled before `settledAt` existed still have to be reachable, or the + // bug this fixes is permanent for every one of them. + it("dates a legacy row by the latest moment it can prove", () => { + const legacy = { ...skipped, settledAt: null }; + // You cannot have skipped a draft before the draft existed. + expect(settledAtOf(legacy)).toBe(RUN.finishedAt); + expect(askedAgainAfterSettling(legacy, "2026-08-24T12:41:22Z")).toBe(true); + expect(askedAgainAfterSettling(legacy, "2026-08-24T09:15:00Z")).toBe(false); + + // Cerber filing it is a later, better-known moment than the run finishing. + const filed = { + ...legacy, + status: "reviewed" as const, + filed: { at: "2026-08-24T13:00:00Z", reason: "own-review" as const, review: null, reply: null }, + }; + expect(settledAtOf(filed)).toBe("2026-08-24T13:00:00Z"); + expect(askedAgainAfterSettling(filed, "2026-08-24T12:41:22Z")).toBe(false); + + // Nothing to go on at all: a stub you skipped before it was ever drafted. + expect(settledAtOf({ ...legacy, run: null })).toBeNull(); + }); + + it("sends a row back to the draft it has, or to waiting for one", () => { + expect(reopenedStatus(skipped)).toBe("ready"); + expect(reopenedStatus({ ...skipped, run: null })).toBe("awaiting"); + expect(reopenedStatus({ ...skipped, run: { ...RUN, error: "claude exited 1" } })).toBe("awaiting"); + expect(reopenedStatus({ ...skipped, run: { ...RUN, finishedAt: null } })).toBe("awaiting"); + }); +}); diff --git a/src/server/daemon.ts b/src/server/daemon.ts index 9293b49..2041dec 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -7,6 +7,7 @@ import { classifyReply, currentLogin, fetchConversation, + fetchLastReviewRequest, fetchOwnReview, fetchPrInfo, fetchReviewRequests, @@ -26,7 +27,7 @@ import { updateArtifactByKey, } from "../core/state.js"; import { ReviewInProgressError } from "../runner/inflight.js"; -import { pool, reviewPr } from "../runner/review.js"; +import { SETTLED_BY_YOU, pool, reviewPr } from "../runner/review.js"; export interface DaemonOptions { /** Repos to watch (owner/repo). Empty = everything your gh account can see. */ @@ -151,6 +152,7 @@ export function stubArtifact(ref: DiscoveredPr): Artifact { sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, @@ -230,7 +232,59 @@ export function filedByWithdrawnRequest(artifact: Artifact): boolean { return artifact.run?.trigger === "daemon"; } -/** How many open-PR artifacts to re-check against GitHub per poll, and how often each. */ +/** + * When a settled row was settled, as well as cerber can date it — or null if it + * isn't settled at all, or a send already spoke for it. + * + * `settledAt` is the record and the only exact answer. Rows settled before that + * field existed fall back to the latest moment cerber can prove the decision + * came after: cerber filed it (`filed.at`), or you settled a draft, which you + * cannot have done before the draft existed (`run.finishedAt`). Both are lower + * bounds, so a stale one can only make this read a request as newer than it + * really was — one row coming back into the inbox once, against leaving every + * pre-existing settled row permanently unreachable, which is the bug. + * + * Not `updatedAt`: opening a settled review refreshes it onto the new head, + * which moves that field forward long after the decision it would be dating. + */ +export function settledAtOf(a: Artifact): string | null { + if (!SETTLED_BY_YOU.has(a.status) || a.sent) return null; + return a.settledAt ?? a.filed?.at ?? a.run?.finishedAt ?? null; +} + +/** + * Whether a review request is a *new* ask rather than the one you settled. + * + * A skip says "I am done with this PR", and a push does not undo that — the + * whole point of `SETTLED_BY_YOU`. Somebody asking you again is a different + * event: your skip answered the request that was open when you made it, and it + * cannot have answered one that came after. Withdrawn-then-re-added is the + * shape this catches, and it is not exotic — an author who pulls the request + * while you are mid-look and puts it back with the next push leaves the inbox + * silent about a review someone is now actively waiting on. + * + * Parsed, not compared as text, for the reason `filedByYourAct` gives: our + * timestamps carry milliseconds and GitHub's do not, and "…:00.500Z" sorts + * before "…:00Z". + */ +export function askedAgainAfterSettling(a: Artifact, requestedAt: string | null): boolean { + if (requestedAt === null) return false; + const settled = settledAtOf(a); + return settled !== null && Date.parse(requestedAt) > Date.parse(settled); +} + +/** + * What a reopened row goes back to being: the draft it has, or a row waiting + * for one. A run that failed left no draft to show, so it reopens as `awaiting` + * and the poll drafts it again. + */ +export function reopenedStatus(a: Artifact): "ready" | "awaiting" { + return a.run?.finishedAt && !a.run.error ? "ready" : "awaiting"; +} + +/** How many artifacts each of the poll's GitHub re-checks — the open-PR state + * refresh, and the settled rows it asks about again — may spend per poll, and + * how often any one artifact is re-asked about. */ const STATE_REFRESH_CAP = 10; const STATE_REFRESH_MIN_MS = 30 * 60_000; @@ -272,6 +326,9 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { /** When GitHub last confirmed an artifact's PR state, by artifact id. In-memory: * a restart just re-checks sooner, which is harmless. */ const stateCheckedAt = new Map(); + /** When GitHub was last asked whether a settled row has been re-requested, by + * artifact id. In-memory for the same reason: a restart just asks sooner. */ + const requestCheckedAt = new Map(); async function discover(): Promise { const filters = opts.repos.length > 0 ? opts.repos : [undefined]; @@ -294,9 +351,65 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { */ async function syncQueue(refs: DiscoveredPr[]): Promise<{ discovered: DiscoveredPr[] }> { const discovered: DiscoveredPr[] = []; + /** Reopen checks spent this poll — the same cap the state refresh below obeys. */ + let reopenChecks = 0; + + // Asked once per pass, and only if a row actually reaches a check that + // needs it. Undefined is "not asked yet"; null is "gh could not say", which + // attributes nothing to you rather than guessing. + let login: string | null | undefined; + + /** + * Put a settled row back in the inbox when somebody has asked you again. + * + * The mirror of `fileIfSettledElsewhere`: that one files rows away once + * GitHub has moved past them, this one brings one back when GitHub has + * moved *to* it. Both exist because the queue's picture of who is waiting + * on what is local, and the thing it is a picture of lives on github.com. + * + * Only rows GitHub is asking about right now get here at all — the caller + * is looping over the awaiting search — so the extra read is one call for + * one question: was that request made after you settled? + */ + async function reopenIfAskedAgain(artifact: Artifact): Promise { + if (settledAtOf(artifact) === null) return; + const last = requestCheckedAt.get(artifact.id) ?? 0; + if (reopenChecks >= STATE_REFRESH_CAP || Date.now() - last < STATE_REFRESH_MIN_MS) return; + reopenChecks++; + requestCheckedAt.set(artifact.id, Date.now()); + if (login === undefined) login = await currentLogin().catch(() => null); + if (login === null) return; + + let requestedAt: string | null; + try { + requestedAt = await fetchLastReviewRequest(artifact.pr, login); + } catch { + // A read that failed is not evidence of anything — next poll retries. + return; + } + if (requestedAt === null || !askedAgainAfterSettling(artifact, requestedAt)) return; + const at = requestedAt; + + // Re-read from disk: a re-review or a send may have started in the time + // that call took, and neither wants its status rewritten underneath it. + const saved = await updateArtifactByKey(artifactKey(artifact.id), (a) => + askedAgainAfterSettling(a, at) + ? // `filed` goes with the status it explained: cerber's account of why + // this row was settled is not the story of a row that is back. + { ...a, status: reopenedStatus(a), settledAt: null, filed: null } + : a, + ); + // The status is what says the write landed. `settledAt` cannot: a legacy + // row never had one, so a mutation that declined would read as a reopen. + if (saved && !SETTLED_BY_YOU.has(saved.status)) { + log(`[${artifact.id}] your review was requested again on ${at.slice(0, 10)} — back in the inbox`); + } + } + for (const ref of refs) { const existing = await loadArtifact(artifactId(ref)); if (existing) { + await reopenIfAskedAgain(existing); // The search result is what the PR is right now; the artifact can // predate a draft→ready flip. Refresh from it — it costs nothing, it's // already in hand — or the queue keeps calling a ready PR a draft until @@ -313,11 +426,6 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { discovered.push(ref); } - // Asked once per pass, and only if a row actually reaches the check that - // needs it. Undefined is "not asked yet"; null is "gh could not say", which - // attributes nothing to you rather than guessing. - let login: string | null | undefined; - /** * File a draft away once GitHub has moved past it without cerber's help. * @@ -352,7 +460,7 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { * these reads took, and neither wants filing out from under it. */ async function file(filed: FiledInfo, note: string, stillFilable: (a: Artifact) => boolean) { const saved = await updateArtifactByKey(artifactKey(artifact.id), (a) => - stillFilable(a) ? { ...a, status: "reviewed" as const, filed } : a, + stillFilable(a) ? { ...a, status: "reviewed" as const, filed, settledAt: filed.at } : a, ); if (saved?.filed) log(`[${artifact.id}] ${note} — filed as reviewed`); } diff --git a/src/server/guards.test.ts b/src/server/guards.test.ts index 416cec4..1b4b28f 100644 --- a/src/server/guards.test.ts +++ b/src/server/guards.test.ts @@ -61,6 +61,7 @@ function artifact(status: ArtifactStatus): Artifact { run: null, sent: null, filed: null, + settledAt: null, refresh: null, calibration: null, chat: [], @@ -146,6 +147,21 @@ describe("PATCH /api/reviews/:key — only the statuses that are your decision", } }); + // The poll asks which side of your decision a review request falls on, so the + // decision has to be dated as it is made. `updatedAt` cannot answer it: just + // opening a settled review refreshes it and moves that field forward. + it("dates the decision as it makes it", async () => { + await saveArtifact(artifact("ready")); + expect((await loadArtifact(ID))!.settledAt).toBeNull(); + + const before = Date.now(); + expect((await patchStatus("skipped")).status).toBe(200); + + const at = (await loadArtifact(ID))!.settledAt; + expect(at).not.toBeNull(); + expect(Date.parse(at!)).toBeGreaterThanOrEqual(before); + }); + it("refuses to call a review sent when nothing was sent", async () => { // The row this used to make claims a review reached GitHub while `sent` is // still null — a state no honest path produces, which the queue and every diff --git a/src/server/index.ts b/src/server/index.ts index e3a63cc..519b5d1 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -315,6 +315,7 @@ export async function buildApp( sent: null, refresh: null, filed: null, + settledAt: null, calibration: null, chat: [], preChat: null, @@ -365,6 +366,10 @@ export async function buildApp( const next = { ...a }; if (body.status !== undefined) { next.status = ArtifactStatusSchema.parse(body.status); + // Dated on the way in, because nothing else can date it later: the poll + // asks whether a review request came before or after this decision, and + // `updatedAt` moves every time the review is opened and refreshed. + next.settledAt = new Date().toISOString(); } if (body.verdictRecommendation !== undefined && next.verdict) { next.verdict = { From 52d61ed12b42699b61b1876f4e2cbf99453c13d0 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:36:33 +0200 Subject: [PATCH 2/4] fix(inbox): a settle of your own clears cerber's filing note, and two doc fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #43. `filed` outranks the status wherever the queue tags a row (`rowTag`, `requestTag`), because "reviewed" on a row cerber filed would read as a click nobody made. That stops being true the moment you click: a row you marked yourself was settled by you, whatever cerber had concluded about it earlier, so the settle now clears the note — the same rule the poll's reopen already follows. Also: say plainly what a new request can do to a settled row rather than trailing off mid-sentence, and stop the failed-read comment claiming the next poll retries. It does not — the leash is spent before the call on purpose, so gh being down cannot turn every settled row into a retry on every poll. Co-Authored-By: Claude Opus 5 (1M context) --- docs/lifecycle.md | 4 ++-- src/server/daemon.ts | 5 ++++- src/server/guards.test.ts | 16 ++++++++++++++++ src/server/index.ts | 6 ++++++ 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 0b64061..aa3f4ab 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -55,8 +55,8 @@ Two derived groupings drive most behaviour: - **`SETTLED = [sent, reviewed, skipped]`** (`web/src/inbox.ts`) — out of the live queue. - **`SETTLED_BY_YOU = [reviewed, skipped]`** (`src/runner/review.ts`) — a new - push must not drag these back. A new *request* is the one thing that can - (§5, "Asked again"). + push must not drag these back. The one thing that can is somebody requesting + your review *again* after you settled it — see §5, "Asked again". Settling one of those stamps `settledAt`. That field exists for exactly one question — did this review request come before or after your decision — and diff --git a/src/server/daemon.ts b/src/server/daemon.ts index 2041dec..a257d44 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -384,7 +384,10 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { try { requestedAt = await fetchLastReviewRequest(artifact.pr, login); } catch { - // A read that failed is not evidence of anything — next poll retries. + // A read that failed is not evidence of anything, so nothing happens to + // the row. It is not re-asked about until the leash above expires: the + // stamp is spent before the call on purpose, so gh being down cannot + // turn every settled row into a retry on every poll. return; } if (requestedAt === null || !askedAgainAfterSettling(artifact, requestedAt)) return; diff --git a/src/server/guards.test.ts b/src/server/guards.test.ts index 1b4b28f..8c05bc9 100644 --- a/src/server/guards.test.ts +++ b/src/server/guards.test.ts @@ -162,6 +162,22 @@ describe("PATCH /api/reviews/:key — only the statuses that are your decision", expect(Date.parse(at!)).toBeGreaterThanOrEqual(before); }); + // `filed` outranks the status wherever the queue tags a row, on the grounds + // that "reviewed" would read as a click nobody made. Clicking makes it a + // click — so cerber's account of the row goes with the status it explained. + it("drops cerber's filing note when you settle the row yourself", async () => { + await saveArtifact({ + ...artifact("reviewed"), + filed: { at: "2026-08-21T09:00:00.000Z", reason: "own-review" as const, review: null, reply: null }, + }); + + expect((await patchStatus("skipped")).status).toBe(200); + + const after = (await loadArtifact(ID))!; + expect(after.status).toBe("skipped"); + expect(after.filed).toBeNull(); + }); + it("refuses to call a review sent when nothing was sent", async () => { // The row this used to make claims a review reached GitHub while `sent` is // still null — a state no honest path produces, which the queue and every diff --git a/src/server/index.ts b/src/server/index.ts index 519b5d1..636d2b9 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -370,6 +370,12 @@ export async function buildApp( // asks whether a review request came before or after this decision, and // `updatedAt` moves every time the review is opened and refreshed. next.settledAt = new Date().toISOString(); + // And cerber's own account of why this row was settled goes with the + // status it explained. `filed` outranks the status wherever the queue + // tags a row (`rowTag`, `requestTag`), on the grounds that "reviewed" + // would otherwise read as a click nobody made — which stops being true + // the moment you click. Same rule the poll's reopen follows. + next.filed = null; } if (body.verdictRecommendation !== undefined && next.verdict) { next.verdict = { From 612e7d30ec3b49bba6afffc35730aa0958a768c6 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:42:01 +0200 Subject: [PATCH 3/4] fix(inbox): widen the review-request window so a busy PR can still reopen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #43. The query asked for the last 20 `REVIEW_REQUESTED_EVENT`s, which is not the same as "every one of them": a PR that cycled through a dozen reviewers can push the request that named *you* off the end, and the answer would come back "nobody asked" — silently restoring the stuck-in-settled behaviour this change exists to remove. 100 is the page maximum and costs exactly what 20 did: one call. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/gh.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/core/gh.ts b/src/core/gh.ts index d990b71..000d0eb 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -380,10 +380,15 @@ export function lastRequestOf(events: RequestedReviewEvent[], login: string): st return mine.reduce((a, b) => (b.createdAt > a.createdAt ? b : a)).createdAt; } +// 100 is the page maximum, and it costs exactly what 20 would: one call. The +// window has to hold every request event on the PR, not just the recent ones — +// a PR that cycled through a dozen reviewers can push the request that named +// *you* off the end, and the answer would then be "nobody asked", silently +// restoring the bug this exists to fix. const LAST_REQUEST_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ repository(owner:$owner,name:$repo){ pullRequest(number:$number){ - timelineItems(last:20,itemTypes:[REVIEW_REQUESTED_EVENT]){ + timelineItems(last:100,itemTypes:[REVIEW_REQUESTED_EVENT]){ nodes{... on ReviewRequestedEvent{createdAt requestedReviewer{__typename ... on User{login}}}} } } @@ -395,8 +400,8 @@ const LAST_REQUEST_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ * * GraphQL rather than the REST timeline on purpose: this runs on rows the queue * already holds, poll after poll, and the timeline of a busy PR is hundreds of - * events across several pages. `timelineItems(last:20, itemTypes:[…])` is one - * call whatever the PR's history looks like. + * events across several pages. Filtered to the one event type, the last page + * holds the lot in one call whatever the PR's history looks like. */ export async function fetchLastReviewRequest(ref: PrRef, login: string): Promise { const out = await gh([ From 2f1f17602534f87f4f9c268a29d1f37fd9f9d5d5 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:47:07 +0200 Subject: [PATCH 4/4] docs(inbox): say what the request window actually covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #43. The rationale claimed the query holds every request event on the PR; it holds the last 100, which is a window and not a guarantee. Say so, and say why it is not paginated: one call per extra page on every settled row on every poll, to cover a PR that does not realistically exist — and the failure mode is the conservative one, a row left settled rather than one wrongly reopened. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/gh.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/core/gh.ts b/src/core/gh.ts index 000d0eb..7a087d5 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -380,11 +380,17 @@ export function lastRequestOf(events: RequestedReviewEvent[], login: string): st return mine.reduce((a, b) => (b.createdAt > a.createdAt ? b : a)).createdAt; } -// 100 is the page maximum, and it costs exactly what 20 would: one call. The -// window has to hold every request event on the PR, not just the recent ones — -// a PR that cycled through a dozen reviewers can push the request that named -// *you* off the end, and the answer would then be "nobody asked", silently -// restoring the bug this exists to fix. +// 100 is the page maximum, and it costs exactly what a smaller window would: +// one call. It wants to be big because the recent events are not the useful +// ones — a PR that cycled through a dozen reviewers can push the request that +// named *you* past a short window, and the answer would come back "nobody +// asked", silently restoring the bug this exists to fix. +// +// It is still a window, not a guarantee: a PR carrying more than 100 review +// requests would lose the oldest, and that is accepted rather than paginated. +// Paginating would cost one call per extra page on every settled row on every +// poll, to cover a PR that does not realistically exist — and the failure mode +// is the conservative one, a row left settled rather than one wrongly reopened. const LAST_REQUEST_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ repository(owner:$owner,name:$repo){ pullRequest(number:$number){ @@ -399,9 +405,11 @@ const LAST_REQUEST_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ * Ask GitHub when your review was last requested on a PR. * * GraphQL rather than the REST timeline on purpose: this runs on rows the queue - * already holds, poll after poll, and the timeline of a busy PR is hundreds of - * events across several pages. Filtered to the one event type, the last page - * holds the lot in one call whatever the PR's history looks like. + * already holds, poll after poll, and the REST timeline of a busy PR is hundreds + * of events across several pages — every one of them fetched to find the handful + * that are review requests. Filtering server-side to the one event type turns + * that into a single call; the window it reads is bounded, as the query above + * explains. */ export async function fetchLastReviewRequest(ref: PrRef, login: string): Promise { const out = await gh([