feat: focused reviews — rename handling, guide movedFrom, and a churn-skim taxonomy

Moved files no longer drown a review in delete+add noise, and "give me a
focused review" is now a defined contract phrase:

- Explicit -M on every git diff: committed/staged renames render merged
  (edits only + a "moved from" badge) regardless of diff.renames config;
  pure renames surface as a muted "renamed · no changes" row folded into
  the Skimmed group instead of vanishing or dominating the diff.
- Working-mode pairing: a plain `mv` (deleted tracked file + byte-identical
  untracked file, unique 1:1) auto-merges into the same rename presentation;
  whole-file Approve stages both paths so the index records a real rename.
- Guide movedFrom: the agent declares moved+edited files; the desk merges
  the pair so only the real edits need eyes (working repo mode, strict on a
  new guide, silent fallback on carry-forward).
- Decisions/comments/sign-offs migrate old→new path across the reload that
  introduces a rename instead of silently dropping.
- Focused review defined in the spec: a reviewer-requested modifier with a
  default churn policy (lockfiles/generated → skim, import-only blocks →
  skimBlocks, moved+edited → movedFrom, logic/config → never skim) and a
  display-only `focused` guide flag that badges the overview.
- Spec deduped alongside (single statements for the READ-ONLY question rule
  and the reload-resets invariant; the loop example iterates questions[]
  with a space-safe while-read).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QJ3L7PhcBSV6awUcBa8KuK

Author: ymansurozer

skills/galley/SKILL.md

@@ -14,7 +14,7 @@ The review is the human's; the agent acts on the decisions and answers questions
 
 ## When to use it
 
-Reach for Galley when the user should review something turn-by-turn: **code changes you made** (the working tree or staged diff), **a markdown plan or single artifact**, or **a branch / PR**. Use it when the user asks to "open the Galley", or whenever a diff is better reviewed interactively than pasted into chat.
+Reach for Galley when the user should review something turn-by-turn: **code changes you made** (the working tree or staged diff), **a markdown plan or single artifact**, or **a branch / PR**. Use it when the user asks to "open the Galley", or whenever a diff is better reviewed interactively than pasted into chat. When the user asks for a **focused review** (mechanical churn — lockfiles, generated code, import churn, moved files — de-emphasized so only the real changes stand out), attach a guide; `galley spec` documents the focused-review schema.
 
 ## Getting the tool
 

skills/galley/agents-snippet.md

@@ -12,4 +12,6 @@ When the user should review a plan, a PR, or code changes you've made, hand it t
 - **A markdown plan / single artifact** → `galley file <path> &`.
 - **A branch / PR** → `galley pr <ref> &`.
 
+For a **focused review** (mechanical churn de-emphasized so only the real changes stand out), attach a guide — `galley spec` documents it.
+
 **For the full contract — the `await`/`comment`/`reload` loop, event shapes, `ReviewResult`, how to act on accepted/rejected/requested changes, the guided-review schema, and all options — run `galley spec` and follow it (once per session before your first review).**

src/cli.ts

@@ -16,13 +16,14 @@ import {
   mergeReviewState,
   persistReview,
   readDeskLock,
+  resolveMovedFrom,
   resolveSkim,
   reviewDir,
   sanitizeSession,
   stablePort,
   syncGitState,
 } from "./state.js";
