feat(review): say when a review is still running, and refuse to submit until it is not

packages/cli/src/agent.ts

@@ -15,6 +15,7 @@ import {
 } from './threads.js';
 import { createTour, addTourStep, updateTourStatus } from './tours.js';
 import { readAnchor } from './anchor.js';
+import { startReviewRun, finishReviewRun } from './review-run.js';
 import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git';
 import { readFileSync } from 'node:fs';
 
@@ -243,6 +244,25 @@ Examples:
       process.stdout.write(raw);
     });
 
+  agent
+    .command('review-start')
+    .description('Announce that a review is under way, so the page can say so')
+    .option('--note <text>', 'What is being reviewed', '')
+    .action((opts) => {
+      const session = requireSession();
+      startReviewRun(session.id, opts.note);
+      console.log(pc.green('Review marked as in progress'));
+    });
+
+  agent
+    .command('review-done')
+    .description('Announce that the review is finished')
+    .action(() => {
+      const session = requireSession();
+      finishReviewRun(session.id);
+      console.log(pc.green('Review marked as finished'));
+    });
+
   agent
     .command('standards')
     .description("Print the project's review standards and severity labels")

packages/cli/src/db.ts

@@ -113,6 +113,13 @@ function migrateDb(db: DatabaseSync): void {
       created_at TEXT NOT NULL DEFAULT (datetime('now'))
     );
 
