fix: the six known bugs, and a typecheck that will notice the next one

package.json

@@ -14,12 +14,13 @@
  "scripts": {
  "build": "tsx scripts/build.ts",
  "build:skills": "tsx scripts/build-skills.ts",
- "test": "npm run test -w @diffity/git && npm run test -w @diffity/github && npm run test -w @diffity/parser && npm run test -w @diffity/ui && npm run test -w diffity && npm run test:scripts",
+ "test": "npm run typecheck && npm run test -w @diffity/git && npm run test -w @diffity/github && npm run test -w @diffity/parser && npm run test -w @diffity/ui && npm run test -w diffity && npm run test:scripts",
  "link-dev": "tsx scripts/link-dev.ts",
  "dev": "tsx scripts/dev.ts",
  "release:patch": "npm run build && tsx scripts/release.ts patch && npm publish -w packages/cli",
  "release:minor": "npm run build && tsx scripts/release.ts minor && npm publish -w packages/cli",
- "test:scripts": "vitest run scripts"
+ "test:scripts": "vitest run scripts",
+ "typecheck": "npm run typecheck -w @diffity/ui && npm run typecheck -w diffity"
  },
  "keywords": [
  "git",

packages/cli/package.json

@@ -1,6 +1,6 @@
 {
  "name": "diffity",
- "version": "0.9.10",
+ "version": "0.9.13",
  "description": "GitHub-style git diff viewer in the browser",
  "type": "module",
  "bin": {
@@ -11,7 +11,8 @@
  "dev": "tsx src/index.ts",
  "dev:watch": "tsx build.ts --watch",
  "test": "vitest run",
- "test:watch": "vitest"
+ "test:watch": "vitest",
+ "typecheck": "tsc --noEmit"
  },
  "dependencies": {
  "commander": "^14.0.3",

packages/cli/src/agent.ts

@@ -22,6 +22,7 @@ import { createHash } from 'node:crypto';
 import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession, getTour } from './tours.js';
 import { unansweredRequest } from './live-unanswered.js';
 import { readAnchor, clampToFile, countWorkingTreeLines } from './anchor.js';
+import { unescapeMarkdown as fromShell } from './unescape.js';
 import { startReviewRun, finishReviewRun } from './review-run.js';
 import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git';
 import { readFileSync } from 'node:fs';
@@ -251,7 +252,7 @@ Examples:
  opts.side,
  startLine,
  endLine,
- opts.body,
+fromShell(opts.body),
  { name: 'Agent', type: 'agent' },
  // Recorded so the finding can follow its code when a later commit moves it.
  opts.side === 'new' ? readAnchor(opts.file, startLine, endLine) : undefined,
@@ -268,7 +269,7 @@ Examples:
  const session = requireSession();
  const thread = resolveThreadId(id, session.id);
  const author = opts.summary ? { name: 'Agent', type: 'agent' as const } : undefined;
- updateThreadStatus(thread.id, 'resolved', opts.summary, author);
+ updateThreadStatus(thread.id, 'resolved', fromShell(opts.summary ?? ''), author);
  console.log(pc.green(`Resolved thread ${thread.id.slice(0, 8)}`));
  });
 
@@ -281,7 +282,7 @@ Examples:
  const session = requireSession();
  const thread = resolveThreadId(id, session.id);
  const author = opts.reason ? { name: 'Agent', type: 'agent' as const } : undefined;
- updateThreadStatus(thread.id, 'dismissed', opts.reason, author);
+ updateThreadStatus(thread.id, 'dismissed', fromShell(opts.reason ?? ''), author);
  console.log(pc.green(`Dismissed thread ${thread.id.slice(0, 8)}`));
  });
 
@@ -296,7 +297,7 @@ Examples:
  const session = requireSession();
  const thread = resolveThreadId(id, session.id);
  const stillOpen = unansweredRequest(thread.comments);
