diff --git a/CLAUDE.md b/CLAUDE.md index b307657..738a75b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -143,15 +143,26 @@ any of them — it names the files each rule lives in. are one JSON file with a zod schema and sane defaults; absent must keep working, and every field must be hand-editable - Don't handle API keys — `gh` and `claude` own auth -- Don't let a re-review discard human work: comments the user wrote or edited - are carried across (`carryOverComments`), never regenerated away. Nor is the - decision to be done with a PR: `reviewed` and `skipped` survive a new push +- A re-review replaces the whole draft, comments included — the user's own + along with the AI's. That is a decision, not a gap to fix: half-keeping them + (carrying on success, losing on failure) costs the code and still loses the + work, so cerber does neither and `docs/lifecycle.md` tells the reader plainly + instead. Don't add carry-over back without changing that page too. +- Don't let a re-review discard a *decision*, on any path it can take. Its + result is folded onto whatever the artifact says now (`mergeRunResult`) + rather than written over it, so a send stands and the chat is kept. + `reviewed` and `skipped` survive a new push (`SETTLED_BY_YOU` in `review.ts`), because a row dragged back into the inbox - every time the author pushes is cerber overruling you. Only `ready` and - `sent` track the head; the way back in is the re-review button, which forces. A chat turn - is the second way to lose it — it refuses to rewrite the user's own comments, - and its result is folded onto the current artifact (`mergeConcurrentEdits`) - rather than overwriting whatever they edited while the turn ran + every time the author pushes is cerber overruling you, and a settle or a send + that lands mid-run outlives the run. Only `ready` and `sent` track the head — + against `run.reviewedSha`, the commit the AI actually read, never `pr.headSha`, + which a refresh moves forward whenever a draft is merely opened. The way back + in is the re-review button, which forces. A chat turn is held to a stricter + line than a re-review — it refuses to rewrite the user's own comments, and + its result is folded onto the current artifact (`mergeConcurrentEdits`) + rather than overwriting whatever they edited while the turn ran. The + difference is deliberate: a re-review is asking for a new draft, a chat turn + is asking for an edit to this one - Don't add an accept step to a revision the user asked for. The chat agent writes the draft directly; Send is where a human vouches for what reaches GitHub, and one pre-chat snapshot is the way back. A per-turn undo is diff --git a/docs/lifecycle.md b/docs/lifecycle.md index a3c8cbb..5f6ec78 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -120,23 +120,25 @@ Notes on the edges that surprise people: - **A restart turns a leftover `running` into `failed`** and marks a pending chat turn errored (`reconcileRunning`, `src/core/state.ts`). Nothing in a fresh process is actually running, so anything still marked so would wedge - forever. It is not ordered before the first poll, though — `serve` starts the - daemon (which polls at once) before it awaits reconciliation, so the two can - race over a leftover row. Tracked as - [#38](https://github.com/fullstackhouse/cerber/issues/38). -- **`failed` is retried by the poll** — it is neither settled nor + forever. `serve` does it before it starts anything that polls, and + reconciliation also skips any run this process actually owns (`inUse`), so a + first tick that overlaps it cannot have "interrupted" stamped over a review + that has just legitimately begun. +- **`failed` is retried automatically** — it is neither settled nor head-sensitive, so the freshness guard below lets it through every time. Only - by the poll, though: `reviewAll` runs solely when auto-review is on and solely - over PRs the awaiting search returned, so a failed artifact GitHub has stopped - asking about is never retried on its own. + by the poll, and only on its terms: `reviewAll` runs solely when auto-review + is on and solely over PRs the awaiting search returned, so a failed artifact + GitHub has stopped asking about is never picked up *on its own*. You can + always start one yourself — both forcing paths take any unsent status. - **Nothing *reaches GitHub* except by a deliberate human act — the cockpit's Send button or `cerber send` — or by opt-in auto-send.** That is the one hard - rule of the product, and it is a rule about GitHub writes rather than about - the status field: `PATCH /api/reviews/:key` will set `status` to `sent` on - request without submitting anything or writing a `sent` record. The cockpit - never asks it to, and nothing is posted either way, but the endpoint is - wider than the rule it looks like it enforces — - [#39](https://github.com/fullstackhouse/cerber/issues/39). + rule of the product. The status field is held to it as well: + `PATCH /api/reviews/:key` takes only `reviewed` and `skipped`, the two that + are your decision, so nothing but the send path can write `sent`. +- **Neither will send a draft a run is rewriting.** The cockpit answers `409`, + refusing on the artifact's status *and* on this process's own claim; + `cerber send` prints the reason and exits non-zero, and goes on the status + alone, which is all another terminal can see. --- @@ -149,8 +151,9 @@ a token it checks the artifact already on disk: `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.* -3. Status in `HEAD_SENSITIVE` (`ready`, `sent`) **and** head SHA unchanged? → - **skip** as up to date. +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.) 4. Otherwise → run. So a `ready` **or `sent`** artifact on a PR that gets a new commit is meant to @@ -161,36 +164,27 @@ look (`review.test.ts`). Both need the PR to still be in the awaiting search: `reviewAll(refs)` is fed the search results, so a PR you pasted by hand and that GitHub isn't asking you about is never auto re-reviewed. -> ⚠ **Opening a review currently suppresses that re-draft.** The guard compares -> `existing.pr.headSha` against the PR's head, and the refresh that runs when -> you open a review writes the new head onto the artifact while leaving its -> status `ready` (`refreshArtifact`). So: the author pushes, you open the draft -> to look at it, and the poll now reads the row as up to date and never -> re-reviews it. Only the two forcing paths get past it — the re-review button -> and `cerber review --force`. The artifact keeps no -> record of which commit the *AI* actually read — `refresh.toSha` is the -> closest thing — so the guard has nothing else to compare against. - -**A re-review that succeeds does not throw away your work.** -`carryOverComments` (`src/core/refresh.ts`) carries your own comments *and* AI -comments you edited onto the new diff, re-anchored. AI comments you didn't -touch are dropped — the new run just regenerated them. - -> ⚠ **Two ways it loses them anyway**, both from the same cause — the runner -> saves its result wholesale instead of merging onto what is on disk, the way -> a chat turn does (`mergeConcurrentEdits`). -> -> - **A run that fails throws them away permanently.** It saves a fresh -> artifact with `comments: []` before calling Claude, so they leave the disk -> there and live only in the run's memory; `carryOverComments` puts them back -> on the success path, and the failure handler never reaches it. -> - **A run that succeeds still overwrites whatever you did while it ran.** The -> carry-over works from `existing`, read minutes earlier at the start of the -> run, and the review stays editable throughout — so a comment you add, edit -> or drop mid-run is written over by the result. -> -> Both tracked as [#37](https://github.com/fullstackhouse/cerber/issues/37); -> this warning goes when the fix lands. +**Opening a review does not count as reviewing it.** The comparison is against +`run.reviewedSha` — the head this artifact's last run actually *read*, written +when it finished — and not against `pr.headSha`, which the refresh moves +forward every time you open a draft so its comments stay anchored to current +code. Those two used to be the same field, so merely looking at a draft after a +push convinced the guard the draft was current and the poll never re-reviewed +that PR again. An artifact written before `reviewedSha` existed has none, and +falls back to the old comparison. + +**A re-review replaces the whole draft, including comments you wrote.** They +are dropped when the run starts and they do not come back — not on success, not +if the run fails. This is a decision rather than an oversight, and it is stated +here rather than left to be discovered: if you have written comments you want +to keep, send the review or copy them out before pressing re-review. + +What a re-review does *not* touch are the decisions you have made +(`mergeRunResult`, `src/core/refresh.ts`). The run's result is folded onto +whatever the artifact says now rather than written over it, so a send stands, a +`reviewed` or `skipped` you set while the run was going stands — with the fresh +draft underneath it, which is what the row shows if you change your mind — and +the chat transcript is kept. Only the draft itself is the run's to replace. ### Re-review vs refresh — different things @@ -312,11 +306,15 @@ leave something behind: request is its only evidence and a `null` cannot be told from a draft you asked for. 3. **On success** — `ready`, plus `summary`, `chapters`, `comments`, `verdict`, - `run.costUsd`, and `run.sessionId` — the Claude session chat turns resume, - recorded **only for a source-backed run** (`source ? review.sessionId : null`), - since there is no checkout for a `--no-source` turn to resume into. -4. **On failure** — `failed`, with `run.error` — but only for failures *after* - step 1. `reviewPr` fetches the diff and resolves trust and the checkout + `run.costUsd`, and `run.reviewedSha`, the head this run actually read, which + is what §4's freshness guard compares against. Also `run.sessionId` — the + Claude session chat turns resume, recorded **only for a source-backed run** + (`source ? review.sessionId : null`), since there is no checkout for a + `--no-source` turn to resume into. +4. **On failure** — `run.error`, and `failed` *unless the status is yours*: a + `sent`, `reviewed` or `skipped` that landed while the run worked stands, and + the error is recorded beside it (`userOwnsStatus`). Only for failures *after* + step 1, at that. `reviewPr` fetches the diff and resolves trust and the checkout before it first saves, so a failure there leaves no `failed` row behind for a direct caller (`cerber review`, the poll) to find. The cockpit's endpoints close that hole themselves: create and re-review both persist `running` @@ -359,8 +357,11 @@ read-only) and auto-send does nothing but log. | `daemon.repos` | `[]` | empty = everything `gh` can see | | `trust` | `[]` | whose PRs may run commands: `@login`, `@org/team`, `@org/*`, and `!`-prefixed denials (`!@org/team`) that carve an exception out of a broader grant. Denials win | -CLI flags cap the config, never raise it: `--no-poll`, `--no-auto-review`, -`--no-source`, `--no-trust`. +CLI flags cap the config, never raise it — `--no-poll`, `--no-auto-review`, +`--no-trust`. `--no-source` is the exception: it sets the default a run starts +from, and the re-review endpoint's `?source=` overrides it either way, so the +cockpit's button can ask for a source-backed run on a server started without +one. **Auto-send** is deliberately narrow (`src/core/autosend.ts`): only `ready`, only an `approve` verdict, only with **zero** standing blocker findings, only @@ -386,14 +387,15 @@ review. and auto-review both on and the PR still in the awaiting search. Given that: 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, and a `ready` -one you have opened since the push will *not* come back (the §4 warning). Only +— 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. **"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`): -status is `reviewed`/`skipped`, or the head SHA is unchanged on a `ready`/`sent` -row. `cerber review --force` forces past both. The cockpit's re-review button +status is `reviewed`/`skipped`, or the head the last run *read* is still the +PR's head on a `ready`/`sent` row. `cerber review --force` forces past both. The cockpit's re-review button forces too, but refuses a `sent` artifact outright — that record is not rewritten from the UI, though the poll will still re-draft it once the head moves. @@ -402,9 +404,8 @@ moves. `filed.reason` says which of the three cases. The draft is untouched and still sendable. -**"Where did my edited comment go after a re-review?"** — If it was there when -the run started and the run succeeded: still there, re-anchored; only untouched -AI comments are regenerated. If you wrote or changed it *while* the run was -going, or the run **failed**, it is gone — both are -[#37](https://github.com/fullstackhouse/cerber/issues/37), and the warning in -§4 says why. +**"Where did my edited comment go after a re-review?"** — Gone, deliberately: a +re-review regenerates the draft and your comments go with it, whether the run +succeeded or failed. Send or copy anything you want to keep first. What does +survive is the chat, and any decision you had already made — a send, a +`reviewed`, a `skipped`. diff --git a/src/cli/index.ts b/src/cli/index.ts index b524160..38eb6e8 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -13,9 +13,11 @@ import { listArtifacts, loadArtifact, readAutoSendLog, + reconcileRunning, updateArtifactByKey, } from "../core/state.js"; import { pool, reviewPr } from "../runner/review.js"; +import { isReviewRunning } from "../runner/inflight.js"; import { startDaemon } from "../server/daemon.js"; import { startServer } from "../server/index.js"; @@ -174,6 +176,17 @@ program console.error(`Already sent at ${artifact.sent.at}${artifact.sent.url ? ` — ${artifact.sent.url}` : ""}`); process.exit(1); } + // The persisted status is the only signal here: a run started by `serve` or + // by another terminal is invisible to this process. Sending mid-run vouches + // for a draft that is being replaced as you read it. + if (artifact.status === "running") { + console.error( + `A review of ${id} is running — wait for it to finish, then send. ` + + `If nothing is actually running, the last run died mid-flight: re-run it, or restart \`cerber serve\`, ` + + `either of which clears the stale status.`, + ); + process.exit(1); + } const event = ( opts.event ?? @@ -459,6 +472,13 @@ program ); } + // Before anything polls. Reconciliation is what makes "nothing is running" + // true after an unclean shutdown, and the daemon's first tick fires the + // moment it is constructed — so starting it first put both of them on the + // same leftover artifact with no ordering between them. + const cleared = await reconcileRunning({ inUse: isReviewRunning }); + if (cleared > 0) console.log(`Cleared ${cleared} interrupted run(s) left by a previous process.`); + const daemon = opts.poll ? startDaemon({ repos: opts.repo.length > 0 ? opts.repo : d.repos, diff --git a/src/core/artifact.ts b/src/core/artifact.ts index d1a6f9e..fb36d4f 100644 --- a/src/core/artifact.ts +++ b/src/core/artifact.ts @@ -111,6 +111,17 @@ export const RunInfoSchema = z.object({ * be filed away automatically and one that must not be. */ trigger: z.enum(["daemon", "user"]).nullable().default(null), + /** + * The head commit this run actually read, set when it finishes. + * + * `pr.headSha` cannot answer that question: a refresh moves it forward to + * keep the comments anchored to current code, without anybody re-reading + * anything. The freshness guard compares against *this* instead, so merely + * opening a draft after a push no longer convinces the poll that the draft + * is up to date. Null on runs recorded before this existed, and on one still + * in flight — both fall back to the old comparison. + */ + reviewedSha: z.string().nullable().default(null), }); export type RunInfo = z.infer; diff --git a/src/core/refresh.test.ts b/src/core/refresh.test.ts index 443f5ac..27b942a 100644 --- a/src/core/refresh.test.ts +++ b/src/core/refresh.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { Artifact, Comment } from "./artifact.js"; -import { carryOverComments, refreshArtifact } from "./refresh.js"; +import { mergeRunResult, refreshArtifact } from "./refresh.js"; const DIFF_AT_HEAD1 = `diff --git a/src/a.ts b/src/a.ts --- a/src/a.ts @@ -128,45 +128,50 @@ describe("refreshArtifact", () => { }); }); -describe("carryOverComments", () => { - const previous = makeArtifact({ - comments: [ - comment({ id: "ai-untouched", origin: "ai" }), - comment({ id: "ai-edited", origin: "ai", editedByUser: true, body: "my rewrite" }), - comment({ id: "mine", origin: "user", body: "my own note" }), - comment({ id: "mine-dropped", origin: "user", status: "dropped" }), - ], - }); +describe("mergeRunResult — folding a finished run onto what is on disk", () => { + // The run owns the draft outright, comments included; what it may not touch + // are the decisions the user made while it worked. const fresh = makeArtifact({ diff: DIFF_AT_HEAD2, + summary: "the new draft", comments: [comment({ id: "ai-new", body: "fresh ai comment" })], }); - it("keeps the human's comments and drops the regenerated AI ones", () => { - const { comments, carried } = carryOverComments(previous, fresh); - expect(carried).toBe(3); - expect(comments.map((c) => c.id)).toEqual(["ai-new", "ai-edited", "mine", "mine-dropped"]); - expect(comments.find((c) => c.id === "ai-edited")!.body).toBe("my rewrite"); - expect(comments.find((c) => c.id === "mine-dropped")!.status).toBe("dropped"); + it("replaces the previous draft's comments, whoever wrote them", () => { + const current = makeArtifact({ + comments: [ + comment({ id: "ai-untouched", origin: "ai" }), + comment({ id: "ai-edited", origin: "ai", editedByUser: true, body: "my rewrite" }), + comment({ id: "mine", origin: "user", body: "my own note" }), + ], + }); + const merged = mergeRunResult(fresh, current); + expect(merged.comments.map((c) => c.id)).toEqual(["ai-new"]); + expect(merged.summary).toBe("the new draft"); }); - it("re-anchors what it carries to the new diff", () => { - const { comments } = carryOverComments(previous, fresh); - expect(comments.find((c) => c.id === "mine")!.line).toBe(12); + it("does not undo a send that landed while the run worked", () => { + const sent = { at: "2026-08-21T10:02:00.000Z", event: "APPROVE" as const, url: "u", auto: false }; + const merged = mergeRunResult(fresh, makeArtifact({ status: "sent", sent })); + expect(merged.sent).toEqual(sent); + expect(merged.status).toBe("sent"); + expect(merged.summary).toBe("the new draft"); }); - it("leaves a review with no human input alone", () => { - const untouched = makeArtifact({ comments: [comment({ id: "ai-1" })] }); - const { comments, carried } = carryOverComments(untouched, fresh); - expect(carried).toBe(0); - expect(comments).toBe(fresh.comments); + it("does not reopen a decision you made while the run worked", () => { + for (const status of ["reviewed", "skipped"] as const) { + expect(mergeRunResult(fresh, makeArtifact({ status })).status).toBe(status); + } }); - it("carries a comment's severity across a re-review", () => { - const graded = makeArtifact({ - comments: [comment({ id: "mine", origin: "user", severity: "blocker" })], - }); - const { comments } = carryOverComments(graded, fresh); - expect(comments.find((c) => c.id === "mine")!.severity).toBe("blocker"); + it("keeps the conversation, which is the user's writing", () => { + const chat = [ + { id: "t1", role: "user" as const, at: "2026-08-21T10:01:00.000Z", body: "why?", refs: [], revisions: [], refused: [], costUsd: null }, + ]; + expect(mergeRunResult(fresh, makeArtifact({ chat })).chat).toEqual(chat); + }); + + it("otherwise takes the run's own status", () => { + expect(mergeRunResult(fresh, makeArtifact({ status: "running" })).status).toBe(fresh.status); }); }); diff --git a/src/core/refresh.ts b/src/core/refresh.ts index 6231860..2dce76a 100644 --- a/src/core/refresh.ts +++ b/src/core/refresh.ts @@ -46,26 +46,51 @@ export function refreshArtifact(artifact: Artifact, pr: PrInfo, diff: string): R }; } + /** - * Comments a human put work into: their own, and AI ones they rewrote. - * A re-review regenerates everything the AI said, but must not silently bin - * these — see carryOverComments. + * Is this artifact's status the user's to keep, rather than the run's to set? + * + * A send or a settle is a decision about the PR; a run finishing is a fact + * about the code. Whichever way the run went — a draft or an error — it does + * not get to reopen one, which is the same rule `SETTLED_BY_YOU` applies to + * starting a run in the first place. */ -export function humanComments(artifact: Artifact): Artifact["comments"] { - return artifact.comments.filter((c) => c.origin === "user" || c.editedByUser); +export function userOwnsStatus(a: Artifact): boolean { + return a.sent !== null || a.status === "reviewed" || a.status === "skipped"; } /** - * Carry a previous review's human comments into a fresh one, re-anchored to - * the new diff. AI comments are dropped: the new run just regenerated them. + * Fold a finished review run onto whatever the artifact says now. + * + * A run takes minutes and the cockpit stays live throughout, so by the time one + * lands the artifact may have moved under it. Saving the result wholesale + * silently undid whatever had happened in between — the same mistake + * `mergeConcurrentEdits` exists to stop a chat turn making. + * + * `fresh` is what the run produced; `current` is what is on disk now. The run + * owns the draft — summary, chapters, verdict, comments — and that includes + * replacing comments the user wrote: a re-review starts fresh by design, and + * `docs/lifecycle.md` says so plainly rather than leaving anyone to count on + * work surviving one. What it does *not* own are the decisions: + * + * - **a send** stands, and takes the status with it. Without this a run + * finishing after a send wrote `sent: null` back over the record, and the + * "already sent" guard would then wave a second submission through. + * - **a settle** stands too: `reviewed` and `skipped` are decisions about the + * PR, not facts about the code, so a run completing does not reopen one. + * The fresh draft still lands underneath, which is what the row shows if + * the user changes their mind. + * - **the conversation** is the user's writing, snapshot and all. */ -export function carryOverComments( - previous: Artifact, - fresh: Artifact, -): { comments: Artifact["comments"]; carried: number; drifted: number } { - const human = humanComments(previous); - if (human.length === 0) return { comments: fresh.comments, carried: 0, drifted: 0 }; - - const { comments, drifted } = reanchorComments(human, previous.diff, fresh.diff); - return { comments: [...fresh.comments, ...comments], carried: comments.length, drifted }; +export function mergeRunResult(fresh: Artifact, current: Artifact): Artifact { + return { + ...fresh, + status: userOwnsStatus(current) ? current.status : fresh.status, + sent: current.sent, + calibration: current.calibration, + filed: current.filed, + chat: current.chat, + preChat: current.preChat, + pendingChat: current.pendingChat, + }; } diff --git a/src/core/revise.ts b/src/core/revise.ts index 6c71b7a..ea6d874 100644 --- a/src/core/revise.ts +++ b/src/core/revise.ts @@ -8,7 +8,7 @@ import { Artifact, Refusal, Revision, ReviewSnapshot } from "./artifact.js"; * asking again with an "accept" button would be friction wearing safety's coat. * What it must NOT do is quietly overwrite the user's own words: a comment the * user wrote or rewrote is the one thing in the artifact the AI did not author, - * and `carryOverComments` already exists so a re-review never discards it. A + * and a re-review already carries it across rather than discarding it. A * chat turn is the second way to lose that work, and this is where it doesn't. * * Pure: no I/O, no clock beyond what the caller passes in. diff --git a/src/core/state.test.ts b/src/core/state.test.ts index b56606e..62cc30a 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -69,6 +69,7 @@ describe("reconcileRunning", () => { trusted: false, sessionId: null, trigger: null, + reviewedSha: null, }, }), ); @@ -91,6 +92,35 @@ describe("reconcileRunning", () => { expect(after?.pendingChat?.error).toMatch(/interrupted/); }); + it("leaves a run this process actually owns alone", async () => { + // `serve` reconciles before it starts anything that polls, but ordering is + // a promise about wiring and this is the guard that holds regardless: a + // reconciliation racing a daemon's first tick must not stamp "interrupted" + // over a review that has just legitimately begun. + await saveArtifact( + artifact({ + status: "running", + run: { + model: null, + startedAt: "t", + finishedAt: null, + costUsd: null, + error: null, + withSource: true, + trusted: false, + sessionId: null, + trigger: "daemon", + reviewedSha: null, + }, + }), + ); + + expect(await reconcileRunning({ inUse: (id) => id === "acme/widgets#42" })).toBe(0); + const after = await loadArtifact("acme/widgets#42"); + expect(after?.status).toBe("running"); + expect(after?.run?.error).toBeNull(); + }); + it("leaves a turn that already failed, and everything else, alone", async () => { await saveArtifact( artifact({ pendingChat: { message: "why?", refs: [], startedAt: "t", progress: [], error: "boom" } }), diff --git a/src/core/state.ts b/src/core/state.ts index f30df7d..8428b8b 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -112,11 +112,22 @@ export async function updateArtifactByKey( * this it stays wedged forever, with no way to retry it from the cockpit. A * `cerber review` running in another terminal is the one false positive; it * overwrites the artifact when it finishes anyway. + * + * `inUse` names the runs this process genuinely owns, and they are left alone. + * Callers should still reconcile before starting anything that polls — but + * ordering alone is a promise about wiring, and this is the guard that holds + * whatever order they are started in: without it, reconciliation racing a + * daemon's first tick could stamp "interrupted" over a review that had just + * legitimately begun. */ -export async function reconcileRunning(): Promise { +export async function reconcileRunning( + opts: { inUse?: (id: string) => boolean } = {}, +): Promise { + const inUse = opts.inUse ?? (() => false); const artifacts = await listArtifacts(); let cleared = 0; for (const artifact of artifacts) { + if (inUse(artifact.id)) continue; const stuckRun = artifact.status === "running"; const stuckChat = artifact.pendingChat && artifact.pendingChat.error == null ? artifact.pendingChat : null; diff --git a/src/runner/chat.test.ts b/src/runner/chat.test.ts index a4fa34e..dbffaf1 100644 --- a/src/runner/chat.test.ts +++ b/src/runner/chat.test.ts @@ -64,6 +64,7 @@ function artifact(over: Partial = {}): Artifact { trusted: false, sessionId: "sess-review", trigger: null, + reviewedSha: null, }, sent: null, refresh: null, diff --git a/src/runner/review-merge.test.ts b/src/runner/review-merge.test.ts new file mode 100644 index 0000000..1ed2b1b --- /dev/null +++ b/src/runner/review-merge.test.ts @@ -0,0 +1,206 @@ +import { mkdtempSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; +import { Artifact, Comment, PrInfo, SCHEMA_VERSION } from "../core/artifact.js"; +import { fetchPrDiff, fetchPrInfo } from "../core/gh.js"; +import { loadArtifact, saveArtifact, updateArtifactByKey } from "../core/state.js"; +import { runClaude } from "./claude.js"; +import { reviewPr } from "./review.js"; + +// What the user did while the run was going is the whole subject here, so the +// run is driven end to end with only `claude` and GitHub stubbed out. +vi.mock("../core/gh.js", async (orig) => ({ + ...(await orig()), + fetchPrInfo: vi.fn(), + fetchPrDiff: vi.fn(), +})); +vi.mock("./claude.js", async (orig) => ({ + ...(await orig()), + runClaude: vi.fn(), +})); + +const prInfo = fetchPrInfo as Mock; +const diff = fetchPrDiff as Mock; +const claude = runClaude as Mock; + +process.env.CERBER_HOME = mkdtempSync(path.join(os.tmpdir(), "cerber-merge-")); + +const REF = { owner: "acme", repo: "widgets", number: 7 }; +const ID = "acme/widgets#7"; +const KEY = "acme__widgets__7"; + +/** One hunk, two context lines — enough for a comment to anchor to by text. */ +const DIFF = ["--- a/a.ts", "+++ b/a.ts", "@@ -1,2 +1,2 @@", " const x = 1;", "+const y = 2;"].join("\n"); + +function pr(headSha: string): PrInfo { + return { + ...REF, + title: "feat: add sprockets", + url: "https://github.com/acme/widgets/pull/7", + author: "someone", + body: "", + baseRefName: "main", + headRefName: "feature", + headSha, + state: "OPEN", + isDraft: false, + additions: 1, + deletions: 0, + changedFiles: 1, + }; +} + +function comment(over: Partial = {}): Comment { + return { + id: "c1", + path: "a.ts", + line: 2, + body: "I wrote this myself", + chapterId: null, + severity: null, + origin: "user", + status: "draft", + editedByUser: false, + originalLine: null, + drifted: false, + ...over, + }; +} + +function ready(comments: Comment[], headSha = "old-sha"): Artifact { + const now = "2026-08-21T10:00:00.000Z"; + return { + schemaVersion: SCHEMA_VERSION, + id: ID, + status: "ready", + createdAt: now, + updatedAt: now, + pr: pr(headSha), + diff: DIFF, + summary: "the previous draft", + chapters: [], + comments, + verdict: null, + run: null, + sent: null, + filed: null, + refresh: null, + calibration: null, + chat: [], + preChat: null, + pendingChat: null, + }; +} + +/** A well-formed AI answer, so the run reaches its success path. */ +const AI_ANSWER = JSON.stringify({ + summary: "the new draft", + chapters: [], + comments: [{ path: "a.ts", line: 2, body: "the AI's finding", chapterId: null, severity: "minor" }], + verdict: { recommendation: "approve", confidence: 90, reasoning: "looks fine" }, +}); + +/** Let the caller act on the artifact partway through the run, as a user would. */ +function claudeThat(midRun: () => Promise, answer: string | Error = AI_ANSWER) { + claude.mockImplementation(async () => { + await midRun(); + if (answer instanceof Error) throw answer; + return { text: answer, sessionId: null, costUsd: null, model: null }; + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + prInfo.mockResolvedValue(pr("new-sha")); + diff.mockResolvedValue(DIFF); +}); + +describe("what a re-review does to the previous draft", () => { + it("replaces the comments wholesale, including ones you wrote", async () => { + // A decided behaviour, not an oversight: a re-review starts fresh. Pinned + // so that changing it back is a deliberate act with a failing test, and so + // `docs/lifecycle.md` cannot quietly drift from what the code does. + await saveArtifact(ready([comment({ id: "mine", body: "I wrote this myself" })])); + claudeThat(async () => {}); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.comments.map((c) => c.body)).toEqual(["the AI's finding"]); + }); + + it("does not accumulate the AI's own comments either", async () => { + await saveArtifact(ready([comment({ id: "old-ai", origin: "ai", body: "a stale AI finding" })])); + claudeThat(async () => {}); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.comments.map((c) => c.body)).toEqual(["the AI's finding"]); + }); + + it("leaves nothing behind when it fails", async () => { + // The failure path holds the same line as the success path: no half-kept + // draft, so what a reader is told about re-review is true either way. + await saveArtifact(ready([comment({ id: "mine", body: "I wrote this myself" })])); + claudeThat(async () => {}, new Error("model unavailable")); + + await expect(reviewPr(REF, { withSource: false })).rejects.toThrow("model unavailable"); + const after = (await loadArtifact(ID))!; + expect(after.status).toBe("failed"); + expect(after.comments).toEqual([]); + }); +}); + +describe("what a re-review does to a decision you made while it ran", () => { + it("does not undo a send", async () => { + // The run used to write its own `sent: null` over the record, after which + // the send path's "already sent" guard would wave a second submission + // through — one click, two reviews on the PR. + await saveArtifact(ready([])); + const sent = { at: "2026-08-21T10:02:00.000Z", event: "APPROVE" as const, url: "u", auto: false }; + claudeThat(async () => { + await updateArtifactByKey(KEY, (a) => ({ ...a, status: "sent" as const, sent })); + }); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.sent).toEqual(sent); + expect(artifact.status).toBe("sent"); + // And the fresh draft still landed underneath it. + expect(artifact.summary).toBe("the new draft"); + }); + + it("does not reopen one you settled", async () => { + for (const status of ["reviewed", "skipped"] as const) { + await saveArtifact(ready([])); + claudeThat(async () => { + await updateArtifactByKey(KEY, (a) => ({ ...a, status })); + }); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.status).toBe(status); + expect(artifact.summary).toBe("the new draft"); + } + }); + + 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. + await saveArtifact(ready([])); + claudeThat(async () => { + await updateArtifactByKey(KEY, (a) => ({ ...a, status: "skipped" as const })); + }, new Error("model unavailable")); + + await expect(reviewPr(REF, { withSource: false })).rejects.toThrow("model unavailable"); + + const after = (await loadArtifact(ID))!; + expect(after.status).toBe("skipped"); + // The error is still recorded, so the row can say what happened. + expect(after.run?.error).toMatch(/model unavailable/); + }); + + it("records the head it actually read", async () => { + await saveArtifact(ready([])); + claudeThat(async () => {}); + + const { artifact } = await reviewPr(REF, { withSource: false }); + expect(artifact.run?.reviewedSha).toBe("new-sha"); + }); +}); diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index 66debbc..f274eb3 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -66,6 +66,20 @@ function artifact(status: ArtifactStatus, headSha: string): Artifact { }; } +/** A finished run, for the cases that turn on what it recorded. */ +const runBlock = { + model: null, + startedAt: "2026-08-21T10:00:00.000Z", + finishedAt: "2026-08-21T10:05:00.000Z", + costUsd: null, + error: null, + withSource: false, + trusted: false, + sessionId: null, + trigger: "daemon" as const, + reviewedSha: null as string | null, +}; + beforeEach(() => { vi.clearAllMocks(); diff.mockRejectedValue(new Error("a run started when it should not have")); @@ -116,6 +130,35 @@ describe("what a new push does to a review", () => { expect(diff).not.toHaveBeenCalled(); }); + it("still re-reviews a draft you opened after the push", async () => { + // Opening a review refreshes it, which moves `pr.headSha` onto the new head + // so the comments stay anchored to current code. Nothing was re-read, and + // the guard used to compare against that field — so merely looking at a + // draft convinced it the draft was current, and the poll never re-reviewed + // that PR again. It compares against the sha the AI actually read instead. + await saveArtifact({ + ...artifact("ready", "new-sha"), + run: { ...runBlock, reviewedSha: "old-sha" }, + refresh: { at: "2026-08-21T11:00:00.000Z", fromSha: "old-sha", toSha: "new-sha", moved: 1, drifted: 0 }, + }); + prInfo.mockResolvedValue(pr("new-sha")); + + await expect(reviewPr(REF)).rejects.toThrow("a run started when it should not have"); + expect(diff).toHaveBeenCalled(); + }); + + it("skips one whose recorded review is of this very head", async () => { + await saveArtifact({ + ...artifact("ready", "same-sha"), + run: { ...runBlock, reviewedSha: "same-sha" }, + }); + prInfo.mockResolvedValue(pr("same-sha")); + + const result = await reviewPr(REF); + expect(result.skipped).toBe(true); + expect(diff).not.toHaveBeenCalled(); + }); + it("re-reviews a sent one when the author pushes and asks again", async () => { // A submitted review clears GitHub's request, so a fresh one only ever // arrives because someone asked for another look. That is when a new draft diff --git a/src/runner/review.ts b/src/runner/review.ts index db206b8..afceb0d 100644 --- a/src/runner/review.ts +++ b/src/runner/review.ts @@ -6,11 +6,12 @@ import { PrInfo, SCHEMA_VERSION, artifactId, + artifactKey, } from "../core/artifact.js"; import { createRunDir, evictOldCheckouts, prepareCheckout, removeRunDir } from "../core/checkout.js"; import { PrRef, fetchPrDiff, fetchPrInfo, isOrgMember, isTeamMember } from "../core/gh.js"; -import { carryOverComments } from "../core/refresh.js"; -import { loadArtifact, saveArtifact } from "../core/state.js"; +import { mergeRunResult, userOwnsStatus } from "../core/refresh.js"; +import { loadArtifact, saveArtifact, updateArtifactByKey } from "../core/state.js"; import { loadConfig } from "../core/config.js"; import { decideTrust, membershipQueries, parseTrustRules } from "../core/trust.js"; import { ClaudeEvent, extractJson, runClaude, unauthenticatedEnv } from "./claude.js"; @@ -136,12 +137,15 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise log(`You marked this ${existing.status} — leaving it alone. Use --force to re-review.`); return { artifact: existing, skipped: true }; } - if ( - HEAD_SENSITIVE.has(existing.status) && - existing.pr.headSha !== "" && - existing.pr.headSha === pr.headSha - ) { - log(`Up to date (head ${pr.headSha.slice(0, 7)}, status ${existing.status}) — skipping. Use --force to re-review.`); + // The sha the AI *read*, not the one the artifact happens to mention. + // `pr.headSha` is moved forward by the refresh that runs whenever a review + // is opened, so comparing against it meant that merely looking at a draft + // after a push convinced this guard the draft was current, and the poll + // never re-reviewed it again. Older artifacts have no `reviewedSha` and + // fall back to the old comparison. + const reviewedSha = existing.run?.reviewedSha ?? existing.pr.headSha; + if (HEAD_SENSITIVE.has(existing.status) && reviewedSha !== "" && reviewedSha === pr.headSha) { + log(`Up to date (reviewed at ${reviewedSha.slice(0, 7)}, status ${existing.status}) — skipping. Use --force to re-review.`); return { artifact: existing, skipped: true }; } } @@ -173,6 +177,15 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise } } + // A re-review regenerates the draft, comments and all — deliberately, and + // including any the user wrote. Carrying them across is a feature this tool + // has decided not to have, so nothing here tries to half-keep them: they go + // when the run starts, whichever way it ends. `docs/lifecycle.md` says so + // where a reader would otherwise assume otherwise. + if (existing && existing.comments.length > 0) { + log(`Replacing the previous draft's ${existing.comments.length} comment(s) — a re-review starts fresh.`); + } + let artifact: Artifact = { schemaVersion: SCHEMA_VERSION, id: artifactId(pr), @@ -195,6 +208,7 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise trusted: trusted && source !== null, sessionId: null, trigger: opts.trigger ?? "user", + reviewedSha: null, }, sent: null, refresh: null, @@ -260,37 +274,36 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise // whose working directory was empty has nothing a chat turn could read. sessionId: source ? review.sessionId : null, trigger: artifact.run!.trigger, + // What this run actually read — the thing the freshness guard needs + // and `pr.headSha` cannot say, because a refresh moves that forward + // without anybody re-reading the code. + reviewedSha: pr.headSha, }, }; - - // A re-review regenerates the AI's comments, but the human's own comments - // and their rewrites of AI ones are work the run must not throw away. - if (existing) { - const { comments, carried, drifted } = carryOverComments(existing, artifact); - if (carried > 0) { - artifact = { ...artifact, comments }; - log( - `Carried over ${carried} comment(s) you wrote or edited` + - (drifted > 0 ? `; ${drifted} no longer match the diff and will post in the body.` : "."), - ); - } - } } catch (err: unknown) { - artifact = { - ...artifact, - status: "failed", - updatedAt: now(), - run: { - ...artifact.run!, - finishedAt: now(), - error: err instanceof Error ? err.message : String(err), - }, + const run = { + ...artifact.run!, + finishedAt: now(), + error: err instanceof Error ? err.message : String(err), }; - await saveArtifact(artifact); + // Onto what is on disk, not over it: the artifact this run built holds none + // of the comments or marks that may have landed while it ran, and a failure + // is no reason to lose them — nor to reopen a decision the user made. + const saved = await updateArtifactByKey(artifactKey(artifact.id), (a) => ({ + ...a, + status: userOwnsStatus(a) ? a.status : ("failed" as const), + run, + })); + artifact = saved ?? { ...artifact, status: "failed", run, updatedAt: now() }; + if (!saved) await saveArtifact(artifact); throw err; } - await saveArtifact(artifact); + const merged = await updateArtifactByKey(artifactKey(artifact.id), (current) => + mergeRunResult(artifact, current), + ); + if (merged) artifact = merged; + else await saveArtifact(artifact); return { artifact, skipped: false }; } diff --git a/src/server/daemon.test.ts b/src/server/daemon.test.ts index 86dfd7f..807da4a 100644 --- a/src/server/daemon.test.ts +++ b/src/server/daemon.test.ts @@ -240,6 +240,7 @@ describe("stubArtifact", () => { trusted: false, sessionId: null, trigger: null, + reviewedSha: null, }, }), ).toBe(false); @@ -363,6 +364,7 @@ describe("draft PRs in the inbox", () => { trusted: false, sessionId: null, trigger: null, + reviewedSha: null, }, }); @@ -419,6 +421,7 @@ describe("a draft you already answered on GitHub", () => { trusted: true, sessionId: null, trigger: "daemon" as const, + reviewedSha: null, }; const YOUR_REVIEW = { @@ -655,6 +658,7 @@ describe("filedByYourAct", () => { trusted: false, sessionId: null, trigger: "user" as const, + reviewedSha: null, }; expect(filedByYourAct({ ...artifact, run }, sameSecond.at)).toBe(false); // And the same run started a second *before* it still files: the guard is @@ -686,6 +690,7 @@ describe("filedByWithdrawnRequest", () => { trusted: false, sessionId: null, trigger: "daemon" as const, + reviewedSha: null, }; it("only files what the poll wrote on its own", () => { diff --git a/src/server/guards.test.ts b/src/server/guards.test.ts new file mode 100644 index 0000000..416cec4 --- /dev/null +++ b/src/server/guards.test.ts @@ -0,0 +1,169 @@ +import { mkdtempSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; +import { Artifact, ArtifactStatus, PrInfo, SCHEMA_VERSION } from "../core/artifact.js"; +import { submitReview } from "../core/gh.js"; +import { loadArtifact, saveArtifact } from "../core/state.js"; +import { beginReview, endReview } from "../runner/inflight.js"; +import { buildApp } from "./index.js"; + +// The only GitHub write in the product. Stubbed so a test can assert it was +// *not* called, which is the whole subject of half this file. +vi.mock("../core/gh.js", async (orig) => ({ + ...(await orig()), + submitReview: vi.fn(), +})); +vi.mock("../runner/review.js", () => ({ reviewPr: vi.fn(), pool: vi.fn() })); + +const submit = submitReview as Mock; + +const home = mkdtempSync(path.join(os.tmpdir(), "cerber-guards-")); +process.env.CERBER_HOME = home; + +const ID = "acme/widgets#42"; +const KEY = "acme__widgets__42"; + +function pr(): PrInfo { + return { + owner: "acme", + repo: "widgets", + number: 42, + title: "Add a thing", + url: "https://github.com/acme/widgets/pull/42", + author: "someone", + body: "", + baseRefName: "main", + headRefName: "f", + headSha: "abc", + state: "OPEN", + isDraft: false, + additions: 1, + deletions: 0, + changedFiles: 1, + }; +} + +function artifact(status: ArtifactStatus): Artifact { + const now = "2026-08-21T10:00:00.000Z"; + return { + schemaVersion: SCHEMA_VERSION, + id: ID, + status, + createdAt: now, + updatedAt: now, + pr: pr(), + diff: "--- a\n+++ b\n", + summary: "a draft", + chapters: [], + comments: [], + verdict: { recommendation: "approve", confidence: 90, reasoning: "fine" }, + run: null, + sent: null, + filed: null, + refresh: null, + calibration: null, + chat: [], + preChat: null, + pendingChat: null, + }; +} + +const send = async () => { + const app = await buildApp({}); + return app.request(`/api/reviews/${KEY}/send`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ event: "APPROVE", confirm: true }), + }); +}; + +const patchStatus = async (status: string) => { + const app = await buildApp({}); + return app.request(`/api/reviews/${KEY}`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ status }), + }); +}; + +beforeEach(() => { + vi.clearAllMocks(); + submit.mockResolvedValue({ url: "https://github.com/acme/widgets/pull/42#r1" }); +}); + +describe("POST /api/reviews/:key/send — not while a run is rewriting the draft", () => { + it("refuses when the artifact says a run is in flight", async () => { + // Cross-process: a `cerber review` in another terminal is invisible to this + // one's in-memory claim, so the persisted status has to carry the answer. + await saveArtifact(artifact("running")); + + const res = await send(); + expect(res.status).toBe(409); + expect(submit).not.toHaveBeenCalled(); + }); + + it("refuses when this process is running one", async () => { + // And the mirror: a run this process started moments ago may not have + // written anything yet, so the in-memory claim has to carry that one. + await saveArtifact(artifact("ready")); + beginReview(ID); + try { + const res = await send(); + expect(res.status).toBe(409); + expect(submit).not.toHaveBeenCalled(); + } finally { + endReview(ID); + } + }); + + it("still sends a finished draft", async () => { + await saveArtifact(artifact("ready")); + + const res = await send(); + expect(res.status).toBe(200); + expect(submit).toHaveBeenCalledOnce(); + expect((await loadArtifact(ID))!.status).toBe("sent"); + }); + + it("refuses a second submission of one already sent", async () => { + await saveArtifact(artifact("ready")); + expect((await send()).status).toBe(200); + + const again = await send(); + expect(again.status).toBe(409); + expect(submit).toHaveBeenCalledOnce(); + }); +}); + +describe("PATCH /api/reviews/:key — only the statuses that are your decision", () => { + it("accepts reviewed and skipped", async () => { + for (const status of ["reviewed", "skipped"]) { + await saveArtifact(artifact("ready")); + const res = await patchStatus(status); + expect(res.status).toBe(200); + expect((await loadArtifact(ID))!.status).toBe(status); + } + }); + + 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 + // guard keyed on the record then read two different ways. + await saveArtifact(artifact("ready")); + + const res = await patchStatus("sent"); + expect(res.status).toBe(400); + const after = (await loadArtifact(ID))!; + expect(after.status).toBe("ready"); + expect(after.sent).toBeNull(); + expect(submit).not.toHaveBeenCalled(); + }); + + it("refuses the statuses the runner owns", async () => { + for (const status of ["running", "ready", "failed", "awaiting"]) { + await saveArtifact(artifact("ready")); + expect((await patchStatus(status)).status).toBe(400); + } + }); +}); diff --git a/src/server/index.ts b/src/server/index.ts index c201cbf..e3a63cc 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -27,7 +27,6 @@ import { listArtifacts, loadArtifact, loadArtifactByKey, - reconcileRunning, saveArtifact, updateArtifactByKey, } from "../core/state.js"; @@ -40,6 +39,22 @@ import { ChatTurnSchema } from "../core/artifact.js"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); +/** + * The statuses `PATCH /api/reviews/:key` will set: the two that are the user's + * own decision about a PR. Everything else is a fact some other path owns. + */ +const SETTLEABLE = ["reviewed", "skipped"]; + +/** + * Is an AI run rewriting this draft right now? + * + * Two questions, because neither alone is enough: `isReviewRunning` is an + * in-memory claim and cannot see a `cerber review` going in another terminal, + * while the persisted status cannot see a run this process started moments ago + * that has not written yet. + */ +const inFlight = (a: Artifact) => a.status === "running" || isReviewRunning(a.id); + export interface ServeOptions { port: number; host: string; @@ -295,6 +310,7 @@ export async function buildApp( trusted: false, sessionId: null, trigger: "user", + reviewedSha: null, }, sent: null, refresh: null, @@ -334,6 +350,17 @@ export async function buildApp( app.patch("/api/reviews/:key", async (c) => { const body = await c.req.json(); + // Only the two statuses that are the user's own decision. The schema + // permits all seven, and taking them all meant this endpoint would happily + // write `status: "sent"` with no `sent` record and nothing submitted — + // a row that claims a review reached GitHub, which no honest path produces. + // `running`/`ready`/`failed` belong to the runner, `sent` to the send path. + if (body.status !== undefined && !SETTLEABLE.includes(body.status)) { + return c.json( + { error: `status must be one of ${SETTLEABLE.join(", ")} — got ${body.status}` }, + 400, + ); + } const updated = await updateArtifactByKey(c.req.param("key"), (a) => { const next = { ...a }; if (body.status !== undefined) { @@ -483,6 +510,7 @@ export async function buildApp( trusted: false, sessionId: null, trigger: "user", + reviewedSha: null, }, })); @@ -659,6 +687,13 @@ export async function buildApp( if (artifact.sent) { return c.json({ error: `already sent at ${artifact.sent.at}` }, 409); } + // Not while a run is rewriting this draft. Vouching for a review that is + // being replaced under you is reason enough on its own; on top of that the + // run used to save `sent: null` back over the record when it landed, so the + // guard above would wave a *second* submission through afterwards. + if (inFlight(artifact)) { + return c.json({ error: "a review of this PR is running — wait for it to finish" }, 409); + } const payload = buildReviewPayload(artifact, event as ReviewEvent); try { @@ -708,13 +743,13 @@ export async function buildApp( return app; } +/** + * Serve the cockpit. `reconcileRunning` is deliberately *not* called here: the + * caller must have done it before starting the daemon, because the daemon polls + * as soon as it is constructed. Doing it here as well would have marked that + * poll's own fresh run as an interrupted leftover. + */ export async function startServer(opts: ServeOptions): Promise { - const cleared = await reconcileRunning(); - if (cleared > 0) { - console.log( - `Cleared ${cleared} AI run(s) left in flight by a previous process — start them again from the cockpit.`, - ); - } const app = await buildApp(opts); serve({ fetch: app.fetch, port: opts.port, hostname: opts.host }, (info) => { const tokenHint = opts.token ? `/?token=${opts.token}` : "";