feat(history): every review keeps a record of what happened to it by jtomaszewski · Pull Request #42 · fullstackhouse/cerber

jtomaszewski · 2026-08-25T07:10:35Z

The problem

A PR was missing from the inbox, and answering why took an hour of reconstruction. The artifact said skipped — but not when, not from where, and not whether the author's later re-request had been seen and deliberately ignored. The best the evidence could give was a five-hour window, bounded by an unrelated log file that happened to prove the poll had been alive, plus an inference that only one button in the whole codebase can write that value.

The cause is small and structural: a review keeps a single "last updated" time, so every write erases the answer to when did this become skipped, and did anything ask for it again afterwards?

The poll's silences are worse than that, because they leave nothing at all. It looks at a settled row, decides on purpose not to reopen it, and moves on. From the outside that is indistinguishable from a poll that never looked — which is exactly the shape of the question people ask about the inbox.

The fix

Every review now keeps a history: status changes, pushes it saw, each run and what it could read and cost, verdicts, comment churn, sends, filings and refreshes — each stamped with which part of cerber did it and what it was doing at the time. It sits at the foot of the review in the cockpit, and cerber history <pr> prints it.

It is appended by saveArtifact itself, never by its callers. Around twenty places write artifacts, and several of them overwrite one wholesale from a copy built minutes earlier; a log any of them had to remember to carry would be lost by the first that didn't. So a history handed in is ignored — what is on disk is the only copy — and a write path added later is recorded without knowing history exists.

The silences go in too. When the poll leaves a row alone — you settled it, so a push doesn't reopen it; the head it read is still the head; someone answered you; GitHub still lists you as a requested reviewer though its own search has stopped saying so — it writes that down. The last of those is the one fact nobody can reconstruct afterwards: what the awaiting search said at that minute, and that the PR itself disagreed with it.

Two things are deliberately left out. GitHub's timeline — pushes, requests, reviews — is GitHub's own record and gh can be asked for it again; mirroring it locally is a database wearing a different hat. And the chat, which already carries its own turns, timestamps and edits.

Reconstructed, the case that started this now reads:

Aug 24 11:07  daemon   appeared in the inbox — GitHub is asking you for a review        ·  poll
Aug 24 11:10  runner   review started (haiku, reading the source, asked for by the poll) ·  review
Aug 24 11:17  runner   review finished at 306658c (≈$6.80 at API rates)                 ·  review
Aug 24 11:23  cockpit  status ready → skipped                     ·  PATCH /api/reviews/…
Aug 24 14:45  daemon   left alone: you marked it skipped, so a new push or review request does not reopen it  ·  poll
Aug 24 14:45  daemon   gone from the awaiting search, but GitHub still lists you as a requested reviewer — left alone  ·  poll

Details

src/core/history.ts (new) — three parts. describeChange is a watchlist, not a deep diff: a generic comparison would bury the timeline under a running turn's narration, which is rewritten to the artifact every couple of seconds and says nothing about where the review got to. withWriter is an ambient AsyncLocalStorage context set once per entry point, so writes are labelled daemon / cockpit / cli / runner without a parameter on twenty call sites — and nesting means a run started by a cockpit click labels itself as the run. appendHistory ignores the caller's copy and caps a pathological row at 500 entries, saying where older ones were dropped.
src/core/state.ts — saveArtifact re-reads the file on every save, even when its caller has just read it: two writers share these files, so appending to a caller's copy would drop whatever the other recorded in between. It reads defensively — a hand-edited file that no longer parses is still overwritten, as before, but the timeline says so rather than quietly claiming the review began at that moment. noteHistory records a decision that changed nothing: it leaves updatedAt alone, because a note is not a change to the review and must not reorder the queue, and it writes nothing at all when the note repeats the last entry.
src/runner/review.ts, src/server/daemon.ts — the four decision notes. reviewPr is split at the guards: runReview decides whether to run in its caller's context, performReview runs as the runner. That boundary is the notes' whole value — a review that did not happen has no runner to blame, and "who wanted one" is the fact worth keeping.
src/server/index.ts — one middleware labels every route at once, including routes added later.
src/core/artifact.ts — history, optional: nothing outside saveArtifact writes it.
web/src/Detail.tsx, styles.css — a collapsed card at the foot of the review, newest first, with a rail jump that opens it on the way. src/cli/index.ts — cerber history <pr>.
docs/lifecycle.md — §6 gets the reference; §4 and §5 note where the silences are written; §8 gets the question that started this.