-import type { ReviewMode } from "./types.js";
+import type { Guide, ReviewMode } from "./types.js";
 import { maybeOfferUpdate } from "./update.js";
 
 function parseArgs(argv: string[]) {
@@ -433,18 +434,36 @@ async function runDesk(
     return;
   }
   const saved = await loadLatestReview(base.root, session);
-  const state = mergeReviewState(base, saved);
-  // Optional agent-generated guided review, attached at startup. Required to be a readable,
-  // valid JSON file when --guide is passed; survives reload via the state merge.
+  // Load the new guide (if any) up front — loadGuideArg validates it. Guide-declared moves
+  // (movedFrom) must merge into `base` BEFORE reconciliation, so the merged pair's distinct paths
+  // drive mergeReviewState's rename migration (issue 01). A new guide resolves strictly (an
+  // unresolvable move aborts the launch); a guide carried forward by a previous session resolves
+  // leniently off the saved state (the move drops back to delete+add).
+  let newGuide: Guide | undefined;
   if (args.guide !== undefined) {
-    const guide = loadGuideArg(args.guide);
-    if (!guide) {
+    const loaded = loadGuideArg(args.guide);
+    if (!loaded) {
+      process.exitCode = 1;
+      return;
+    }
+    newGuide = loaded;
+  }
+  const moveGuide = newGuide ?? saved?.guide;
+  if (moveGuide) {
+    const moved = resolveMovedFrom(base, moveGuide, { strict: !!newGuide });
+    if (!moved.ok) {
+      console.error(`Invalid guide: ${moved.reason}.`);
       process.exitCode = 1;
       return;
     }
+  }
+  const state = mergeReviewState(base, saved);
+  // Optional agent-generated guided review, attached at startup. Required to be a readable,
+  // valid JSON file when --guide is passed; survives reload via the state merge.
+  if (newGuide) {
     // Stamp the diff hash the guide was generated against; if a later reload advances the
     // diff past it, the desk flags the guide as possibly stale (slice 05).
-    state.guide = { ...guide, baseDiffHash: state.baseDiffHash };
+    state.guide = { ...newGuide, baseDiffHash: state.baseDiffHash };
     // Resolve skim spans against the fresh diff and stamp the collapsed blocks. Strict at
     // initial attach: an unresolvable span aborts the launch naming the offending field, like
     // any other invalid-guide input.

src/diffsource.test.ts

@@ -1,6 +1,6 @@
 import assert from "node:assert/strict";
 import { execFileSync } from "node:child_process";
-import { mkdtempSync, writeFileSync, rmSync } from "node:fs";
+import { mkdtempSync, writeFileSync, rmSync, renameSync } from "node:fs";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { test, before, after } from "node:test";
@@ -138,3 +138,145 @@ test("repo mode: staged diff ignores untracked files", async () => {
   assert.equal(src, null); // nothing staged, and untracked must not leak into staged mode
   rmSync(path.join(root, "new.ts"));
 });
+
+// ── git -M rename handling (issue 01) ────────────────────────────────────────
+// Self-contained repos (own git init) so ordering can't collide with the shared-root tests above,
+// and so we can force `diff.renames=false` — proving detection rides our explicit -M, not config.
+
+function freshRepo(renamesOff: boolean): { dir: string; main: string } {
+  const dir = mkdtempSync(path.join(tmpdir(), "galley-rn-"));
+  const g = (args: string[]) => execFileSync("git", args, { cwd: dir }).toString();
+  g(["init", "-q"]);
+  g(["config", "user.email", "t@t.co"]);
+  g(["config", "user.name", "tester"]);
+  if (renamesOff) g(["config", "diff.renames", "false"]);
+  writeFileSync(path.join(dir, "old.txt"), "a\nb\nc\n");
+  g(["add", "."]);
+  g(["commit", "-qm", "init"]);
+  const main = g(["rev-parse", "--abbrev-ref", "HEAD"]).trim();
+  return { dir, main };
+}
+
+test("pr mode: rename+edit with diff.renames=false → one file at the new path, edited lines only", async () => {
+  const { dir, main } = freshRepo(true);
+  const g = (args: string[]) => execFileSync("git", args, { cwd: dir }).toString();
+  g(["checkout", "-q", "-b", "feature"]);
+  g(["mv", "old.txt", "new.txt"]);
+  writeFileSync(path.join(dir, "new.txt"), "a\nB\nc\n"); // one edited line
+  g(["commit", "-qam", "rename + edit"]);
+  const src = await buildDiffSource({ mode: "pr", root: dir, base: main });
+  assert.ok(src);
+  assert.equal(src!.files.length, 1);
+  const f = src!.files[0]!;
+  assert.equal(f.path, "new.txt");
+  assert.equal(f.oldFile.name, "old.txt"); // distinct names → @pierre infers rename-changed
+  assert.equal(f.newFile.name, "new.txt");
+  assert.equal(f.oldFile.contents, "a\nb\nc\n"); // full old content (moved, not re-added)
+  assert.equal(f.newFile.contents, "a\nB\nc\n");
+  assert.equal(src!.changes.length, 1); // only the edited line is up for review, not the whole file
+  rmSync(dir, { recursive: true, force: true });
+});
+
+test("pr mode: a pure committed rename → a zero-hunk entry at the new path, distinct names", async () => {
+  const { dir, main } = freshRepo(true);
+  const g = (args: string[]) => execFileSync("git", args, { cwd: dir }).toString();
+  g(["checkout", "-q", "-b", "feature"]);
+  g(["mv", "old.txt", "new.txt"]);
+  g(["commit", "-qam", "pure rename"]);
+  const src = await buildDiffSource({ mode: "pr", root: dir, base: main });
+  assert.ok(src, "a pure rename must not vanish from the review");
+  assert.equal(src!.files.length, 1);
+  const f = src!.files[0]!;
+  assert.equal(f.path, "new.txt");
+  assert.equal(f.oldFile.name, "old.txt");
+  assert.equal(f.newFile.name, "new.txt");
+  assert.equal(f.hunks.length, 0); // no content change
+  assert.equal(f.oldFile.contents, f.newFile.contents); // identical → the muted moved row in the UI
+  assert.equal(src!.changes.length, 0);
+  rmSync(dir, { recursive: true, force: true });
+});
+
+test("staged mode: a staged git mv + edit renders as a merged rename", async () => {
+  const { dir } = freshRepo(false);
+  const g = (args: string[]) => execFileSync("git", args, { cwd: dir }).toString();
+  g(["mv", "old.txt", "new.txt"]);
+  writeFileSync(path.join(dir, "new.txt"), "a\nB\nc\n");
+  g(["add", "."]);
+  const src = await buildDiffSource({ mode: "repo", root: dir, staged: true });
+  assert.ok(src);
+  assert.equal(src!.files.length, 1);
+  const f = src!.files[0]!;
+  assert.equal(f.path, "new.txt");
+  assert.equal(f.oldFile.name, "old.txt");
+  assert.equal(f.newFile.name, "new.txt");
+  assert.equal(src!.changes.length, 1);
+  rmSync(dir, { recursive: true, force: true });
+});
+
+// ── working-mode move pairing (issue 02) ─────────────────────────────────────
+// A plain `mv` (no git mv) shows as a full deletion + a full untracked addition; git can't see
+// the move. buildDiffSource pairs byte-identical halves into one rename-pure entry.
+
+test("repo mode: a plain mv (no edit) pairs into one rename-pure entry", async () => {
+  const { dir } = freshRepo(false);
+  renameSync(path.join(dir, "old.txt"), path.join(dir, "new.txt")); // plain mv, not git mv
+  const src = await buildDiffSource({ mode: "repo", root: dir });
+  assert.ok(src);
+  assert.equal(src!.files.length, 1); // merged, not delete + add
+  const f = src!.files[0]!;
+  assert.equal(f.path, "new.txt");
+  assert.equal(f.oldPath, "old.txt");
+  assert.equal(f.newPath, "new.txt");
+  assert.equal(f.oldFile.name, "old.txt");
+  assert.equal(f.newFile.name, "new.txt");
+  assert.equal(f.oldFile.contents, f.newFile.contents); // identical → the muted moved row
+  assert.equal(src!.changes.length, 0);
+  rmSync(dir, { recursive: true, force: true });
+});
+
+test("repo mode: two identical untracked copies of a deleted file → nothing paired (ambiguity)", async () => {
+  const { dir } = freshRepo(false);
+  rmSync(path.join(dir, "old.txt")); // delete the tracked file…
+  writeFileSync(path.join(dir, "copy1.txt"), "a\nb\nc\n"); // …and two byte-identical untracked copies
+  writeFileSync(path.join(dir, "copy2.txt"), "a\nb\nc\n");
+  const src = await buildDiffSource({ mode: "repo", root: dir });
+  assert.ok(src);
+  assert.deepEqual(
+    src!.files.map((f) => f.path).sort(),
+    ["copy1.txt", "copy2.txt", "old.txt"], // deletion stays + two additions, none merged
+  );
+  assert.equal(
+    src!.files.find((f) => f.path === "old.txt")!.newPath,
+    undefined, // still a plain deletion, not a rename
+  );
+  rmSync(dir, { recursive: true, force: true });
+});
+
+test("repo mode: mv + edit is NOT paired — renders as delete + add (exact-content only)", async () => {
+  const { dir } = freshRepo(false);
+  rmSync(path.join(dir, "old.txt"));
+  writeFileSync(path.join(dir, "new.txt"), "a\nB\nc\n"); // moved AND edited → not byte-identical
+  const src = await buildDiffSource({ mode: "repo", root: dir });
+  assert.ok(src);
+  assert.deepEqual(src!.files.map((f) => f.path).sort(), ["new.txt", "old.txt"]);
+  assert.equal(src!.files.find((f) => f.path === "old.txt")!.newPath, undefined); // deletion
+  assert.equal(src!.files.find((f) => f.path === "new.txt")!.oldFile.contents, ""); // untracked add
+  rmSync(dir, { recursive: true, force: true });
+});
+
+test("mode-only and same-path binary changes still produce no review entry", async () => {
+  const { dir } = freshRepo(false);
+  const g = (args: string[]) => execFileSync("git", args, { cwd: dir }).toString();
+  const shPath = path.join(dir, "s.sh");
+  const binPath = path.join(dir, "img.bin");
+  writeFileSync(shPath, "#!/bin/sh\necho hi\n");
+  writeFileSync(binPath, Buffer.from([0, 1, 2, 0, 255]));
+  g(["add", "."]);
+  g(["commit", "-qm", "add binary + script"]);
+  // A mode-only change on the script, and a byte change to the binary — both zero-hunk, same-path.
+  execFileSync("chmod", ["755", shPath], { cwd: dir });
+  writeFileSync(binPath, Buffer.from([0, 9, 9, 9, 255]));
+  const src = await buildDiffSource({ mode: "repo", root: dir });
+  assert.equal(src, null); // no text hunks and same-path → both drop; nothing else → null
+  rmSync(dir, { recursive: true, force: true });
+});

src/git.test.ts

@@ -37,3 +37,66 @@ test("a pure addition keys on the deletions side count of zero", () => {
   const blocks = changeBlocks(parseUnifiedDiff(diff)[0]!.hunks[0]!);
   assert.equal(changeStableKeyFromBlock(blocks[0]!), "additions:4:0:1");
 });
+
+// ── git -M rename handling (issue 01) ────────────────────────────────────────
+
+test("a pure rename (zero hunks, distinct paths) is kept, from the rename headers", () => {
+  const diff = `diff --git a/old name.txt b/new name.txt
+similarity index 100%
+rename from old name.txt
+rename to new name.txt
+`;
+  const files = parseUnifiedDiff(diff);
+  assert.equal(files.length, 1); // NOT filtered out despite zero hunks
+  // Paths come from the rename headers, so spaces survive (the diff --git regex would mangle them).
+  assert.equal(files[0]!.oldPath, "old name.txt");
+  assert.equal(files[0]!.newPath, "new name.txt");
+  assert.equal(files[0]!.hunks.length, 0);
+});
+
+test("a rename+edit keeps distinct paths AND its hunk", () => {
+  const diff = `diff --git a/old.txt b/new.txt
+similarity index 80%
+rename from old.txt
+rename to new.txt
+index 1111111..2222222 100644
+--- a/old.txt
++++ b/new.txt
+@@ -1,2 +1,2 @@
+ a
+-b
++B
+`;
+  const files = parseUnifiedDiff(diff);
+  assert.equal(files.length, 1);
+  assert.equal(files[0]!.oldPath, "old.txt");
+  assert.equal(files[0]!.newPath, "new.txt");
+  assert.equal(files[0]!.hunks.length, 1);
+});
+
+test("a mode-only change (zero hunks, same path) stays dropped", () => {
+  const diff = `diff --git a/s.sh b/s.sh
+old mode 100644
+new mode 100755
+`;
+  assert.equal(parseUnifiedDiff(diff).length, 0);
+});
+
+test("a same-path binary diff stays dropped", () => {
+  const diff = `diff --git a/img.png b/img.png
+index 1111111..2222222 100644
+Binary files a/img.png and b/img.png differ
+`;
+  assert.equal(parseUnifiedDiff(diff).length, 0);
+});
+
+test("a renamed binary (distinct paths + Binary line) is dropped, not read as text", () => {
+  const diff = `diff --git a/old.png b/new.png
+similarity index 60%
+rename from old.png
+rename to new.png
+index 1111111..2222222 100644
+Binary files a/old.png and b/new.png differ
+`;
+  assert.equal(parseUnifiedDiff(diff).length, 0);
+});

src/git.ts

@@ -48,6 +48,15 @@ export async function getBranch(cwd: string) {
 
 export function parseUnifiedDiff(raw: string): DiffFile[] {
   const files: DiffFile[] = [];
+  // Files whose content section is binary ("Binary files … differ" / "GIT binary patch") — a
+  // transient parse-time set (never a DiffFile field, so it can't leak into ReviewFile). Used
+  // to drop a renamed binary: it's zero-hunk with distinct paths, so the rename-keep rule below
+  // would otherwise keep it and its bytes would later be read as mangled utf8 text (fileAt).
+  const binaryFiles = new WeakSet<DiffFile>();
+  // Files whose paths came from the `rename from`/`rename to` extended headers — those give the
+  // raw, unprefixed path (reliable for paths with spaces, unlike the `diff --git`/`--- +++`
+  // regexes), so once set we don't let the later `--- a/`/`+++ b/` lines clobber them.
+  const renamedFiles = new WeakSet<DiffFile>();
   let file: DiffFile | undefined;
   let hunk: DiffHunk | undefined;
   let oldLine = 0;
@@ -68,12 +77,30 @@ export function parseUnifiedDiff(raw: string): DiffFile[] {
       continue;
     }
     if (!file) continue;
+    // git -M rename headers. Authoritative over the diff --git / --- +++ paths (see above), and
+    // they arrive BEFORE any hunk, so they must be handled ahead of the `!hunk` guard below.
+    if (rawLine.startsWith("rename from ")) {
+      file.oldPath = rawLine.slice("rename from ".length);
+      renamedFiles.add(file);
+      continue;
+    }
+    if (rawLine.startsWith("rename to ")) {
+      file.newPath = rawLine.slice("rename to ".length);
+      renamedFiles.add(file);
+      continue;
+    }
+    if (rawLine.startsWith("Binary files ") || rawLine.startsWith("GIT binary patch")) {
+      binaryFiles.add(file);
+      continue;
+    }
     if (rawLine.startsWith("--- ")) {
+      if (renamedFiles.has(file)) continue;
       const value = rawLine.slice(4).trim();
       file.oldPath = value === "/dev/null" ? undefined : value.replace(/^a\//, "");
       continue;
     }
     if (rawLine.startsWith("+++ ")) {
+      if (renamedFiles.has(file)) continue;
       const value = rawLine.slice(4).trim();
       file.newPath = value === "/dev/null" ? undefined : value.replace(/^b\//, "");
       continue;
@@ -119,7 +146,16 @@ export function parseUnifiedDiff(raw: string): DiffFile[] {
     }
   }
 
-  return files.filter((f) => f.hunks.length > 0);
+  // Keep every file that has real hunks, plus a zero-hunk PURE rename (git -M with 100%
+  // similarity emits no content) — distinct old/new paths and not binary. A renamed binary is
+  // also zero-hunk with distinct paths, but its "Binary files … differ" line marks it (excluded
+  // so its bytes aren't read as text); same-path zero-hunk sections (mode-only changes, same-path
+  // binary diffs) have no rename to surface and stay dropped.
+  return files.filter(
+    (f) =>
+      f.hunks.length > 0 ||
+      (!!f.oldPath && !!f.newPath && f.oldPath !== f.newPath && !binaryFiles.has(f)),
+  );
 }
 
 export async function fileAt(root: string, rel: string | undefined, ref?: string) {