+    CREATE TABLE IF NOT EXISTS review_runs (
+      session_id TEXT PRIMARY KEY REFERENCES review_sessions(id),
+      started_at TEXT NOT NULL,
+      finished_at TEXT,
+      note TEXT NOT NULL DEFAULT ''
+    );
+
     CREATE INDEX IF NOT EXISTS idx_tours_session ON tours(session_id);
     CREATE INDEX IF NOT EXISTS idx_tour_steps_tour ON tour_steps(tour_id);
   `);

packages/cli/src/fingerprint.ts

@@ -0,0 +1,26 @@
+import { createHash } from 'node:crypto';
+import { getDiffStat, getDiffStatForRef, getHeadHash, getUntrackedFiles } from '@diffity/git';
+
+/**
+ * A diffstat only counts lines, so a commit that rewrites the same number of them produces an
+ * identical stat and the diff would look unchanged. Including the head commit makes any new
+ * commit visible, while the stat still catches edits to the working tree, where HEAD does not move.
+ */
+export function computeDiffFingerprint(
+  ref: string | null,
+  diffArgs: string[] = [],
+  includeUntracked = false,
+): string {
+  let stat: string;
+
+  if (ref) {
+    stat = getDiffStatForRef(ref);
+  } else {
+    stat = getDiffStat(diffArgs);
+    if (includeUntracked) {
+      stat += '\n' + getUntrackedFiles().join('\n');
+    }
+  }
+
+  return createHash('sha1').update(`${getHeadHash()}\n${stat}`).digest('hex');
+}

packages/cli/src/review-routes.ts

@@ -11,7 +11,7 @@ import {
   type ThreadAuthor,
   type ThreadStatus,
 } from './threads.js';
-import { getCurrentSession } from './session.js';
+import { getCurrentSession, resolveSessionId } from './session.js';
 import { sendJson, sendError, withJsonBody } from './http-utils.js';
 
 export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pathname: string, url: URL): boolean {
@@ -22,9 +22,9 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat
   }
 
   if (pathname === '/api/threads' && req.method === 'GET') {
-    const sid = url.searchParams.get('session');
+    const sid = resolveSessionId(url.searchParams.get('session'));
     if (!sid) {
-      sendError(res, 400, 'Missing session parameter');
+      sendError(res, 400, 'No review session');
       return true;
     }
     const status = url.searchParams.get('status') as ThreadStatus | null;

packages/cli/src/review-run.ts

@@ -0,0 +1,69 @@
+import { getDb, queryOne } from './db.js';
+
+export interface ReviewRun {
+  inProgress: boolean;
+  startedAt: string | null;
+  note: string;
+}
+
+const ABSENT: ReviewRun = { inProgress: false, startedAt: null, note: '' };
+
+/**
+ * Whether an agent is part-way through writing findings. A reader who cannot tell the difference
+ * between "no problems found" and "not finished looking" can approve a change too early.
+ */
+export function getReviewRun(sessionId: string): ReviewRun {
+  const row = queryOne<{ started_at: string; finished_at: string | null; note: string }>(
+    'SELECT started_at, finished_at, note FROM review_runs WHERE session_id = ?',
+    sessionId,
+  );
+
+  if (!row) {
+    return ABSENT;
+  }
+
+  return {
+    inProgress: !row.finished_at,
+    startedAt: row.started_at,
+    note: row.note,
+  };
+}
+
+export function startReviewRun(sessionId: string, note: string): void {
+  getDb()
+    .prepare(
+      `INSERT INTO review_runs (session_id, started_at, finished_at, note)
+       VALUES (?, datetime('now'), NULL, ?)
+       ON CONFLICT(session_id) DO UPDATE SET
+         started_at = datetime('now'),
+         finished_at = NULL,
+         note = excluded.note`,
+    )
+    .run(sessionId, note);
+}
+
+export function finishReviewRun(sessionId: string): void {
+  getDb()
+    .prepare("UPDATE review_runs SET finished_at = datetime('now') WHERE session_id = ?")
+    .run(sessionId);
+}
+
+/** An unfinished run follows the session, so committing mid-review does not clear the warning. */
+export function carryReviewRun(fromSessionId: string, toSessionId: string): void {
+  const run = queryOne<{ started_at: string; note: string }>(
+    'SELECT started_at, note FROM review_runs WHERE session_id = ? AND finished_at IS NULL',
+    fromSessionId,
+  );
+
+  if (!run) {
+    return;
+  }
+
+  getDb()
+    .prepare(
+      `INSERT INTO review_runs (session_id, started_at, finished_at, note)
+       VALUES (?, ?, NULL, ?)
+       ON CONFLICT(session_id) DO UPDATE SET started_at = excluded.started_at, finished_at = NULL, note = excluded.note`,
+    )
+    .run(toSessionId, run.started_at, run.note);
+}

packages/cli/src/server.ts

@@ -12,7 +12,6 @@ import { dirname } from 'node:path';
 import { parseDiff, type ParsedDiff } from '@diffity/parser';
 import {
   getDiff,
-  getDiffStatForRef,
   getDiffStat,
   getUntrackedFiles,
   getUntrackedDiff,
@@ -47,6 +46,8 @@ import {
   type ReviewEvent,
 } from '@diffity/github';
 import { findOrCreateSession } from './session.js';
+import { computeDiffFingerprint } from './fingerprint.js';
+import { getReviewRun } from './review-run.js';
 import { createThread, addReply, getThreadsForSession } from './threads.js';
 import { handleReviewRoute } from './review-routes.js';
 import { handleTourRoute } from './tour-routes.js';
@@ -397,20 +398,9 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
 
         if (pathname === '/api/diff-fingerprint') {
           const ref = url.searchParams.get('ref');
-          let stat: string;
-          if (ref) {
-            stat = getDiffStatForRef(ref);
-          } else {
-            stat = getDiffStat(diffArgs);
-            if (includeUntracked) {
-              stat += '\n' + getUntrackedFiles().join('\n');
-            }
-          }
-          const hash = createHash('sha1')
-            .update(stat)
-            .digest('hex')
-            .slice(0, 12);
-          sendJson(res, { fingerprint: hash });
+          sendJson(res, {
+            fingerprint: computeDiffFingerprint(ref, diffArgs, includeUntracked),
+          });
           return;
         }
 
@@ -486,6 +476,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
             description: refDescription,
             capabilities,
             sessionId,
+            review: sessionId ? getReviewRun(sessionId) : null,
             github: githubRemote,
             editor: editorAvailable,
           });

packages/cli/src/session.ts

@@ -4,6 +4,7 @@ import { join } from 'node:path';
 import { getHeadHash, getDiffityDir } from '@diffity/git';
 import { getDb, queryAll, queryOne } from './db.js';
 import { reanchorInWorkingTree } from './anchor.js';
+import { carryReviewRun } from './review-run.js';
 import { updateThreadLines } from './threads.js';
 
 export interface Session {
@@ -74,6 +75,7 @@ export function carryForward(fromSessionId: string, toSessionId: string): void {
     fromSessionId,
   );
 
+  carryReviewRun(fromSessionId, toSessionId);
   reanchorThreads(toSessionId);
 }
 
@@ -101,6 +103,32 @@ function reanchorThreads(sessionId: string): void {
   }
 }
 
+/**
+ * The session a request is about. A browser tab holds whichever id it loaded with, and a commit
+ * since then will have carried the threads into a newer session for the same ref — so honouring a
+ * stale id literally would tell the tab the review is empty.
+ */
+export function resolveSessionId(sessionId: string | null | undefined): string {
+  if (!sessionId) {
+    return getCurrentSession()?.id ?? '';
+  }
+
+  const known = queryOne<{ ref: string }>(
+    'SELECT ref FROM review_sessions WHERE id = ?',
+    sessionId,
+  );
+  if (!known) {
+    return sessionId;
+  }
+
+  const newest = queryOne<{ id: string }>(
+    'SELECT id FROM review_sessions WHERE ref = ? ORDER BY created_at DESC, rowid DESC LIMIT 1',
+    known.ref,
+  );
+
+  return newest?.id ?? sessionId;
+}
+
 export function getCurrentSession(): Session | null {
   try {
     const raw = readFileSync(sessionFilePath(), 'utf-8');

packages/cli/src/tour-routes.ts

@@ -8,12 +8,13 @@ import {
   type TourStatus,
 } from './tours.js';
 import { sendJson, sendError, withJsonBody } from './http-utils.js';
+import { resolveSessionId } from './session.js';
 
 export function handleTourRoute(req: IncomingMessage, res: ServerResponse, pathname: string, url: URL): boolean {
   if (pathname === '/api/tours' && req.method === 'GET') {
-    const sid = url.searchParams.get('session');
+    const sid = resolveSessionId(url.searchParams.get('session'));
     if (!sid) {
-      sendError(res, 400, 'Missing session parameter');
+      sendError(res, 400, 'No review session');
       return true;
     }
     const tours = getToursForSession(sid);

packages/cli/tests/review-run.test.ts

@@ -0,0 +1,105 @@
+import { describe, it, expect, beforeAll, afterAll } from 'vitest';
+import { execFileSync } from 'node:child_process';
+import { mkdtempSync, writeFileSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+
+let root: string;
+let repoDir: string;
+let origCwd: string;
+
+function commit(name: string): void {
+  writeFileSync(join(repoDir, name), `${name}\n`);
+  execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' });
+  execFileSync('git', ['commit', '-m', name], { cwd: repoDir, stdio: 'pipe' });
+}
+
+beforeAll(() => {
+  origCwd = process.cwd();
+  root = mkdtempSync(join(tmpdir(), 'diffity-run-'));
+  repoDir = join(root, 'repo');
+  execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' });
+  execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' });
+  execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' });
+  commit('a.txt');
+  process.env.DIFFITY_DATA_DIR = join(root, 'notes');
+  process.chdir(repoDir);
+});
+
+afterAll(() => {
+  process.chdir(origCwd);
+  delete process.env.DIFFITY_DATA_DIR;
+  rmSync(root, { recursive: true, force: true });
+});
+
+describe('a review run', () => {
+  it('is absent until one starts', async () => {
+    const { getReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+
+    expect(getReviewRun(findOrCreateSession('work').id).inProgress).toBe(false);
+  });
+
+  it('is in progress once started, carrying its note', async () => {
+    const { getReviewRun, startReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+    const session = findOrCreateSession('work');
+
+    startReviewRun(session.id, 'reading the idempotency change');
+    const run = getReviewRun(session.id);
+
+    expect(run.inProgress).toBe(true);
+    expect(run.note).toBe('reading the idempotency change');
+    expect(run.startedAt).toBeTruthy();
+  });
+
+  it('stops being in progress when it finishes', async () => {
+    const { getReviewRun, startReviewRun, finishReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+    const session = findOrCreateSession('work');
+
+    startReviewRun(session.id, '');
+    finishReviewRun(session.id);
+
+    expect(getReviewRun(session.id).inProgress).toBe(false);
+  });
+
+  it('keeps one run per session when started twice', async () => {
+    const { getReviewRun, startReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+    const session = findOrCreateSession('work');
+
+    startReviewRun(session.id, 'first');
+    startReviewRun(session.id, 'second');
+
+    expect(getReviewRun(session.id).note).toBe('second');
+    expect(getReviewRun(session.id).inProgress).toBe(true);
+  });
+
+  it('follows the session when a commit moves HEAD mid-review', async () => {
+    const { getReviewRun, startReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+    const before = findOrCreateSession('work');
+    startReviewRun(before.id, 'still going');
+
+    commit('b.txt');
+    const after = findOrCreateSession('work');
+
+    expect(after.id).not.toBe(before.id);
+    expect(getReviewRun(after.id).inProgress).toBe(true);
+    expect(getReviewRun(after.id).note).toBe('still going');
+  });
+
+  it('does not resurrect a finished run on a later session', async () => {
+    const { getReviewRun, startReviewRun, finishReviewRun } = await import('../src/review-run.js');
+    const { findOrCreateSession } = await import('../src/session.js');
+    const before = findOrCreateSession('work');
+    startReviewRun(before.id, 'done with this');
+    finishReviewRun(before.id);
+
+    commit('c.txt');
+    const after = findOrCreateSession('work');
+
+    expect(getReviewRun(after.id).inProgress).toBe(false);
+  });
+});