Verification

pnpm typecheck, pnpm test (498 passed / 32 files) and pnpm build all clean, on the branch with main merged in.

Beyond the unit tests, the whole path was driven end to end against a temporary CERBER_HOME: poll discovers → runner drafts → skip through the real PATCH route on a running server → poll looks twice and stands down → cockpit refresh onto a new head, then read back both in the browser and through cerber history. That is where the transcript above comes from.

src/core/history.test.ts (new) — the derivation case by case, including the two omissions: a turn's narration produces no entry, and a caller's own history is ignored in favour of disk.
src/core/state.test.ts — the persistence invariants: the log survives a write that overwrites the artifact wholesale from a stale copy, an entry another writer lands between a read and its write is kept (the race driven deterministically, and confirmed to fail without the fix), the writer is recorded, a repeated note is neither written twice nor rewritten to disk at all (asserted on mtime), updatedAt is untouched by a note, a note still lands when the log is already at its cap, and a broken file on disk says so.
src/runner/review.test.ts — the two guards that stop a re-review now say why, once; the freshness note carries the sha so it speaks again when the head moves; and a guarded skip during a poll is credited to daemon · poll rather than to a run that never happened.

Not covered: the cockpit card has no test — this repository has no React test harness, and the card was verified in a browser instead.

Breaking changes

None. history is optional, so artifacts written before it load unchanged — and absent means absent rather than an empty log, so the cockpit and the CLI can say the review predates the history being kept instead of implying nothing has ever happened to it.

Merged main (a94950e). #43 landed there meanwhile — the other half of this story, and it touches the same files. It also made one of the notes here false: "a new push or review request does not reopen it" was true when written and is not now, so the note claims only the push half. And since a reopen is a new way for a row to move, and this branch exists to explain why rows move, reopenIfAskedAgain now writes down why the row came back — a status change alone cannot say that somebody asked again after you settled it. Covered by a test in src/server/daemon.test.ts.

Review feedback addressed over three rounds, each fix confirmed to fail its test when reverted. dedaf09: saveArtifact no longer accepts a caller-supplied prior artifact (it undercut the from-disk guarantee when two writers share a file), and the dead .default([]) on history is gone. a9cd8d3: noteHistory decided whether to write by comparing the log's length, which at the cap is equal either way — a review that reached MAX_ENTRIES would have stopped recording poll decisions permanently. c313e50: the runner's writer context covered the guards too, so the notes about reviews that never ran were credited to the runner instead of to whoever wanted one — the split now happens at the guards. All threads replied to and resolved; the three suppressed comments answered in the conversation.

An artifact carries one `updatedAt`, so every write erased the answer to "when did I skip this, and did they ask again afterwards?". Answering it meant reasoning about which code path could have written a value, and bounding the time from an unrelated log file. Each review now keeps a history: status changes, pushes, runs and what they cost, verdicts, comment churn, sends, filings and refreshes — each stamped with which part of cerber did it and what it was doing at the time. It is at the foot of every review in the cockpit, and in a new `cerber history <pr>`. It is appended by `saveArtifact` itself rather than by its callers. Around twenty places write artifacts and several overwrite one wholesale from a copy built minutes earlier; a log any of them had to remember to carry would be lost by the first that didn't. So a history handed in is ignored — disk is the only copy — and a write path added later is recorded without knowing history exists. The poll's silences go in too. When it looks at a row and deliberately does nothing — you settled it, so a push doesn't reopen it; the head it read is still the head; someone answered you; GitHub still lists you as a requested reviewer though its search has stopped saying so — it writes that down. That last one is the only fact nobody can reconstruct later: what the search said at that minute, and that the PR disagreed. A decision re-taken every poll is recorded once and costs no write, and a note leaves `updatedAt` alone so it cannot reorder the queue. GitHub's own timeline is deliberately not copied — `gh` can be asked for it again. The chat is left out for the same reason: it already carries its own turns and edits. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Copilot

Pull request overview

Adds an append-only per-review history log to artifacts so Cerber can explain when and why a review changed state (including “poll silences”), and exposes it in both the cockpit UI and a new cerber history <pr> CLI command.

Changes:

