diff --git a/CLAUDE.md b/CLAUDE.md index 864b432..a85f213 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 aa3f4ab..b87d0f3 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). --- @@ -171,6 +173,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 @@ -299,6 +305,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. + ### Asked again: the way back out of settled Filing's mirror image, and the only thing that reopens a settled row on its own @@ -386,6 +398,48 @@ 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. + +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, + 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 @@ -454,6 +508,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 0cfce6c..6e01f36 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; @@ -337,6 +338,16 @@ 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, 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).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..f81943b --- /dev/null +++ b/src/core/history.test.ts @@ -0,0 +1,264 @@ +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, + settledAt: 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..07e79b2 --- /dev/null +++ b/src/core/history.ts @@ -0,0 +1,252 @@ +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; +} + +/** + * 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. + * + * 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. + noteIsRepeat(prior, 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 12741f3..91500a5 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"; @@ -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 { MAX_ENTRIES, withWriter } = await import("./history.js"); function artifact(over: Partial = {}): Artifact { return { @@ -130,3 +132,120 @@ 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("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" }, () => + 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("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"); + 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..279ff66 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, noteIsRepeat } from "./history.js"; export function cerberHome(): string { return process.env.CERBER_HOME ?? path.join(os.homedir(), ".cerber"); @@ -15,15 +16,92 @@ 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. + * + * 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: { note?: string } = {}, +): Promise { await fs.mkdir(reviewsDir(), { recursive: true }); const file = artifactPath(artifact.id); + const prior = 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; + // 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 }); +} + export async function loadArtifact(id: string): Promise { try { const raw = await fs.readFile(artifactPath(id), "utf8"); diff --git a/src/runner/review.test.ts b/src/runner/review.test.ts index 47c9d30..72df7cb 100644 --- a/src/runner/review.test.ts +++ b/src/runner/review.test.ts @@ -4,7 +4,8 @@ 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 { withWriter } from "../core/history.js"; import { reviewPr } from "./review.js"; // Only the freshness gate is under test: whether a run happens at all. Every @@ -171,3 +172,55 @@ 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 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 + // 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 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", + ]); + }); + + 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 d227484..18d49ab 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"; @@ -124,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); @@ -135,6 +144,14 @@ 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. + // Only a push, now — somebody asking again does reopen it, and says so + // for itself (`reopenIfAskedAgain`). + await noteHistory( + existing.id, + `left alone: you marked it ${existing.status}, so a new push does not reopen it`, + ); return { artifact: existing, skipped: true }; } // The sha the AI *read*, not the one the artifact happens to mention. @@ -146,10 +163,29 @@ 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 }; } } + // 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/src/server/daemon.test.ts b/src/server/daemon.test.ts index fb01e82..d915347 100644 --- a/src/server/daemon.test.ts +++ b/src/server/daemon.test.ts @@ -901,6 +901,18 @@ describe("a review you settled, and were asked for again", () => { expect(after?.run?.reviewedSha).toBe("abc1234"); }); + it("writes down why the row came back, not just that it did", async () => { + // A status change says a row moved. The point of undoing a settle is *why* + // it was undone, and that is the one thing the status cannot carry. + await saveArtifact(settled()); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + + const after = await pollOnce(); + expect(after?.history?.map((e) => e.what)).toContain( + "back in the inbox: your review was requested again on 2026-08-24, after you settled it", + ); + }); + it("leaves your skip standing when the ask is the one you already answered", async () => { await saveArtifact(settled()); lastRequest.mockResolvedValue("2026-08-21T10:43:27Z"); diff --git a/src/server/daemon.ts b/src/server/daemon.ts index a257d44..bad2e6f 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -16,6 +16,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 { @@ -23,6 +24,7 @@ import { deleteArtifact, listArtifacts, loadArtifact, + noteHistory, saveArtifact, updateArtifactByKey, } from "../core/state.js"; @@ -406,6 +408,12 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { // row never had one, so a mutation that declined would read as a reopen. if (saved && !SETTLED_BY_YOU.has(saved.status)) { log(`[${artifact.id}] your review was requested again on ${at.slice(0, 10)} — back in the inbox`); + // The status change alone says a row moved; this says why it moved, + // which is the whole reason a settle is allowed to be undone at all. + await noteHistory( + artifact.id, + `back in the inbox: your review was requested again on ${at.slice(0, 10)}, after you settled it`, + ); } } @@ -498,13 +506,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", @@ -661,7 +680,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 636d2b9..a6bbc49 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..9e9e200 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,82 @@ 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"} + + + {/* A disclosure, so it says whether it is open — the label alone leaves + a screen reader to infer that from the word "show". */} + +
+ {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. 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 }) => ( +
  1. + {stamp(e.at)} + {e.by} + {e.what} + {e.cause && {e.cause}} +
  2. + ))} +
+ )} +
+ )} +
+ ); +} + function FreshnessBanner({ artifact, freshness, @@ -1249,6 +1326,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 +1766,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 +1949,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. */