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..aa3f4ab 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. 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 +`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..7a087d5 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -356,6 +356,80 @@ 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; +} + +// 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){ + timelineItems(last:100,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 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([ + "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..a257d44 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,68 @@ 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, 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; + 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 +429,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 +463,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..8c05bc9 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,37 @@ 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); + }); + + // `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 e3a63cc..636d2b9 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,16 @@ 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(); + // 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 = {