Introduces src/core/history.ts and wires saveArtifact to append history entries automatically on every write.
Adds writer attribution (daemon / cockpit / cli / runner) via withWriter, and records explicit “did nothing” notes via noteHistory.
Surfaces history in the cockpit (collapsed card) and via cerber history, plus docs/README updates.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
web/src/types.ts	Extends the web `Artifact` type with optional `history` and defines `HistoryEntry`.
web/src/styles.css	Adds layout/styles for the history card rows and columns.
web/src/Icon.tsx	Adds an `up` icon for the history card toggle.
web/src/Detail.tsx	Adds `HistoryCard` UI and a rail jump button to open/scroll to history.
src/server/index.ts	Adds request-scoped writer attribution middleware for all routes.
src/server/daemon.ts	Records “poll did nothing” notes and wraps poll execution in a daemon writer context.
src/runner/review.ts	Wraps runs in a runner writer context and records “guarded skip” notes to history.
src/runner/review.test.ts	Adds tests asserting the runner’s “did nothing” notes are recorded once.
src/core/state.ts	Updates `saveArtifact` to append history and adds `noteHistory`; passes `prior` in some callers.
src/core/state.test.ts	Adds persistence/invariant tests for history appending, attribution, dedupe, and unreadable disk state.
src/core/history.ts	New: history entry schema, writer context, watchlist-based change descriptions, and capping logic.
src/core/history.test.ts	New: tests for change descriptions, writer attribution, dedupe, unreadable restart, and capping.
src/core/artifact.ts	Adds `history` to the artifact schema using `HistoryEntrySchema`.
src/cli/index.ts	Adds `cerber history` command and stamps CLI writes via a top-level writer context.
README.md	Documents the new per-review history feature and the CLI command.
docs/lifecycle.md	Documents history semantics, writer attribution, and poll “silence” notes.
CLAUDE.md	Updates architecture notes to mention the new history system.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

… default Two things the review caught. `saveArtifact` could be handed the artifact its caller had just read, to save reading the file twice. But two writers share these files — the poll's timer and the cockpit's button — so a caller's copy can be out of date by the time it writes, and appending to that copy drops whatever the other writer recorded in between. That is the one guarantee this whole design rests on, so the read is no longer skippable. The rest of the artifact is lost in that race either way; the history need not be. A test drives the race deterministically and fails without the fix. And `history` was declared `.default([]).optional()`, where the default never applies — zod's optional short-circuits before it. The behaviour was the intended one, but the schema claimed something it did not do. Absent now means absent, which is what the cockpit and the CLI already say about an artifact from before the history was kept. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

…'s length `noteHistory` skipped the write when appending would have changed nothing, and worked out "nothing" by comparing the log's length before and after. At the cap that comparison is always equal — an appended entry trims an older one — so a review that reached 500 entries would silently stop recording poll decisions from then on, permanently. The rule for "this note repeats the last thing said" belongs to appendHistory, so it is now asked directly (`noteIsRepeat`) rather than inferred from a quantity the capping deliberately holds constant. One rule, one place, and no derived signal in between. A test seeds a log at the cap and fails without the fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

web/src/Detail.tsx:1196

