fix(runner): stop a review going stale after you open it, and close three smaller holes

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

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`.

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,

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<typeof RunInfoSchema>;