From b8dc1539fb3b26378005c63010f1ff71fe39d773 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 08:57:53 +0200 Subject: [PATCH 1/5] feat(history): every review keeps a record of what happened to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `. 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) --- CLAUDE.md | 6 +- README.md | 26 +++- docs/lifecycle.md | 58 ++++++++- src/cli/index.ts | 47 ++++++- src/core/artifact.ts | 9 ++ src/core/history.test.ts | 263 ++++++++++++++++++++++++++++++++++++++ src/core/history.ts | 238 ++++++++++++++++++++++++++++++++++ src/core/state.test.ts | 72 ++++++++++- src/core/state.ts | 108 +++++++++++++--- src/runner/review.test.ts | 38 +++++- src/runner/review.ts | 16 ++- src/server/daemon.ts | 23 +++- src/server/index.ts | 9 ++ web/src/Detail.tsx | 89 +++++++++++++ web/src/Icon.tsx | 1 + web/src/styles.css | 49 +++++++ web/src/types.ts | 16 +++ 17 files changed, 1041 insertions(+), 27 deletions(-) create mode 100644 src/core/history.test.ts create mode 100644 src/core/history.ts diff --git a/CLAUDE.md b/CLAUDE.md index 738a75b..f1f57d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,7 +75,11 @@ any of them — it names the files each rule lives in. (`checkout.ts` — shallow `refs/pull/N/head` clone per PR under `~/.cerber/src`; an LRU cache of 8, evicted as reviews run, reclaimable with `cerber prune`), trust rules (`trust.ts` — `@login`, `@org/team`, `@org/*`; people only, no - way to trust a repo; denials win) and settings + way to trust a repo; denials win), the per-review history + (`history.ts` — appended by `saveArtifact` itself, never by its callers, so + no write path can forget it; a watchlist of what changed, who wrote it from + an ambient `withWriter`, and `noteHistory` for the decisions that changed + nothing) and settings (`config.ts` — `~/.cerber/config.json`, zod-validated, written by the CLI and the cockpit's settings screen) - `src/runner/` — review prompt + headless `claude -p --output-format diff --git a/README.md b/README.md index a64016e..9a7fe3b 100644 --- a/README.md +++ b/README.md @@ -355,12 +355,36 @@ the summary, comments and verdict alone, and is still one deliberate click. A review that has already been sent can't be argued with: that artifact is the record of what GitHub has. +### Every row remembers what happened to it + +A review keeps one "last updated" time, which means every write erases the +answer to *when did I skip this, and did they ask again afterwards?* So each +one also keeps a history: the status changes with their timestamps, every push +it saw, each run and what it cost, sends, refreshes — and which part of cerber +did it, whether that was you in the cockpit, the CLI, an AI run or the poll. + +The poll's silences are in there too. When it looks at a row and deliberately +leaves it alone — you settled it, so a new push does not reopen it; or GitHub +still lists you as a requested reviewer even though its own search has stopped +saying so — it writes that down instead of passing without a trace. That is +usually the answer when a PR is not where you expected it to be. + +It's at the foot of every review in the cockpit, and: + +```bash +cerber history owner/repo#123 +``` + +Nothing about GitHub's own timeline is copied here — GitHub keeps that, and +`gh` can be asked for it again. This is cerber's side of the story. + ## Status Early, but whole: everything described above has shipped — reviewing, editing and the gated Send, inbox discovery with parallel runs, confidence calibration (`cerber stats`), shadow-mode and opt-in auto-send, re-anchoring -onto new commits, source-backed and trusted runs, and the reviewer chat. +onto new commits, source-backed and trusted runs, the reviewer chat, and a +per-review history of everything that touched it (`cerber history`). `cerber export` writes a review out as markdown if you want it elsewhere. ## Running on a VPS diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 5f6ec78..1a2e4d5 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -28,10 +28,12 @@ Four things write the artifact, and most questions in this document are really | **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` | -| **you** | the CLI → `src/cli/index.ts` | `review` (`--force` re-reviews) and `send`. That is all it writes — `export` only renders, `prune` only clears checkouts, and there is no edit, mark or chat | +| **you** | the CLI → `src/cli/index.ts` | `review` (`--force` re-reviews) and `send`. That is all it writes — `export` only renders, `history` only reads, `prune` only clears checkouts, and there is no edit, mark or chat | There is no database and no migration step. The file is hand-editable; readers -are defensive and writers are atomic (tmp+rename, `src/core/state.ts`). +are defensive and writers are atomic (tmp+rename, `src/core/state.ts`). Which +of the four moved a given row, and when, is on the artifact itself: every write +appends to its `history` (§6). --- @@ -156,6 +158,10 @@ a token it checks the artifact already on disk: and why it is not `pr.headSha`, is the paragraph below.) 4. Otherwise → run. +Steps 2 and 3 write a note to the review's history (§6) saying so — a poll that +looks at a row and deliberately does nothing is otherwise indistinguishable +from one that never looked, which is the hardest thing about it to debug. + So a `ready` **or `sent`** artifact on a PR that gets a new commit is meant to be re-drafted by the next poll: `HEAD_SENSITIVE` only skips while the head is *unchanged*. For a sent one that is the point — submitting cleared GitHub's @@ -280,6 +286,12 @@ 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. +The two cases that leave a row alone — someone has answered you, and GitHub +still lists you as a requested reviewer despite the search — write a note to +the review's history (§6) rather than passing in silence. The second is the one +fact nobody can reconstruct afterwards: what the awaiting search said at that +minute, and that the PR itself disagreed with it. + ### "Whose move is it" Independent of status. Each poll reads the PR conversation per awaiting PR @@ -336,6 +348,42 @@ Written by other paths: both answer `202` and put their state on the artifact for the cockpit to poll — failures included, since there is no response left to hand them to. +### The history: the record one `updatedAt` cannot keep + +An artifact carries a single `updatedAt`, so every write erases the answer to +"when did this become `skipped`, and did anything ask for it again afterwards?". +`history` is the answer that survives — an append-only list on the artifact, +oldest first, read in the cockpit's **history** card and with `cerber history +`. + +It is written by `saveArtifact` itself (`src/core/state.ts`), never by its +callers: several write paths hand over an artifact built minutes earlier, and 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 new write path is recorded without knowing history exists. + +Three things go in, and two deliberately don't (`src/core/history.ts`): + +- **What changed**, from a watchlist: status, head sha, PR state and draftness, + a run starting/finishing/failing and what it could read, the verdict, + comment churn, send, filing, refresh. A watchlist rather than a deep diff, + or a running turn's narration — rewritten every couple of seconds — would + bury everything else. +- **Who did it**: `daemon`, `cockpit`, `cli`, `runner`, with the request, poll + or run that caused it. Set once at each entry point (`withWriter`), ambient + from there down. +- **What the poll decided *not* to do** — the notes in §4 and §5 below, written + with `noteHistory`. A decision re-taken every poll is recorded once, and a + note does not touch `updatedAt`: it is not a change to the review and must + not reorder the queue. +- **Not** GitHub's timeline. Pushes, requests and reviews are GitHub's own + record and `gh` can be asked for them again; the exception is what the + awaiting *search* said at a given minute, which cannot be asked for later. +- **Not** the chat, which already carries its own turns, timestamps and edits. + +The most recent 500 entries are kept, with a marker where older ones were +dropped. Deleting a stub deletes its history with it; nothing else removes one. + --- ## 7. Settings that change any of this @@ -400,6 +448,12 @@ 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. +**"When did I skip this — and did they ask again after?"** — `cerber history +`, or the **history** card at the foot of the review. It carries the status +change with its timestamp and who made it, every push it saw, and the poll's +own notes for the times it looked at the row and deliberately left it alone. +Empty on reviews that predate it being kept. + **"Why does it say reviewed when I never touched it?"** — The poll filed it; `filed.reason` says which of the three cases. The draft is untouched and still sendable. diff --git a/src/cli/index.ts b/src/cli/index.ts index 38eb6e8..d6d911a 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -5,6 +5,7 @@ import { artifactId, artifactKey } from "../core/artifact.js"; import { listCheckouts, removeCheckout } from "../core/checkout.js"; import { toMarkdown } from "../core/export.js"; import { configPath, loadConfig, saveConfig } from "../core/config.js"; +import { withWriter } from "../core/history.js"; import { TrustRuleError, describeRule, explainRule, parseTrustRule } from "../core/trust.js"; import { PrRef, parsePrRef, searchAwaitingMe, submitReview } from "../core/gh.js"; import { ReviewEvent, buildReviewPayload, computeCalibration, eventForRecommendation } from "../core/send.js"; @@ -139,6 +140,47 @@ program } }); +program + .command("history") + .description( + "Everything that has happened to a review: what changed, when, which part of cerber did it — and the decisions the poll took to leave it alone", + ) + .argument("", "PR URL, owner/repo#number, or number (with --repo)") + .option("-R, --repo ", "repository for bare PR numbers") + .action(async (input: string, opts: { repo?: string }) => { + const ref = parsePrRef(input, opts.repo); + const artifact = await loadArtifact(artifactId(ref)); + if (!artifact) { + console.error(`No review found for ${artifactId(ref)}. Run: cerber review ${input}`); + process.exit(1); + } + console.log(`${artifact.id} — ${artifact.pr.title}\n`); + const history = artifact.history ?? []; + if (history.length === 0) { + console.log( + "Nothing recorded. This review predates cerber keeping a history — it starts at the next thing that happens to it.", + ); + return; + } + 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" })}`; + }; + // The stamp's width is the locale's business, not ours — measure it. + const stampWidth = Math.max(...history.map((e) => stamp(e.at).length)) + 2; + for (const entry of history) { + // The cause names the path that made the change — which endpoint, which + // command — and is the whole point on a row two paths could have written. + console.log( + `${stamp(entry.at).padEnd(stampWidth)}${entry.by.padEnd(9)}${entry.what}` + + `${entry.cause ? ` · ${entry.cause}` : ""}`, + ); + } + console.log(`\n(times are local · ${history.length} entr${history.length === 1 ? "y" : "ies"})`); + }); + program .command("export") .description("Print a review as markdown (never touches GitHub)") @@ -513,7 +555,10 @@ function collect(value: string, previous: string[]): string[] { return [...previous, value]; } -program.parseAsync().catch((err) => { +// Every artifact write under this command is stamped with the command that +// made it. Nested contexts win, so `serve` labels its requests, its poll and +// its runs for themselves rather than all of them "cli". +withWriter({ by: "cli", cause: process.argv[2] ?? null }, () => program.parseAsync()).catch((err) => { console.error(err instanceof Error ? err.message : err); process.exit(1); }); diff --git a/src/core/artifact.ts b/src/core/artifact.ts index fb36d4f..8c4f448 100644 --- a/src/core/artifact.ts +++ b/src/core/artifact.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { HistoryEntrySchema } from "./history.js"; export const SCHEMA_VERSION = 1 as const; @@ -325,6 +326,14 @@ export const ArtifactSchema = z.object({ pendingChat: PendingChatSchema.nullable().default(null), /** The review as it stood before the first chat turn — "reset" restores this. */ preChat: ReviewSnapshotSchema.nullable().default(null), + /** + * Everything that has happened to this review, oldest first. + * + * Optional because nothing outside `saveArtifact` writes it: hand one in and + * it is ignored, since the log on disk is the only copy that is current. See + * `history.ts` for what is recorded and what is deliberately left out. + */ + history: z.array(HistoryEntrySchema).default([]).optional(), }); export type Artifact = z.infer; diff --git a/src/core/history.test.ts b/src/core/history.test.ts new file mode 100644 index 0000000..df4de48 --- /dev/null +++ b/src/core/history.test.ts @@ -0,0 +1,263 @@ +import { describe, expect, it } from "vitest"; +import { Artifact, Comment, SCHEMA_VERSION } from "./artifact.js"; +import { MAX_ENTRIES, appendHistory, describeChange, withWriter } from "./history.js"; + +function artifact(over: Partial = {}): Artifact { + return { + schemaVersion: SCHEMA_VERSION, + id: "acme/widgets#42", + status: "ready", + createdAt: "2026-08-19T00:00:00Z", + updatedAt: "2026-08-19T00:00:00Z", + pr: { + owner: "acme", + repo: "widgets", + number: 42, + title: "Add a thing", + url: "u", + author: "someone", + body: "", + baseRefName: "main", + headRefName: "f", + headSha: "306658c1111", + state: "OPEN", + isDraft: false, + additions: 1, + deletions: 0, + changedFiles: 1, + }, + diff: "diff", + summary: "s", + chapters: [], + comments: [], + verdict: null, + run: null, + sent: null, + refresh: null, + filed: null, + calibration: null, + chat: [], + preChat: null, + pendingChat: null, + ...over, + }; +} + +function comment(over: Partial = {}): Comment { + return { + id: "c1", + path: "src/a.ts", + line: 3, + body: "this is wrong", + chapterId: null, + severity: "blocker", + origin: "ai", + status: "draft", + editedByUser: false, + originalLine: null, + drifted: false, + ...over, + }; +} + +const run = (over: Partial> = {}) => ({ + model: "haiku", + startedAt: "2026-08-24T11:10:00Z", + finishedAt: null, + costUsd: null, + error: null, + withSource: true, + trusted: false, + sessionId: null, + trigger: "daemon" as const, + reviewedSha: null, + ...over, +}); + +describe("describeChange", () => { + it("says a row appeared in the inbox", () => { + expect(describeChange(null, artifact({ status: "awaiting" }))).toEqual([ + "appeared in the inbox — GitHub is asking you for a review", + ]); + }); + + it("records the status change that a single updatedAt would erase", () => { + const before = artifact({ status: "ready" }); + expect(describeChange(before, artifact({ status: "skipped" }))).toEqual([ + "status ready → skipped", + ]); + }); + + it("records a push under the review", () => { + const before = artifact(); + const after = artifact({ pr: { ...before.pr, headSha: "5502944aaaa" } }); + expect(describeChange(before, after)).toEqual(["head moved 306658c → 5502944"]); + }); + + it("says what a run could read and who asked for it", () => { + expect(describeChange(artifact(), artifact({ status: "running", run: run() }))).toEqual([ + "status ready → running", + "review started (haiku, reading the source, asked for by the poll)", + ]); + expect( + describeChange( + artifact(), + artifact({ status: "running", run: run({ withSource: false, trusted: true, trigger: "user" }) }), + )[1], + ).toBe("review started (haiku, diff only, trusted — may run commands, asked for by you)"); + }); + + it("records what a finished run cost and which commit it read", () => { + const started = artifact({ status: "running", run: run() }); + const finished = artifact({ + status: "ready", + run: run({ finishedAt: "2026-08-24T11:17:28Z", costUsd: 6.8, reviewedSha: "306658c1111" }), + verdict: { recommendation: "request_changes", confidence: 72, reasoning: "r" }, + comments: [comment(), comment({ id: "c2", origin: "user" })], + }); + expect(describeChange(started, finished)).toEqual([ + "status running → ready", + "review finished at 306658c (≈$6.80 at API rates)", + "verdict set to request changes (72% sure of the findings)", + "comments: +1 from the review, +1 you wrote", + ]); + }); + + it("records a failure rather than a finish", () => { + const started = artifact({ status: "running", run: run() }); + const failed = artifact({ + status: "failed", + run: run({ finishedAt: "2026-08-24T11:17:28Z", error: "claude exited 1" }), + }); + expect(describeChange(started, failed)).toEqual([ + "status running → failed", + "review failed: claude exited 1", + ]); + }); + + it("counts what happened to the comments", () => { + const before = artifact({ + comments: [comment(), comment({ id: "c2" }), comment({ id: "c3" })], + }); + const after = artifact({ + comments: [ + comment({ body: "reworded" }), + comment({ id: "c2", status: "dropped" }), + comment({ id: "c4", origin: "user", severity: null }), + ], + }); + expect(describeChange(before, after)).toEqual([ + "comments: +1 you wrote, 1 edited, 1 dropped, 1 gone", + ]); + }); + + it("records a send, a filing and a pull-forward", () => { + const before = artifact(); + expect( + describeChange( + before, + artifact({ + status: "sent", + sent: { at: "2026-08-25T09:00:00Z", event: "APPROVE", url: null, auto: true }, + }), + ), + ).toEqual(["status ready → sent", "sent to GitHub as approve, by auto-send"]); + + expect( + describeChange( + before, + artifact({ + filed: { at: "2026-08-25T09:00:00Z", reason: "request-withdrawn", review: null, reply: null }, + }), + ), + ).toEqual(["filed under settled — nobody is asking for this review any more"]); + + expect( + describeChange( + before, + artifact({ + refresh: { + at: "2026-08-25T08:26:00Z", + fromSha: "306658c", + toSha: "5502944", + moved: 1, + drifted: 1, + }, + }), + ), + ).toEqual(["pulled forward onto 5502944 — 1 comment(s) followed the code, 1 drifted"]); + }); + + it("ignores a running turn's narration — it says nothing about where the review got to", () => { + const before = artifact({ + pendingChat: { message: "why?", refs: [], startedAt: "t", progress: ["reading a.ts"], error: null }, + }); + const after = artifact({ + pendingChat: { + message: "why?", + refs: [], + startedAt: "t", + progress: ["reading a.ts", "searching for foo", "thinking"], + error: null, + }, + }); + expect(describeChange(before, after)).toEqual([]); + }); +}); + +describe("appendHistory", () => { + it("keeps what is on disk and ignores the copy the caller is holding", () => { + const prior = artifact({ + history: [{ at: "2026-08-24T11:07:00Z", by: "daemon", what: "appeared", cause: "poll" }], + }); + // The stale artifact a re-review built minutes ago: its own history is empty. + const stale = artifact({ status: "sent", history: [] }); + const history = appendHistory(prior, stale); + expect(history.map((e) => e.what)).toEqual(["appeared", "status ready → sent"]); + }); + + it("stamps each entry with whoever is writing", () => { + const entries = withWriter({ by: "cockpit", cause: "PATCH /api/reviews/x" }, () => + appendHistory(artifact(), artifact({ status: "skipped" })), + ); + expect(entries).toEqual([ + expect.objectContaining({ by: "cockpit", cause: "PATCH /api/reviews/x", what: "status ready → skipped" }), + ]); + }); + + it("attributes nothing when nothing claimed the write", () => { + expect(appendHistory(artifact(), artifact({ status: "skipped" }))[0]).toMatchObject({ + by: "unknown", + cause: null, + }); + }); + + it("records a decision that changed nothing, once", () => { + const note = "left alone: you marked it skipped"; + const first = appendHistory(artifact(), artifact(), { note }); + expect(first.map((e) => e.what)).toEqual([note]); + // The poll re-takes this decision every few minutes; saying so every time + // would bury everything else. + const again = appendHistory(artifact({ history: first }), artifact(), { note }); + expect(again).toEqual(first); + }); + + it("says so rather than claiming the review began, when the old file cannot be read", () => { + expect(appendHistory(null, artifact(), { unreadable: true }).map((e) => e.what)).toEqual([ + "history restarts here — the previous file could not be read", + ]); + }); + + it("caps a pathological row and admits what it dropped", () => { + const history = Array.from({ length: MAX_ENTRIES + 40 }, (_, i) => ({ + at: "2026-08-24T11:07:00Z", + by: "daemon" as const, + what: `entry ${i}`, + cause: null, + })); + const capped = appendHistory(artifact({ history }), artifact({ status: "skipped" })); + expect(capped).toHaveLength(MAX_ENTRIES); + expect(capped[0]?.what).toBe("… earlier entries dropped"); + expect(capped.at(-1)?.what).toBe("status ready → skipped"); + }); +}); diff --git a/src/core/history.ts b/src/core/history.ts new file mode 100644 index 0000000..33f1efd --- /dev/null +++ b/src/core/history.ts @@ -0,0 +1,238 @@ +import { AsyncLocalStorage } from "node:async_hooks"; +import { z } from "zod"; +import type { Artifact, Comment } from "./artifact.js"; + +/** + * What happened to a review, and when — cerber's side of the story. + * + * An artifact keeps one `updatedAt`, so every write erases the answer to "when + * did this become skipped?". This is the record that survives: one line per + * thing that changed, appended by `saveArtifact` itself rather than by its + * callers, so no write path can forget to keep it. + * + * It deliberately records only what cerber did and saw. GitHub keeps its own + * timeline of pushes, requests and reviews, and `gh` can always be asked for it + * again — mirroring it here would be a database wearing a different hat. The + * one thing worth writing down is what the poll *saw* at a given minute, since + * a search index cannot be asked what it said an hour ago. + * + * The chat is the other deliberate omission: a conversation already carries its + * own turns, timestamps and revisions. + */ +export const HistoryActorSchema = z.enum(["daemon", "cockpit", "cli", "runner", "unknown"]); +export type HistoryActor = z.infer; + +export const HistoryEntrySchema = z.object({ + at: z.string(), + /** Which part of cerber wrote it. "unknown" when nothing claimed the write. */ + by: HistoryActorSchema.default("unknown"), + /** What happened, in plain words: "status ready → skipped". */ + what: z.string(), + /** What was being done at the time: "PATCH /api/reviews/…", "poll", "review". */ + cause: z.string().nullable().default(null), +}); +export type HistoryEntry = z.infer; + +/** + * How many entries one review keeps. + * + * Entries are ~100 bytes next to a diff that is routinely a hundred times + * that, and a watchlist plus note de-duplication keeps a busy PR to a few + * dozen — so this is a backstop against a pathological row, not a budget. + */ +export const MAX_ENTRIES = 500; + +interface Writer { + by: HistoryActor; + cause: string | null; +} + +/** + * Who is writing right now. + * + * Ambient rather than a parameter on every write: there are twenty-odd call + * sites and the useful answer is the same for all the writes one request, poll + * or run makes. Set it once at each entry point — the HTTP middleware, the + * poll, the CLI, an AI run — and every artifact write underneath is labelled, + * including ones added later that never think about history at all. Nesting + * works the obvious way: a run started by a request labels its own writes. + */ +const writer = new AsyncLocalStorage(); + +export function withWriter(w: { by: HistoryActor; cause?: string | null }, fn: () => T): T { + return writer.run({ by: w.by, cause: w.cause ?? null }, fn); +} + +export function currentWriter(): Writer { + return writer.getStore() ?? { by: "unknown", cause: null }; +} + +const short = (sha: string) => (sha.length > 7 ? sha.slice(0, 7) : sha); + +const FILED_PHRASE: Record = { + "own-review": "you had already reviewed it on GitHub", + "own-reply": "you answered on the PR and nobody has answered back", + "request-withdrawn": "nobody is asking for this review any more", +}; + +/** The shape of a run, said once at the top of it: what it could read, who asked. */ +function runShape(run: NonNullable): string { + const parts = [run.model ?? "default model", run.withSource ? "reading the source" : "diff only"]; + if (run.trusted) parts.push("trusted — may run commands"); + if (run.trigger) parts.push(run.trigger === "daemon" ? "asked for by the poll" : "asked for by you"); + return parts.join(", "); +} + +/** Comment churn as one line: what a re-review, a chat turn or your own edits did. */ +function describeComments(before: Comment[], after: Comment[]): string | null { + const was = new Map(before.map((c) => [c.id, c])); + const is = new Map(after.map((c) => [c.id, c])); + let fromReview = 0; + let yours = 0; + let edited = 0; + let regraded = 0; + let dropped = 0; + let restored = 0; + for (const c of after) { + const old = was.get(c.id); + if (!old) { + if (c.origin === "user") yours++; + else fromReview++; + continue; + } + if (old.body !== c.body) edited++; + if (old.severity !== c.severity) regraded++; + if (old.status !== "dropped" && c.status === "dropped") dropped++; + if (old.status === "dropped" && c.status !== "dropped") restored++; + } + const gone = before.filter((c) => !is.has(c.id)).length; + + const parts: string[] = []; + if (fromReview) parts.push(`+${fromReview} from the review`); + if (yours) parts.push(`+${yours} you wrote`); + if (edited) parts.push(`${edited} edited`); + if (regraded) parts.push(`${regraded} re-graded`); + if (dropped) parts.push(`${dropped} dropped`); + if (restored) parts.push(`${restored} restored`); + if (gone) parts.push(`${gone} gone`); + return parts.length > 0 ? `comments: ${parts.join(", ")}` : null; +} + +/** + * What changed between two versions of a review, in plain words. + * + * 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. + */ +export function describeChange(before: Artifact | null, after: Artifact): string[] { + const lines: string[] = []; + + if (!before) { + lines.push( + after.status === "awaiting" + ? "appeared in the inbox — GitHub is asking you for a review" + : `first written here (${after.status})`, + ); + } else { + if (before.status !== after.status) lines.push(`status ${before.status} → ${after.status}`); + if (before.pr.headSha !== after.pr.headSha && after.pr.headSha) { + lines.push( + before.pr.headSha + ? `head moved ${short(before.pr.headSha)} → ${short(after.pr.headSha)}` + : `head is ${short(after.pr.headSha)}`, + ); + } + if (before.pr.state !== after.pr.state) { + lines.push(after.pr.state === "OPEN" ? "PR reopened" : `PR ${after.pr.state.toLowerCase()}`); + } + if (before.pr.isDraft !== after.pr.isDraft) { + lines.push(after.pr.isDraft ? "turned back into a draft" : "marked ready for review"); + } + } + + const wasRun = before?.run ?? null; + const run = after.run; + if (run && run.startedAt !== wasRun?.startedAt) lines.push(`review started (${runShape(run)})`); + if (run?.finishedAt && run.finishedAt !== wasRun?.finishedAt && !run.error) { + lines.push( + `review finished${run.reviewedSha ? ` at ${short(run.reviewedSha)}` : ""}` + + `${run.costUsd != null ? ` (≈$${run.costUsd.toFixed(2)} at API rates)` : ""}`, + ); + } + if (run?.error && run.error !== wasRun?.error) lines.push(`review failed: ${run.error}`); + + const wasVerdict = before?.verdict ?? null; + const verdict = after.verdict; + if ( + verdict && + (!wasVerdict || + wasVerdict.recommendation !== verdict.recommendation || + wasVerdict.confidence !== verdict.confidence) + ) { + lines.push( + `verdict ${wasVerdict ? "changed to" : "set to"} ${verdict.recommendation.replace("_", " ")}` + + ` (${verdict.confidence}% sure of the findings)`, + ); + } + + const comments = describeComments(before?.comments ?? [], after.comments); + if (comments) lines.push(comments); + + if (after.sent && after.sent.at !== before?.sent?.at) { + lines.push( + `sent to GitHub as ${after.sent.event.toLowerCase().replace("_", " ")}` + + `${after.sent.auto ? ", by auto-send" : ""}`, + ); + } + if (after.filed && after.filed.at !== before?.filed?.at) { + lines.push(`filed under settled — ${FILED_PHRASE[after.filed.reason] ?? after.filed.reason}`); + } + if (after.refresh && after.refresh.at !== before?.refresh?.at) { + const r = after.refresh; + lines.push( + `pulled forward onto ${short(r.toSha)} — ${r.moved} comment(s) followed the code` + + `${r.drifted > 0 ? `, ${r.drifted} drifted` : ""}`, + ); + } + + return lines; +} + +/** + * The history to write, given what is on disk and what is about to replace it. + * + * Whatever history the caller is holding is ignored: several write paths + * legitimately hand over an artifact built minutes ago, and their copy of the + * log is stale by definition. Disk is the only source. + */ +export function appendHistory( + prior: Artifact | null, + next: Artifact, + opts: { note?: string; unreadable?: boolean } = {}, +): HistoryEntry[] { + const kept = prior?.history ?? []; + const { by, cause } = currentWriter(); + const at = new Date().toISOString(); + + const what = opts.unreadable + ? ["history restarts here — the previous file could not be read"] + : opts.note + ? // A note explains a decision the poll re-takes every few minutes. Said + // once: repeating it until something else happens is noise, and the + // entry it would duplicate already says the same thing. + kept.at(-1)?.what === opts.note + ? [] + : [opts.note] + : describeChange(prior, next); + + const all = [...kept, ...what.map((w) => ({ at, by, cause, what: w }))]; + if (all.length <= MAX_ENTRIES) return all; + const [oldest, ...rest] = all.slice(-(MAX_ENTRIES - 1)); + if (!oldest) return all; + return [ + { at: oldest.at, by: "unknown" as const, cause: null, what: "… earlier entries dropped" }, + oldest, + ...rest, + ]; +} diff --git a/src/core/state.test.ts b/src/core/state.test.ts index 62cc30a..de52835 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -7,7 +7,9 @@ import { Artifact, SCHEMA_VERSION } from "./artifact.js"; const home = mkdtempSync(path.join(os.tmpdir(), "cerber-state-")); process.env.CERBER_HOME = home; -const { loadArtifact, reconcileRunning, saveArtifact } = await import("./state.js"); +const { loadArtifact, noteHistory, reconcileRunning, saveArtifact, updateArtifactByKey } = + await import("./state.js"); +const { withWriter } = await import("./history.js"); function artifact(over: Partial = {}): Artifact { return { @@ -129,3 +131,71 @@ describe("reconcileRunning", () => { expect((await loadArtifact("acme/widgets#42"))?.pendingChat?.error).toBe("boom"); }); }); + +describe("the history every write keeps", () => { + const id = "acme/widgets#42"; + const key = "acme__widgets__42"; + const whatHappened = async () => (await loadArtifact(id))?.history?.map((e) => e.what) ?? []; + + it("keeps the log across a write that overwrites the artifact wholesale", async () => { + await saveArtifact(artifact({ status: "awaiting" })); + // What a re-review does: hand over an artifact built minutes ago, whose own + // copy of the history is empty. Disk is the only source, so nothing is lost. + await saveArtifact(artifact({ status: "ready" })); + await saveArtifact(artifact({ status: "sent" })); + expect(await whatHappened()).toEqual([ + "appeared in the inbox — GitHub is asking you for a review", + "status awaiting → ready", + "status ready → sent", + ]); + }); + + it("ignores a history handed in by the caller", async () => { + await saveArtifact(artifact({ status: "ready" })); + await saveArtifact( + artifact({ + status: "skipped", + history: [{ at: "1999-01-01T00:00:00Z", by: "cli", what: "invented", cause: null }], + }), + ); + expect(await whatHappened()).toEqual(["first written here (ready)", "status ready → skipped"]); + }); + + it("names which part of cerber made the change", async () => { + await saveArtifact(artifact({ status: "ready" })); + await withWriter({ by: "cockpit", cause: "PATCH /api/reviews/x" }, () => + updateArtifactByKey(key, (a) => ({ ...a, status: "skipped" as const })), + ); + const entry = (await loadArtifact(id))?.history?.at(-1); + expect(entry).toMatchObject({ by: "cockpit", cause: "PATCH /api/reviews/x" }); + }); + + it("records a decision that changed nothing, without disturbing the queue's order", async () => { + await saveArtifact(artifact({ status: "skipped", updatedAt: "2026-08-19T00:00:00Z" })); + const note = "left alone: you marked it skipped, so a new push does not reopen it"; + await noteHistory(id, note); + await noteHistory(id, note); + + const after = await loadArtifact(id); + expect(after?.history?.map((e) => e.what)).toEqual(["first written here (skipped)", note]); + // A note is not a change to the review: it must not float the row to the + // top of a queue sorted by updatedAt. + expect(after?.updatedAt).toBe("2026-08-19T00:00:00Z"); + + // And the repeat cost nothing: a note with nothing to add doesn't rewrite + // an artifact that carries a whole diff. + const file = path.join(home, "reviews", `${key}.json`); + const written = (await fs.stat(file)).mtimeMs; + await noteHistory(id, note); + expect((await fs.stat(file)).mtimeMs).toBe(written); + }); + + it("says so rather than starting over quietly, when the file on disk is broken", async () => { + await saveArtifact(artifact({ status: "ready" })); + await fs.writeFile(path.join(home, "reviews", `${key}.json`), "{ not json"); + await saveArtifact(artifact({ status: "skipped" })); + expect(await whatHappened()).toEqual([ + "history restarts here — the previous file could not be read", + ]); + }); +}); diff --git a/src/core/state.ts b/src/core/state.ts index 8428b8b..8bdcab9 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -2,6 +2,7 @@ import { promises as fs } from "node:fs"; import os from "node:os"; import path from "node:path"; import { Artifact, ArtifactSchema, artifactKey } from "./artifact.js"; +import { appendHistory } from "./history.js"; export function cerberHome(): string { return process.env.CERBER_HOME ?? path.join(os.homedir(), ".cerber"); @@ -15,15 +16,89 @@ function artifactPath(id: string): string { return path.join(reviewsDir(), `${artifactKey(id)}.json`); } -export async function saveArtifact(artifact: Artifact): Promise { +/** + * Read whatever is on disk, tolerating anything. + * + * These files are the user's to edit, so a broken one has to be survivable: + * before, a save simply overwrote it. It still does — but the timeline says so + * rather than quietly claiming the review began at that moment. + */ +async function readPrior(file: string): Promise<{ artifact: Artifact | null; unreadable: boolean }> { + let raw: string; + try { + raw = await fs.readFile(file, "utf8"); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") return { artifact: null, unreadable: false }; + return { artifact: null, unreadable: true }; + } + try { + return { artifact: ArtifactSchema.parse(JSON.parse(raw)), unreadable: false }; + } catch { + return { artifact: null, unreadable: true }; + } +} + +/** + * Write an artifact, and record what changed about it. + * + * The history is appended here rather than by the caller on purpose. 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 one that didn't. Appending at the one place + * every write goes through makes the record a property of writing. + * + * `prior` is an optimisation for callers that have just read the file (it + * saves reading it twice, and an artifact carries a whole diff); `note` records + * a decision that changed nothing, which is the only kind of history the diff + * cannot see. Neither touches `updatedAt` — that is `updateArtifactByKey`'s. + */ +export async function saveArtifact( + artifact: Artifact, + opts: { prior?: Artifact | null; note?: string } = {}, +): Promise { await fs.mkdir(reviewsDir(), { recursive: true }); const file = artifactPath(artifact.id); + const prior = + opts.prior !== undefined + ? { artifact: opts.prior, unreadable: false } + : await readPrior(file); + const next: Artifact = { + ...artifact, + history: appendHistory(prior.artifact, artifact, { + note: opts.note, + unreadable: prior.unreadable, + }), + }; const tmp = `${file}.tmp`; - await fs.writeFile(tmp, JSON.stringify(artifact, null, 2)); + await fs.writeFile(tmp, JSON.stringify(next, null, 2)); await fs.rename(tmp, file); return file; } +/** + * Write down a decision that changed nothing. + * + * The poll's silences are the hardest thing to debug about it — it looks at a + * settled row, decides deliberately to leave it alone, and leaves no trace of + * having looked. This is that trace. It is not a change to the review, so + * `updatedAt` stays put: a note must not reorder the queue. + * + * Missing artifacts are ignored, and a note identical to the last entry is + * dropped, so a decision re-taken every poll is recorded once. + */ +export async function noteHistory(id: string, what: string): Promise { + const prior = await loadArtifact(id).catch(() => null); + if (!prior) return; + // `appendHistory` drops a note identical to the last entry — ask it, rather + // than knowing the rule twice. Nothing to add means nothing to write: the + // poll re-takes these decisions every few minutes, and rewriting a whole + // artifact, diff and all, to change nothing is the expensive half of saying + // it again. + const history = appendHistory(prior, prior, { note: what }); + if (history.length === (prior.history ?? []).length) return; + await saveArtifact(prior, { prior, note: what }); +} + export async function loadArtifact(id: string): Promise { try { const raw = await fs.readFile(artifactPath(id), "utf8"); @@ -101,7 +176,7 @@ export async function updateArtifactByKey( const artifact = await loadArtifactByKey(key); if (!artifact) return null; const updated = { ...mutate(artifact), updatedAt: new Date().toISOString() }; - await saveArtifact(updated); + await saveArtifact(updated, { prior: artifact }); return updated; } @@ -132,18 +207,21 @@ export async function reconcileRunning( const stuckChat = artifact.pendingChat && artifact.pendingChat.error == null ? artifact.pendingChat : null; if (!stuckRun && !stuckChat) continue; - await saveArtifact({ - ...artifact, - status: stuckRun ? "failed" : artifact.status, - updatedAt: new Date().toISOString(), - run: - stuckRun && artifact.run - ? { ...artifact.run, error: "interrupted — cerber restarted while this review was running" } - : artifact.run, - pendingChat: stuckChat - ? { ...stuckChat, error: "interrupted — cerber restarted while this turn was running" } - : artifact.pendingChat, - }); + await saveArtifact( + { + ...artifact, + status: stuckRun ? "failed" : artifact.status, + updatedAt: new Date().toISOString(), + run: + stuckRun && artifact.run + ? { ...artifact.run, error: "interrupted — cerber restarted while this review was running" } + : artifact.run, + pendingChat: stuckChat + ? { ...stuckChat, error: "interrupted — cerber restarted while this turn was running" } + : artifact.pendingChat, + }, + { prior: artifact }, + ); cleared++; } return cleared; diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index f274eb3..a3a753b 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -4,7 +4,7 @@ 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 { fetchPrDiff, fetchPrInfo } from "../core/gh.js"; -import { saveArtifact } from "../core/state.js"; +import { loadArtifact, saveArtifact } from "../core/state.js"; import { reviewPr } from "./review.js"; // Only the freshness gate is under test: whether a run happens at all. Every @@ -170,3 +170,39 @@ describe("what a new push does to a review", () => { expect(diff).toHaveBeenCalled(); }); }); + +describe("what the poll writes down when it decides to do nothing", () => { + const whatHappened = async () => (await loadArtifact("acme/widgets#7"))?.history?.map((e) => e.what) ?? []; + + it("explains a settled row that a push cannot reopen", async () => { + // The silence this records is the one that is impossible to debug from the + // outside: the author keeps pushing, the poll keeps looking, and the row + // never comes back into the inbox — with nothing anywhere saying why. + await saveArtifact(artifact("skipped", "old-sha")); + // The log is cumulative, and every case in this file writes to the same + // artifact — so only what this one adds is under test. + const before = (await whatHappened()).length; + prInfo.mockResolvedValue(pr("new-sha")); + + await reviewPr(REF); + // Re-taken every poll; said once. + await reviewPr(REF); + expect((await whatHappened()).slice(before)).toEqual([ + "left alone: you marked it skipped, so a new push or review request does not reopen it", + ]); + }); + + it("names the commit a draft was judged up to date against", async () => { + await saveArtifact({ + ...artifact("ready", "same-sha"), + run: { ...runBlock, reviewedSha: "same-sha" }, + }); + const before = (await whatHappened()).length; + prInfo.mockResolvedValue(pr("same-sha")); + + await reviewPr(REF); + expect((await whatHappened()).slice(before)).toEqual([ + "already reviewed at same-sh — not re-reviewed", + ]); + }); +}); diff --git a/src/runner/review.ts b/src/runner/review.ts index afceb0d..ee78bd6 100644 --- a/src/runner/review.ts +++ b/src/runner/review.ts @@ -11,7 +11,8 @@ import { import { createRunDir, evictOldCheckouts, prepareCheckout, removeRunDir } from "../core/checkout.js"; import { PrRef, fetchPrDiff, fetchPrInfo, isOrgMember, isTeamMember } from "../core/gh.js"; import { mergeRunResult, userOwnsStatus } from "../core/refresh.js"; -import { loadArtifact, saveArtifact, updateArtifactByKey } from "../core/state.js"; +import { loadArtifact, noteHistory, saveArtifact, updateArtifactByKey } from "../core/state.js"; +import { withWriter } from "../core/history.js"; import { loadConfig } from "../core/config.js"; import { decideTrust, membershipQueries, parseTrustRules } from "../core/trust.js"; import { ClaudeEvent, extractJson, runClaude, unauthenticatedEnv } from "./claude.js"; @@ -79,7 +80,9 @@ const SETTLED_BY_YOU = new Set(["reviewed", "skipped"]); export async function reviewPr(ref: PrRef, opts: ReviewOptions = {}): Promise { beginReview(artifactId(ref)); try { - return await runReview(ref, opts); + // 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 { endReview(artifactId(ref)); } @@ -135,6 +138,12 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise if (existing && !opts.force) { if (SETTLED_BY_YOU.has(existing.status)) { log(`You marked this ${existing.status} — leaving it alone. Use --force to re-review.`); + // Written down because it is the poll's most confusing silence: a row + // the author keeps pushing to, that never comes back into the inbox. + await noteHistory( + existing.id, + `left alone: you marked it ${existing.status}, so a new push or review request does not reopen it`, + ); return { artifact: existing, skipped: true }; } // The sha the AI *read*, not the one the artifact happens to mention. @@ -146,6 +155,9 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise 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.`); + // Carries the sha, so it says itself again the next time the head moves + // and this guard stops being the reason nothing happened. + await noteHistory(existing.id, `already reviewed at ${reviewedSha.slice(0, 7)} — not re-reviewed`); return { artifact: existing, skipped: true }; } } diff --git a/src/server/daemon.ts b/src/server/daemon.ts index 9293b49..c9f591b 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -15,6 +15,7 @@ import { stillRequested, submitReview, } from "../core/gh.js"; +import { withWriter } from "../core/history.js"; import { notice, notify } from "../core/notify.js"; import { buildReviewPayload, computeCalibration } from "../core/send.js"; import { @@ -22,6 +23,7 @@ import { deleteArtifact, listArtifacts, loadArtifact, + noteHistory, saveArtifact, updateArtifactByKey, } from "../core/state.js"; @@ -387,13 +389,24 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { } // Anything but silence from you leaves the row alone: `them` is somebody // answering a comment of yours, which is the opposite of settled. - if (classifyReply(conversation, me) !== "none") return; + if (classifyReply(conversation, me) !== "none") { + await noteHistory(artifact.id, "someone has answered you on the PR — the draft stays out for you"); + return; + } if (!filedByWithdrawnRequest(artifact)) return; // The awaiting search says nobody is asking, and here that is the entire // case — so confirm it against the PR itself before acting on it. The // other two reasons stand on a fact of their own and need no such check. - if (stillRequested(await fetchReviewRequests(artifact.pr), me)) return; + if (stillRequested(await fetchReviewRequests(artifact.pr), me)) { + // The one fact GitHub cannot be asked for later: what its search index + // said at this minute, and that the PR itself disagreed with it. + await noteHistory( + artifact.id, + "gone from the awaiting search, but GitHub still lists you as a requested reviewer — left alone", + ); + return; + } await file( { at: new Date().toISOString(), reason: "request-withdrawn", review: null, reply: null }, "nobody is asking for this review any more", @@ -550,7 +563,11 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { return { reviewed, skipped, failed }; } - async function poll(): Promise { + /** Everything a poll writes is the poll's — including the reviews it starts, + * which stamp themselves as runs from inside this. */ + const poll = () => withWriter({ by: "daemon", cause: "poll" }, runPoll); + + async function runPoll(): Promise { if (status.polling) return; status.polling = true; status.lastPollAt = new Date().toISOString(); diff --git a/src/server/index.ts b/src/server/index.ts index e3a63cc..1a73939 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -21,6 +21,7 @@ import { fetchPrDiff, fetchPrInfo, parsePrRef, submitReview } from "../core/gh.j import { z } from "zod"; import { DaemonConfigSchema, configPath, loadConfig, saveConfig } from "../core/config.js"; import { refreshArtifact } from "../core/refresh.js"; +import { withWriter } from "../core/history.js"; import { TrustRuleError, describeRule, explainRule, parseTrustRule } from "../core/trust.js"; import { ReviewEvent, buildReviewPayload, computeCalibration } from "../core/send.js"; import { @@ -72,6 +73,14 @@ export async function buildApp( ): Promise { const app = new Hono(); + // Whatever any route writes to an artifact is stamped with the request that + // caused it — once, here, rather than route by route, so a route added later + // is labelled without knowing that history exists. A detached run started by + // a request re-labels its own writes for itself. + app.use("*", (c, next) => + withWriter({ by: "cockpit", cause: `${c.req.method} ${new URL(c.req.url).pathname}` }, next), + ); + if (opts.token) { const token = opts.token; app.use("*", async (c, next) => { diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index ad6a080..f71c8ba 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -41,6 +41,7 @@ import { Chapter, ChatRef, ChatTurn, + HistoryEntry, RefreshResult, Revision, ReviewComment, @@ -1165,6 +1166,74 @@ function FiledNote({ filed }: { filed: NonNullable }) { ); } +/** + * Everything that has happened to this review. + * + * An artifact keeps one `updatedAt`, so without this the answer to "when did I + * skip this, and did anything ask for it again afterwards?" is gone the moment + * anything else touches the row. Collapsed by default: it is what you open when + * a review is not where you expected it, not part of reading one. + */ +function HistoryCard({ + entries, + open, + onToggle, + anchorRef, +}: { + entries: HistoryEntry[]; + open: boolean; + onToggle: () => void; + anchorRef: React.RefObject; +}) { + 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 ( +
}> +
+

history

+ + {entries.length > 0 + ? `${entries.length} entr${entries.length === 1 ? "y" : "ies"} — what cerber did to this row, and when` + : "nothing recorded"} + + + +
+ {open && ( +
+ {entries.length === 0 ? ( +
+ This review predates cerber keeping a history — it starts at the next thing that + happens to it. +
+ ) : ( +
    + {/* Newest first: the reason you opened this is almost always the + last thing that happened, or the last thing that didn't. */} + {[...entries].reverse().map((e, i) => ( +
  1. + {stamp(e.at)} + {e.by} + {e.what} + {e.cause && {e.cause}} +
    2. + ))} +
+ )} +
+ )} +
+ ); +} + function FreshnessBanner({ artifact, freshness, @@ -1249,6 +1318,8 @@ export function Detail({ reviewKey }: { reviewKey: string }) { const whyEl = useRef(null); const chatEl = useRef(null); const sendEl = useRef(null); + const historyEl = useRef(null); + const [historyOpen, setHistoryOpen] = useState(false); const topEl = useRef(null); const [eventOverride, setEventOverride] = useState(null); const [sending, setSending] = useState(false); @@ -1687,6 +1758,17 @@ export function Detail({ reviewKey }: { reviewKey: string }) { > chat + {/* Where you go when the review is not where you expected it — + asking for it is asking to read it, so it opens on the way. */} + {chapters.length > 0 && ( @@ -1859,6 +1941,13 @@ export function Detail({ reviewKey }: { reviewKey: string }) { )} + + setHistoryOpen((v) => !v)} + anchorRef={historyEl} + /> {/* The two things you do rather than read. On a wide screen they are a diff --git a/web/src/Icon.tsx b/web/src/Icon.tsx index b774718..04ee863 100644 --- a/web/src/Icon.tsx +++ b/web/src/Icon.tsx @@ -20,6 +20,7 @@ const PATHS: Record = { external: ["M14 4h6v6", "M20 4 10 14", "M18 14v5a1 1 0 0 1-1 1H5a1 1 0 0 1-1-1V7a1 1 0 0 1 1-1h5"], settings: ["M20 7h-9", "M14 17H5"], down: ["M12 5v13", "m6 12 6 6 6-6"], + up: ["M12 19V6", "m6 12 6-6 6 6"], bell: ["M18 9a6 6 0 1 0-12 0c0 5-2 6-2 6h16s-2-1-2-6", "M13.7 20a2 2 0 0 1-3.4 0"], bellOff: [ "M8.5 3.6A6 6 0 0 1 18 9c0 1.7.2 2.9.6 3.9", diff --git a/web/src/styles.css b/web/src/styles.css index be19fff..c6de226 100644 --- a/web/src/styles.css +++ b/web/src/styles.css @@ -1561,6 +1561,55 @@ button { margin-bottom: 14px; } +/* The history card: a dense log, read newest first. Columns rather than a + sentence, so a run of entries scans as one shape and the odd one out shows. */ +.history { + list-style: none; + margin: 0; + padding: 0; +} + +.history-row { + display: flex; + align-items: baseline; + gap: 10px; + padding: 3px 0; + border-bottom: 1px solid var(--line-3); + font-size: 11.5px; +} + +.history-row:last-child { + border-bottom: 0; +} + +.history-at { + color: var(--muted); + white-space: nowrap; + flex-shrink: 0; + min-width: 92px; +} + +.history-by { + color: var(--faint); + white-space: nowrap; + flex-shrink: 0; + min-width: 60px; +} + +.history-what { + color: var(--fg-2); +} + +/* What asked for the write. Wanted only when two paths could have made the + same change, so it sits out of the way at the end of the row. */ +.history-cause { + color: var(--fainter); + margin-left: auto; + padding-left: 12px; + white-space: nowrap; + flex-shrink: 0; +} + .chat-turn { border-left: 2px solid var(--line-2); padding: 2px 0 2px 12px; diff --git a/web/src/types.ts b/web/src/types.ts index 018a8f8..8e4557f 100644 --- a/web/src/types.ts +++ b/web/src/types.ts @@ -172,6 +172,22 @@ export interface Artifact { pendingChat?: PendingChat | null; /** Present once a chat has started — what "reset" goes back to. */ preChat?: { at: string } | null; + /** + * Everything that has happened to this review, oldest first. Absent on + * artifacts written before cerber kept one. + */ + history?: HistoryEntry[]; +} + +/** One thing that happened to a review — see src/core/history.ts. */ +export interface HistoryEntry { + at: string; + /** Which part of cerber did it. "unknown" when nothing claimed the write. */ + by: "daemon" | "cockpit" | "cli" | "runner" | "unknown"; + /** What happened, in plain words. */ + what: string; + /** What was being done at the time: the request, the poll, the run. */ + cause: string | null; } /** A chat turn in flight. Turns run detached; this is what the cockpit polls. */ From dedaf094c52574c13726d25ab322d8dc29944cdd Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:18:09 +0200 Subject: [PATCH 2/5] fix(history): source the log from disk on every save, and drop a dead default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/lifecycle.md | 6 +++++ src/core/artifact.ts | 10 ++++---- src/core/state.test.ts | 28 ++++++++++++++++++++++- src/core/state.ts | 52 +++++++++++++++++++++--------------------- 4 files changed, 65 insertions(+), 31 deletions(-) diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 1a2e4d5..2f0b173 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -362,6 +362,12 @@ 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 new write path is recorded without knowing history exists. +Which is why every save re-reads the file first, even when the caller has just +read it. Two writers share these files (§1), so a caller's copy can be out of +date by the time it writes, and appending to *that* would drop whatever the +other one recorded in between. The rest of the artifact is lost in that race +either way; the history need not be. + Three things go in, and two deliberately don't (`src/core/history.ts`): - **What changed**, from a watchlist: status, head sha, PR state and draftness, diff --git a/src/core/artifact.ts b/src/core/artifact.ts index 8c4f448..5417806 100644 --- a/src/core/artifact.ts +++ b/src/core/artifact.ts @@ -329,11 +329,13 @@ export const ArtifactSchema = z.object({ /** * Everything that has happened to this review, oldest first. * - * Optional because nothing outside `saveArtifact` writes it: hand one in and - * it is ignored, since the log on disk is the only copy that is current. See - * `history.ts` for what is recorded and what is deliberately left out. + * Optional, and with no default: nothing outside `saveArtifact` writes this, + * so absent means absent — an artifact from before it was kept, which the + * cockpit and the CLI say so about rather than showing as an empty history. + * Hand one in and it is ignored; the log on disk is the only current copy. + * See `history.ts` for what is recorded and what is deliberately left out. */ - history: z.array(HistoryEntrySchema).default([]).optional(), + history: z.array(HistoryEntrySchema).optional(), }); export type Artifact = z.infer; diff --git a/src/core/state.test.ts b/src/core/state.test.ts index de52835..61bcb3a 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, promises as fs } from "node:fs"; +import { mkdtempSync, promises as fs, readFileSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; import { beforeEach, describe, expect, it } from "vitest"; @@ -161,6 +161,32 @@ describe("the history every write keeps", () => { expect(await whatHappened()).toEqual(["first written here (ready)", "status ready → skipped"]); }); + it("keeps an entry another writer landed between the read and the write", async () => { + await saveArtifact(artifact({ status: "ready" })); + const file = path.join(home, "reviews", `${key}.json`); + + await updateArtifactByKey(key, (a) => { + // The poll lands a note in the window between the load this mutation was + // handed and the save that follows it — the race two writers on one file + // genuinely have. Appending to the copy in hand would drop it. + const theirs = JSON.parse(readFileSync(file, "utf8")); + theirs.history.push({ + at: "2026-08-24T14:45:00Z", + by: "daemon", + what: "left alone: you marked it skipped", + cause: "poll", + }); + writeFileSync(file, JSON.stringify(theirs)); + return { ...a, status: "skipped" as const }; + }); + + expect(await whatHappened()).toEqual([ + "first written here (ready)", + "left alone: you marked it skipped", + "status ready → skipped", + ]); + }); + it("names which part of cerber made the change", async () => { await saveArtifact(artifact({ status: "ready" })); await withWriter({ by: "cockpit", cause: "PATCH /api/reviews/x" }, () => diff --git a/src/core/state.ts b/src/core/state.ts index 8bdcab9..6022c2d 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -47,21 +47,24 @@ async function readPrior(file: string): Promise<{ artifact: Artifact | null; unr * carry would be lost by the first one that didn't. Appending at the one place * every write goes through makes the record a property of writing. * - * `prior` is an optimisation for callers that have just read the file (it - * saves reading it twice, and an artifact carries a whole diff); `note` records - * a decision that changed nothing, which is the only kind of history the diff - * cannot see. Neither touches `updatedAt` — that is `updateArtifactByKey`'s. + * The read here is not skippable, even for a caller that has just done one of + * its own. 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* would drop whatever the other one recorded in between. + * The rest of the artifact is lost in that race either way; the history need + * not be, and the extra read is one file next to a write of the same file. + * + * `note` records a decision that changed nothing, which is the only kind of + * history a diff cannot see. It does not touch `updatedAt` — that belongs to + * `updateArtifactByKey`. */ export async function saveArtifact( artifact: Artifact, - opts: { prior?: Artifact | null; note?: string } = {}, + opts: { note?: string } = {}, ): Promise { await fs.mkdir(reviewsDir(), { recursive: true }); const file = artifactPath(artifact.id); - const prior = - opts.prior !== undefined - ? { artifact: opts.prior, unreadable: false } - : await readPrior(file); + const prior = await readPrior(file); const next: Artifact = { ...artifact, history: appendHistory(prior.artifact, artifact, { @@ -96,7 +99,7 @@ export async function noteHistory(id: string, what: string): Promise { // it again. const history = appendHistory(prior, prior, { note: what }); if (history.length === (prior.history ?? []).length) return; - await saveArtifact(prior, { prior, note: what }); + await saveArtifact(prior, { note: what }); } export async function loadArtifact(id: string): Promise { @@ -176,7 +179,7 @@ export async function updateArtifactByKey( const artifact = await loadArtifactByKey(key); if (!artifact) return null; const updated = { ...mutate(artifact), updatedAt: new Date().toISOString() }; - await saveArtifact(updated, { prior: artifact }); + await saveArtifact(updated); return updated; } @@ -207,21 +210,18 @@ export async function reconcileRunning( const stuckChat = artifact.pendingChat && artifact.pendingChat.error == null ? artifact.pendingChat : null; if (!stuckRun && !stuckChat) continue; - await saveArtifact( - { - ...artifact, - status: stuckRun ? "failed" : artifact.status, - updatedAt: new Date().toISOString(), - run: - stuckRun && artifact.run - ? { ...artifact.run, error: "interrupted — cerber restarted while this review was running" } - : artifact.run, - pendingChat: stuckChat - ? { ...stuckChat, error: "interrupted — cerber restarted while this turn was running" } - : artifact.pendingChat, - }, - { prior: artifact }, - ); + await saveArtifact({ + ...artifact, + status: stuckRun ? "failed" : artifact.status, + updatedAt: new Date().toISOString(), + run: + stuckRun && artifact.run + ? { ...artifact.run, error: "interrupted — cerber restarted while this review was running" } + : artifact.run, + pendingChat: stuckChat + ? { ...stuckChat, error: "interrupted — cerber restarted while this turn was running" } + : artifact.pendingChat, + }); cleared++; } return cleared; From a9cd8d35a9d50fc70d459ed8482103a4fb286758 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:24:33 +0200 Subject: [PATCH 3/5] fix(history): ask whether a note repeats, don't infer it from the log's length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- src/core/history.ts | 16 +++++++++++++++- src/core/state.test.ts | 25 ++++++++++++++++++++++++- src/core/state.ts | 16 ++++++++-------- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/core/history.ts b/src/core/history.ts index 33f1efd..07e79b2 100644 --- a/src/core/history.ts +++ b/src/core/history.ts @@ -199,6 +199,20 @@ export function describeChange(before: Artifact | null, after: Artifact): string return lines; } +/** + * Would this note be dropped as a repeat of the last thing said? + * + * The rule belongs to `appendHistory`, which drops such a note; this is the + * same question asked ahead of time, so a caller can skip a write that would + * change nothing. Exported rather than inferred from the result: at the cap an + * appended entry trims an older one, so the log's *length* is not evidence of + * anything having been added, and a caller comparing lengths would silently + * stop recording notes the moment a review reached `MAX_ENTRIES`. + */ +export function noteIsRepeat(prior: Artifact | null, note: string): boolean { + return (prior?.history ?? []).at(-1)?.what === note; +} + /** * The history to write, given what is on disk and what is about to replace it. * @@ -221,7 +235,7 @@ export function appendHistory( ? // A note explains a decision the poll re-takes every few minutes. Said // once: repeating it until something else happens is noise, and the // entry it would duplicate already says the same thing. - kept.at(-1)?.what === opts.note + noteIsRepeat(prior, opts.note) ? [] : [opts.note] : describeChange(prior, next); diff --git a/src/core/state.test.ts b/src/core/state.test.ts index 61bcb3a..4c8815f 100644 --- a/src/core/state.test.ts +++ b/src/core/state.test.ts @@ -9,7 +9,7 @@ process.env.CERBER_HOME = home; const { loadArtifact, noteHistory, reconcileRunning, saveArtifact, updateArtifactByKey } = await import("./state.js"); -const { withWriter } = await import("./history.js"); +const { MAX_ENTRIES, withWriter } = await import("./history.js"); function artifact(over: Partial = {}): Artifact { return { @@ -216,6 +216,29 @@ describe("the history every write keeps", () => { expect((await fs.stat(file)).mtimeMs).toBe(written); }); + it("still records a note when the log is already at its cap", async () => { + await saveArtifact(artifact({ status: "skipped" })); + const file = path.join(home, "reviews", `${key}.json`); + const seeded = JSON.parse(readFileSync(file, "utf8")); + seeded.history = Array.from({ length: MAX_ENTRIES }, (_, i) => ({ + at: "2026-08-24T11:07:00Z", + by: "daemon", + what: `entry ${i}`, + cause: null, + })); + writeFileSync(file, JSON.stringify(seeded)); + + const note = "left alone: you marked it skipped"; + await noteHistory(id, note); + + // At the cap, appending trims an older entry — so the log is the same + // length either way, and a caller reading that as "nothing was added" + // would leave a full row unable to record another decision, ever. + const after = await loadArtifact(id); + expect(after?.history).toHaveLength(MAX_ENTRIES); + expect(after?.history?.at(-1)?.what).toBe(note); + }); + it("says so rather than starting over quietly, when the file on disk is broken", async () => { await saveArtifact(artifact({ status: "ready" })); await fs.writeFile(path.join(home, "reviews", `${key}.json`), "{ not json"); diff --git a/src/core/state.ts b/src/core/state.ts index 6022c2d..279ff66 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -2,7 +2,7 @@ import { promises as fs } from "node:fs"; import os from "node:os"; import path from "node:path"; import { Artifact, ArtifactSchema, artifactKey } from "./artifact.js"; -import { appendHistory } from "./history.js"; +import { appendHistory, noteIsRepeat } from "./history.js"; export function cerberHome(): string { return process.env.CERBER_HOME ?? path.join(os.homedir(), ".cerber"); @@ -92,13 +92,13 @@ export async function saveArtifact( export async function noteHistory(id: string, what: string): Promise { const prior = await loadArtifact(id).catch(() => null); if (!prior) return; - // `appendHistory` drops a note identical to the last entry — ask it, rather - // than knowing the rule twice. Nothing to add means nothing to write: the - // poll re-takes these decisions every few minutes, and rewriting a whole - // artifact, diff and all, to change nothing is the expensive half of saying - // it again. - const history = appendHistory(prior, prior, { note: what }); - if (history.length === (prior.history ?? []).length) return; + // Nothing to add means nothing to write: the poll re-takes these decisions + // every few minutes, and rewriting a whole artifact, diff and all, to change + // nothing is the expensive half of saying it again. The rule for "nothing to + // add" is `appendHistory`'s, so ask it rather than knowing it twice — and ask + // it directly, because at the cap an appended note trims an older entry and + // leaves the length exactly as it was. + if (noteIsRepeat(prior, what)) return; await saveArtifact(prior, { note: what }); } From c313e50e83d52699fec977f4eebe6bb695476e51 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 09:34:45 +0200 Subject: [PATCH 4/5] fix(history): credit whoever wanted a review that never ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/runner/review.test.ts | 19 ++++++++++++++++++- src/runner/review.ts | 30 ++++++++++++++++++++++++++---- web/src/Detail.tsx | 16 +++++++++++----- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index a3a753b..35676a0 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -5,6 +5,7 @@ import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; import { Artifact, ArtifactStatus, PrInfo, SCHEMA_VERSION } from "../core/artifact.js"; import { fetchPrDiff, fetchPrInfo } from "../core/gh.js"; import { loadArtifact, saveArtifact } from "../core/state.js"; +import { withWriter } from "../core/history.js"; import { reviewPr } from "./review.js"; // Only the freshness gate is under test: whether a run happens at all. Every @@ -172,7 +173,8 @@ describe("what a new push does to a review", () => { }); describe("what the poll writes down when it decides to do nothing", () => { - const whatHappened = async () => (await loadArtifact("acme/widgets#7"))?.history?.map((e) => e.what) ?? []; + const history = async () => (await loadArtifact("acme/widgets#7"))?.history ?? []; + const whatHappened = async () => (await history()).map((e) => e.what); it("explains a settled row that a push cannot reopen", async () => { // The silence this records is the one that is impossible to debug from the @@ -205,4 +207,19 @@ describe("what the poll writes down when it decides to do nothing", () => { "already reviewed at same-sh — not re-reviewed", ]); }); + + it("credits whoever wanted the review, not the run that never happened", async () => { + // The note's whole value is *who was asking* — the poll's timer, or you at + // a terminal. Stamping it "runner" would name the one party that did + // nothing here, since the guard fired before any AI ran. + await saveArtifact(artifact("skipped", "old-sha")); + const before = (await whatHappened()).length; + prInfo.mockResolvedValue(pr("new-sha")); + + await withWriter({ by: "daemon", cause: "poll" }, () => reviewPr(REF)); + + expect((await history()).slice(before)).toEqual([ + expect.objectContaining({ by: "daemon", cause: "poll" }), + ]); + }); }); diff --git a/src/runner/review.ts b/src/runner/review.ts index ee78bd6..2b6e496 100644 --- a/src/runner/review.ts +++ b/src/runner/review.ts @@ -80,9 +80,7 @@ const SETTLED_BY_YOU = new Set(["reviewed", "skipped"]); export async function reviewPr(ref: PrRef, opts: ReviewOptions = {}): Promise { beginReview(artifactId(ref)); try { - // 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)); + return await runReview(ref, opts); } finally { endReview(artifactId(ref)); } @@ -127,9 +125,17 @@ async function resolveTrust( return decision.trusted; } +/** + * Decide whether to run, then run. + * + * The two halves are deliberately not in the same writer context. Everything + * down to the guards belongs to whoever asked — the poll's timer, you at a + * terminal, the cockpit's button — and that is the whole value of the notes + * they write: a review that did not happen has no runner to blame, and "who + * wanted one" is the fact worth keeping. Only the run itself is the runner's. + */ async function runReview(ref: PrRef, opts: ReviewOptions): Promise { const log = opts.onProgress ?? (() => {}); - const now = () => new Date().toISOString(); log(`Fetching ${ref.owner}/${ref.repo}#${ref.number}…`); const pr = await fetchPrInfo(ref); @@ -162,6 +168,22 @@ async function runReview(ref: PrRef, opts: ReviewOptions): Promise } } + // The run owns its writes from here, 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" }, () => + performReview(ref, pr, existing, opts, log), + ); +} + +async function performReview( + ref: PrRef, + pr: PrInfo, + existing: Artifact | null, + opts: ReviewOptions, + log: (message: string) => void, +): Promise { + const now = () => new Date().toISOString(); + const diff = await fetchPrDiff(ref); const trusted = await resolveTrust(pr, opts, log); diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index f71c8ba..03c209b 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -1193,7 +1193,7 @@ function HistoryCard({ }; return ( -
}> +

history

@@ -1217,15 +1217,21 @@ function HistoryCard({ ) : (
    {/* Newest first: the reason you opened this is almost always the - last thing that happened, or the last thing that didn't. */} - {[...entries].reverse().map((e, i) => ( -
  1. + last thing that happened, or the last thing that didn't. The + key is the entry's position in the *append* order, which does + not move when a new one lands — keying on the reversed index + would re-mount every row on every poll. */} + {entries + .map((e, i) => ({ e, i })) + .reverse() + .map(({ e, i }) => ( +
  2. {stamp(e.at)} {e.by} {e.what} {e.cause && {e.cause}}
    3. - ))} + ))}
)} From d685ec61240d76ab6d139c50efc232721017c828 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 25 Aug 2026 11:10:49 +0200 Subject: [PATCH 5/5] fix(cockpit): the history toggle says whether it is open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- web/src/Detail.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index 03c209b..9e9e200 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -1202,7 +1202,9 @@ function HistoryCard({ : "nothing recorded"} -