HistoryCard casts anchorRef to React.RefObject<HTMLElement> just to pass it to ref. In this same file, other components use the same React.RefObject<HTMLElement | null> shape and pass it directly (e.g. ref={anchorRef}), so this cast is unnecessary and makes the types harder to trust.

  entries: HistoryEntry[];
  open: boolean;
  onToggle: () => void;
  anchorRef: React.RefObject<HTMLElement | null>;
}) {
  const stamp = (at: string) => {
    const d = new Date(at);
    return Number.isNaN(d.getTime())
      ? at
      : `${d.toLocaleDateString(undefined, { month: "short", day: "2-digit" })} ${d.toLocaleTimeString(undefined, { hour: "2-digit", minute: "2-digit" })}`;
  };

  return (
    <section className="card" ref={anchorRef as React.RefObject<HTMLElement>}>
      <header className="card-head">

src/runner/review.ts:86

Wrapping all of runReview in withWriter({ by: "runner", cause: "review" }) means even the early-exit paths (e.g. the poll/CLI deciding to do nothing and calling noteHistory in runReview) will be stamped as runner · review. That loses the key debugging signal for the “poll silences” notes (whether it was the poll vs a CLI/manual invocation), and contradicts the intent of recording those silences with their true cause.

    // The run owns its writes, whoever asked for it: a re-review started from
    // a cockpit click is still the runner rewriting the draft.
    return await withWriter({ by: "runner", cause: "review" }, () => runReview(ref, opts));
  } finally {

web/src/Detail.tsx:1222

Using the array index in the key (key={${e.at}-${i}}) will cause every row to get a new key (and be re-mounted) whenever a new history entry is appended, because the reversed indices shift. A stable key avoids unnecessary DOM churn for up to 500 entries.

              {[...entries].reverse().map((e, i) => (
                <li key={`${e.at}-${i}`} className="history-row" title={e.at}>

The runner claimed the writer context for the whole of `reviewPr`, including the guards that decide not to run at all. So the notes those guards write — the poll's most confusing silences — were stamped `runner · review`, naming the one party that did nothing: no AI ran, and what the note is *for* is recording who was asking. The poll's timer and a person at a terminal became indistinguishable, which is the entire signal. The two halves are now split at the guards. `runReview` decides, in whatever context its caller set; `performReview` runs, as the runner. A test drives a poll's guarded skip and asserts the note carries `daemon · poll`; it fails with the old wrapping. Also from the review: `HistoryCard` cast its ref for no reason, where the panel beside it passes the same type straight through, and the row key used the reversed index — so appending one entry re-keyed every row and re-mounted up to 500 of them. The key is now the entry's position in append order, which an append does not move. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

jtomaszewski · 2026-08-25T07:35:00Z

The last review reported no new comments but suppressed three, in code that had not changed since the round before. Read them anyway — one was the most valuable finding on this PR.

src/runner/review.ts:86 — the writer context swallowed the notes' true cause. Correct, and it defeated the point of the feature. reviewPr claimed the runner context for the whole call, including the guards that decide not to run — so those notes were stamped runner · review, naming the one party that did nothing. A review that never happened has no runner to blame; what the note is for is recording who was asking, and the poll's timer and a person at a terminal had become indistinguishable.

Worth saying how it survived my own verification: the end-to-end run in the PR description called noteHistory directly under a daemon writer, so it printed daemon · poll and never exercised the real path through reviewPr. The test I wrote asserted the note's text and not its author. Both gaps are closed in c313e50 — the halves are split at the guards (runReview decides in the caller's context, performReview runs as the runner), and a test drives a poll's guarded skip and asserts daemon · poll, confirmed to fail against the old wrapping.

web/src/Detail.tsx:1196 — unnecessary ref cast. Right: ChatPanel takes the identical React.RefObject<HTMLElement | null> and passes it straight to ref a few hundred lines up. Removed.

web/src/Detail.tsx:1222 — index key over a reversed array. Also right — appending one entry shifts every reversed index, re-keying and re-mounting up to 500 rows. The key is now the entry's position in append order, which an append does not move.

pnpm typecheck / test (476) / build green, and the card was re-checked in a browser after the JSX change.

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

#43 shipped the other half of this story: a review you skipped comes back when someone asks again. It touches the same files, and it makes one of the notes here false — "a new push or review request does not reopen it" was true when written and is not now. Only the push half still holds, so that is all the note claims. The reopen is also a new way for a row to move, and this branch exists to explain why rows move. A status change says one moved; it cannot say that somebody asked again after you settled it, which is the whole reason a settle is allowed to be undone. So `reopenIfAskedAgain` writes that down, with a test. Also `settledAt: null` in the history test's artifact factory, and both sides of the lifecycle §5 conflict kept — they were separate additions at the same anchor, not competing ones. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

It is a disclosure control, so `aria-expanded` is what carries that to a screen reader — the label alone leaves it to be inferred from the word "show". Matches the bell's `aria-pressed` a couple of files over. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

github-actions · 2026-08-25T09:19:00Z

🎉 This PR is included in version 0.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

The full service specification — schemas, lifecycle, tool policy, the send path — precise enough to reimplement cerber from, written against the code and tests (which win on disagreement; known divergences are recorded in its Appendix B rather than silently resolved). It already describes the review history (#42) and the settled-row reopen rule (#43), so it should land after them. README and CLAUDE.md point at it, CLAUDE.md with the keep-it-true rule that stops it rotting. Co-authored-by: Claude Fable 5 <[email protected]>

jtomaszewski requested a lite review from Copilot August 25, 2026 07:10

Copilot started reviewing on behalf of jtomaszewski August 25, 2026 07:11 View session