- addReply(thread.id, opts.body, { name: 'Agent', type: 'agent' }, opts.aside ? 'aside' : 'review');
+ addReply(thread.id, fromShell(opts.body), { name: 'Agent', type: 'agent' }, opts.aside ? 'aside' : 'review');
  if (opts.answers && !answerLiveRequest(opts.answers)) {
  console.error(
  pc.yellow(
@@ -424,7 +425,7 @@ Examples:
  .action((commentId: string, opts: { body: string }) => {
  const session = requireSession();
  const sent = findSubmittedThreadForComment(commentId, session.id);
- editComment(commentId, opts.body);
+ editComment(commentId, fromShell(opts.body));
  if (sent) {
  // The forge is showing the old wording and will keep showing it; saying so is the only
  // honest thing available, since a posted review comment cannot be edited from here.
@@ -450,7 +451,7 @@ Examples:
  'new',
  0,
  0,
- opts.body,
+fromShell(opts.body),
  { name: 'Agent', type: 'agent' },
  );
  console.log(pc.green(`Created general comment ${thread.id.slice(0, 8)}`));
@@ -476,7 +477,7 @@ Examples:
  .option('--note <text>', 'What is being reviewed', '')
  .action((opts) => {
  const session = requireSession();
- startReviewRun(session.id, opts.note);
+ startReviewRun(session.id, fromShell(opts.note ?? ''));
  console.log(pc.green('Review marked as in progress'));
  });
 
@@ -537,7 +538,7 @@ Examples:
  .option('--json', 'Output as JSON')
  .action((opts) => {
  const session = requireSession();
- const tour = createTour(session.id, opts.topic, opts.body);
+ const tour = createTour(session.id, fromShell(opts.topic), fromShell(opts.body));
  if (opts.json) {
  console.log(JSON.stringify(tour, null, 2));
  return;
@@ -560,7 +561,7 @@ Examples:
  assertFileExists(opts.file);
  const tourId = resolveTourId(opts.tour, session.id);
  const endLine = opts.endLine ?? opts.line;
- const step = addTourStep(tourId, opts.file, opts.line, endLine, opts.body, opts.annotation);
+ const step = addTourStep(tourId, opts.file, opts.line, endLine, fromShell(opts.body), fromShell(opts.annotation));
  if (opts.json) {
  console.log(JSON.stringify(step, null, 2));
  return;

packages/cli/src/renames.ts

@@ -0,0 +1,38 @@
+/**
+ * Where a file went, from `git diff -M --name-status`.
+ *
+ * Threads carry forward across a commit but their file path does not, so a commit that renames a
+ * file leaves every finding on it pointing at a path that no longer exists — and a thread whose
+ * file is absent from the diff is rendered by nothing, so it goes quiet rather than wrong.
+ *
+ * `--name-status` rather than the diff body: this runs on a poll, and the answer is a few hundred
+ * bytes either way. Renames only, never copies — a copy leaves the original in place, so following
+ * one would take a finding off the file it was written about.
+ */
+export function renamedPaths(nameStatus: string): Map<string, string> {
+ const moves = new Map<string, string>();
+
+ for (const line of nameStatus.split('\n')) {
+ const [status, from, to] = line.split('\t');
+ if (!status?.startsWith('R') || !from || !to || from === to) continue;
+ moves.set(from, to);
+ }
+
+ return moves;
+}
+
+/**
+ * A rename can happen twice across the commits a review spans, so the chain is followed. Bounded,
+ * because a swap would otherwise loop.
+ */
+export function followRename(path: string, moves: Map<string, string>): string {
+ const seen = new Set<string>([path]);
+ let current = path;
+
+ while (true) {
+ const next = moves.get(current);
+ if (!next || seen.has(next)) return current;
+ seen.add(next);
+ current = next;
+ }
+}

packages/cli/src/server.ts

@@ -382,7 +382,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
  if (asked) {
  return resolveSessionId(asked);
  }
- return findOrCreateSession(url.searchParams.get('ref') || effectiveRef).id;
+ return findOrCreateSession(url.searchParams.get('ref') || effectiveRef || 'work').id;
  };
 
  if (pathname === '/api/live/status') {

packages/cli/src/session.ts

@@ -1,18 +1,28 @@
 import { randomUUID } from 'node:crypto';
 import { readFileSync, writeFileSync } from 'node:fs';
 import { join } from 'node:path';
-import { getHeadHash, getDiffityDir, getRepoRoot, getCurrentBranch, WORKING_TREE_REFS } from '@diffity/git';
+import { getHeadHash, getDiffityDir, getRepoRoot, getCurrentBranch, getRenameStatus, WORKING_TREE_REFS } from '@diffity/git';
+import { renamedPaths, followRename } from './renames.js';
 import { getDb, queryAll, queryOne } from './db.js';
 import { reanchorInWorkingTree } from './anchor.js';
 import { carryReviewRun } from './review-run.js';
-import { updateThreadLines } from './threads.js';
+import { updateThreadLines, updateThreadPath } from './threads.js';
 
 export interface Session {
  id: string;
  ref: string;
  headHash: string;
 }
 
+/**
+ * `git rev-parse --abbrev-ref HEAD` says `HEAD` on a detached checkout, which is not a branch name.
+ * Recorded as one it matches nothing, so a session written before `gh pr checkout` put the worktree
+ * on a real branch is stranded along with its findings.
+ */
+function namedBranch(branch: string): string | null {
+ return branch === 'HEAD' ? null : branch;
+}
+
 function sessionFilePath(): string {
  return join(getDiffityDir(), 'current-session');
 }
@@ -61,10 +71,27 @@ function sessionsInScope(
  ).filter(row => branchMatches(row.branch, branch) && reviewScope(row.ref) === scope);
 }
 
+/**
+ * Whether work may move from that session into this one.
+ *
+ * Two base refs belong to one review only because a branch's base moves as the branch is updated.
+ * With no branch on this side, that reasoning is gone: nothing connects one base commit to another,
+ * so two unrelated pull requests reviewed in one detached checkout look like a single review and the
+ * newer takes the older's findings. Requiring the same ref strands a review instead of merging it
+ * into somebody else's, which is the right way round to be wrong.
+ *
+ * Only taking work is guarded. Pointing a stale tab at a newer session shows the reader something
+ * they can check, and a row with no branch of its own is a session from before branches were
+ * recorded, which is a migration rather than a collision.
+ */
+function mayCarryFrom(rowRef: string, ref: string, branch: string | null): boolean {
+ return branch !== null || rowRef === ref;
+}
+
 export function findOrCreateSession(ref: string): Session {
  const headHash = getHeadHash();
  const repoRoot = getRepoRoot();
- const branch = getCurrentBranch();
+ const branch = namedBranch(getCurrentBranch());
 
  const { session, created } = openSession(ref, headHash, repoRoot, branch);
 
@@ -75,13 +102,21 @@ export function findOrCreateSession(ref: string): Session {
  // A superseded session is never deleted, so "a sibling exists" stays true forever and cannot be
  // what decides this. `/api/info` calls in here on a five-second poll, and the work below moves
  // rows and reads the working tree once per anchored finding.
- const siblings = sessionsInScope(repoRoot, branch, ref).filter(row => row.id !== session.id);
+ const siblings = sessionsInScope(repoRoot, branch, ref)
+ .filter(row => row.id !== session.id)
+ .filter(row => mayCarryFrom(row.ref, ref, branch));
  const donors = sessionsHoldingWork(siblings.map(row => row.id));
 
  if (donors.length > 0) {
  gatherOpenWork(donors, session.id);
  }
 
+ // Before re-anchoring, which reads the working tree at each thread's path: a thread still
+ // holding a pre-rename path would find nothing there and quietly keep its old lines.
+ if (donors.length > 0) {
+ followRenamesForSession(session.id, donors, headHash);
+ }
+
  if (created || donors.length > 0) {
  // A run belongs to the session the review was last read through — the newest sibling, which is
  // not necessarily one holding findings. Taking one from any older sibling would bring a review
@@ -127,7 +162,7 @@ function openSession(
  ref: string,
  headHash: string,
  repoRoot: string | null,
- branch: string,
+ branch: string | null,
 ): { session: Session; created: boolean } {
  const db = getDb();
 
@@ -221,6 +256,56 @@ function gatherOpenWork(fromSessionIds: string[], toSessionId: string): void {
  * A finding that outlives the commit it was written against points at a line that has since
  * moved. Only the new side is re-anchored: a comment on a removed line has nothing to follow.
  */
+function donorHeads(donorIds: string[]): string[] {
+ if (donorIds.length === 0) {
+ return [];
+ }
+ const placeholders = donorIds.map(() => '?').join(', ');
+ return queryAll<{ head_hash: string }>(
+ `SELECT DISTINCT head_hash FROM review_sessions WHERE id IN (${placeholders})`,
+ ...donorIds,
+ ).map(row => row.head_hash);
+}
+
+/**
+ * A carried thread keeps the path it was written against, and a commit that renames a file leaves
+ * it pointing at one that is gone. Nothing renders a thread whose file is absent from the diff, so
+ * the finding does not look wrong — it disappears.
+ */
+function followRenamesForSession(sessionId: string, donorIds: string[], toHead: string): void {
+ const moves = new Map<string, string>();
+
+ // Between the commit a finding was written against and the one it is carried to. The review's own
+ // diff is no help: it shows the file only under the name it ends up with, so the rename is not
+ // in it to find.
+ for (const from of donorHeads(donorIds)) {
+ if (from === toHead) continue;
+ try {
+ for (const [before, after] of renamedPaths(getRenameStatus(from, toHead))) {
+ moves.set(before, after);
+ }
+ } catch {
+ continue;
+ }
+ }
+
+ if (moves.size === 0) {
+ return;
+ }
+
+ const threads = queryAll<{ id: string; file_path: string }>(
+ "SELECT id, file_path FROM comment_threads WHERE session_id = ? AND status = 'open'",
+ sessionId,
+ );
+
+ for (const thread of threads) {
+ const moved = followRename(thread.file_path, moves);
+ if (moved !== thread.file_path) {
+ updateThreadPath(thread.id, moved);
+ }
+ }
+}
+
 function reanchorThreads(sessionId: string): void {
  const threads = queryAll<{
  id: string;