Skip to content

Commit 3874a91

Browse files
ymansurozerclaude
andcommitted
feat: skimmable changes — guide-driven focused reviews
The agent's guide can now mark parts of the diff as skimmable for a focused review ("ignore pure import churn, show me the real changes"), replacing the synthetic-branch workaround. GuideFile gains skim/ skimReason (whole file) and skimBlocks (new-side line spans the server resolves to change blocks and stamps onto ChangeState). Validation rejects an unresolvable span naming the file and span on attach; a reload carrying the guide forward silently drops stale spans instead — a rewritten block deserves fresh attention. Fully-skimmed files (file-level flag, or every block stamped) leave the reviewer's flow entirely: they gather under a collapsed "Skimmed · N files" group at the bottom of the tree, walkthrough, and Overview, and carry no weight in progress, review-complete, or the wrap/approve-advance seeks. They are never auto-approved — approvedFiles still means the reviewer signed off. Opening one is a deliberate look, so its diff renders expanded; skimmed blocks inside in-flow files collapse to one-line strips ("N skimmed lines · reason") that expand on click, in both split and stacked views. Approving a file accepts pending skimmed blocks exactly like visible ones. Everything is display-only and derived per render, so a reload that drops a stamp returns the file to the flow automatically. The contract documents the fields and their intent: skim lowers attention, flag raises it — never skim your own risky changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jcfPg9g4TTWQksf8vhHps
1 parent f8427cd commit 3874a91

28 files changed

Lines changed: 1378 additions & 58 deletions

scripts/smoke.mjs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,48 @@ try {
6969
assert.ok(state.changes.length >= 1, "desk has a change to review");
7070
console.log(`✓ desk up — repo mode, ${state.changes.length} change(s)`);
7171

72+
// guided review with skim (issue 06): attach a guide (OUTSIDE the repo so it isn't a stray
73+
// untracked file) whose skimBlocks span resolves to the change block, and a file-level skim.
74+
const guideDir = mkdtempSync(path.join(tmpdir(), "galley-smoke-guide-"));
75+
const guidePath = path.join(guideDir, "guide.json");
76+
writeFileSync(
77+
guidePath,
78+
JSON.stringify({
79+
overview: "Smoke overview.",
80+
files: [
81+
{
82+
path: "a.txt",
83+
orientation: "The changed file.",
84+
skim: true,
85+
skimReason: "smoke",
86+
skimBlocks: [{ lines: 2, reason: "line-two churn" }],
87+
},
88+
],
89+
}),
90+
);
91+
const reloaded = JSON.parse(cli("reload", "--repo", tmp, "--session", ID, "--guide", guidePath));
92+
assert.ok(reloaded.ok, "reload with a skim guide accepted");
93+
const guidedState = await getJson("/api/state");
94+
assert.equal(guidedState.guide?.files?.[0]?.skim, true, "file-level skim survives to state");
95+
assert.ok(
96+
guidedState.changes.some((c) => c.skim),
97+
"a skimBlocks span stamped a change block",
98+
);
99+
console.log("✓ reload --guide with skim → accepted, file + block stamped");
100+
101+
// A skim span that resolves to no change block is rejected (diff-aware validation).
102+
const badReload = await post("/api/reload", {
103+
guide: {
104+
overview: "x",
105+
files: [{ path: "a.txt", orientation: "s", skimBlocks: [{ lines: 99 }] }],
106+
},
107+
});
108+
assert.equal(badReload.status, 422, "unresolvable skim span rejected");
109+
const badBody = await badReload.json();
110+
assert.ok(badBody.error?.includes("a.txt"), "rejection names the offending file");
111+
rmSync(guideDir, { recursive: true, force: true });
112+
console.log("✓ reload with an unresolvable skim span → 422 naming the entry");
113+
72114
// human asks a question → `galley await` yields a question event
73115
await post("/api/ask", {
74116
path: "a.txt",

src/cli.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
mergeReviewState,
1717
persistReview,
1818
readDeskLock,
19+
resolveSkim,
1920
reviewDir,
2021
sanitizeSession,
2122
stablePort,
@@ -268,7 +269,7 @@ function loadGuideArg(value: string | boolean | undefined) {
268269
return null;
269270
}
270271
const SCHEMA =
271-
"Expected JSON: { overview, prDescription?, files: [{ path, order, category, orientation, flag? }] }.";
272+
"Expected JSON: { overview, prDescription?, files: [{ path, order, category, orientation, flag?, skim?, skimReason?, skimBlocks? }] }.";
272273
let parsed: unknown;
273274
try {
274275
parsed = JSON.parse(readFileSync(value, "utf8"));
@@ -444,6 +445,19 @@ async function runDesk(
444445
// Stamp the diff hash the guide was generated against; if a later reload advances the
445446
// diff past it, the desk flags the guide as possibly stale (slice 05).
446447
state.guide = { ...guide, baseDiffHash: state.baseDiffHash };
448+
// Resolve skim spans against the fresh diff and stamp the collapsed blocks. Strict at
449+
// initial attach: an unresolvable span aborts the launch naming the offending field, like
450+
// any other invalid-guide input.
451+
const skim = resolveSkim(state.rawDiff, state.changes, state.guide, { strict: true });
452+
if (!skim.ok) {
453+
console.error(`Invalid guide: ${skim.reason}.`);
454+
process.exitCode = 1;
455+
return;
456+
}
457+
} else if (state.guide) {
458+
// A guide carried forward by the merge (restart without --guide): re-resolve leniently
459+
// against the new diff — stale spans drop rather than abort (see resolveSkim).
460+
resolveSkim(state.rawDiff, state.changes, state.guide, { strict: false });
447461
}
448462
if (mode === "repo") await syncGitState(state);
449463
await persistReview(state);

src/guide.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,48 @@ test("validateGuide defaults missing order (by position) and category", () => {
4343
assert.equal(r.guide.files[0]!.flag, undefined);
4444
});
4545

46+
test("validateGuide accepts skim fields and normalizes skimBlocks lines", () => {
47+
const r = validateGuide({
48+
overview: "o",
49+
files: [
50+
{
51+
path: "a.ts",
52+
orientation: "s",
53+
skim: true,
54+
skimReason: "generated",
55+
skimBlocks: [{ lines: 5, reason: "import" }, { lines: [12, 10] }],
56+
},
57+
],
58+
});
59+
assert.ok(r.ok);
60+
if (!r.ok) return;
61+
const f = r.guide.files[0]!;
62+
assert.equal(f.skim, true);
63+
assert.equal(f.skimReason, "generated");
64+
// a bare number normalizes to [n, n]; a reversed pair is ordered ascending.
65+
assert.deepEqual(f.skimBlocks, [{ lines: [5, 5], reason: "import" }, { lines: [10, 12] }]);
66+
});
67+
68+
test("validateGuide leaves skim fields unset when absent", () => {
69+
const r = validateGuide({ overview: "o", files: [{ path: "a.ts", orientation: "s" }] });
70+
assert.ok(r.ok);
71+
if (!r.ok) return;
72+
const f = r.guide.files[0]!;
73+
assert.equal(f.skim, undefined);
74+
assert.equal(f.skimReason, undefined);
75+
assert.equal(f.skimBlocks, undefined);
76+
});
77+
78+
test("validateGuide rejects malformed skimBlocks", () => {
79+
const bad = (skimBlocks: unknown) =>
80+
validateGuide({ overview: "o", files: [{ path: "a.ts", orientation: "s", skimBlocks }] }).ok;
81+
assert.equal(bad("nope"), false); // not an array
82+
assert.equal(bad([null]), false); // entry not an object
83+
assert.equal(bad([{ lines: "x" }]), false); // lines not a number/pair
84+
assert.equal(bad([{ lines: [1] }]), false); // wrong-length pair
85+
assert.equal(bad([{ lines: [1, 2, 3] }]), false); // wrong-length pair
86+
});
87+
4688
test("validateGuide rejects malformed input", () => {
4789
const cases: unknown[] = [
4890
null,

src/guide.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,45 @@ export function validateGuide(input: unknown): GuideValidation {
3232
orientation: f.orientation,
3333
};
3434
if (typeof f.flag === "string" && f.flag.trim()) file.flag = f.flag;
35+
// Skim fields (focused review). File-level `skim`/`skimReason` collapse the whole file;
36+
// `skimBlocks` are new-side line spans. This is SHAPE validation only — whether a span
37+
// actually resolves to a change block is diff-aware and checked later (resolveSkim in
38+
// state.ts), because validateGuide is pure (no diff in hand).
39+
if (f.skim === true) file.skim = true;
40+
if (typeof f.skimReason === "string" && f.skimReason.trim()) file.skimReason = f.skimReason;
41+
if (f.skimBlocks !== undefined) {
42+
if (!Array.isArray(f.skimBlocks))
43+
return { ok: false, reason: `guide.files[${i}].skimBlocks must be an array` };
44+
const blocks: NonNullable<GuideFile["skimBlocks"]> = [];
45+
for (let j = 0; j < f.skimBlocks.length; j++) {
46+
const rawBlock = f.skimBlocks[j] as Record<string, unknown> | null | undefined;
47+
if (!rawBlock || typeof rawBlock !== "object")
48+
return { ok: false, reason: `guide.files[${i}].skimBlocks[${j}] must be an object` };
49+
// `lines` is a [start, end] span or a bare number (a single line, normalized to [n, n]).
50+
const raw = rawBlock.lines;
51+
let span: [number, number] | null = null;
52+
if (typeof raw === "number" && Number.isFinite(raw)) span = [raw, raw];
53+
else if (
54+
Array.isArray(raw) &&
55+
raw.length === 2 &&
56+
typeof raw[0] === "number" &&
57+
typeof raw[1] === "number" &&
58+
Number.isFinite(raw[0]) &&
59+
Number.isFinite(raw[1])
60+
)
61+
span = raw[0] <= raw[1] ? [raw[0], raw[1]] : [raw[1], raw[0]];
62+
if (!span)
63+
return {
64+
ok: false,
65+
reason: `guide.files[${i}].skimBlocks[${j}].lines must be a line number or a [start, end] pair`,
66+
};
67+
const block: { lines: [number, number]; reason?: string } = { lines: span };
68+
if (typeof rawBlock.reason === "string" && rawBlock.reason.trim())
69+
block.reason = rawBlock.reason;
70+
blocks.push(block);
71+
}
72+
if (blocks.length) file.skimBlocks = blocks;
73+
}
3574
files.push(file);
3675
}
3776
files.sort((a, b) => a.order - b.order);

src/server.ts

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
persistReview,
2121
questionPayload,
2222
readGlobalSettings,
23+
resolveSkim,
2324
syncGitState,
2425
writeGlobalSettings,
2526
} from "./state.js";
@@ -441,10 +442,27 @@ export async function startServer(options: ServerOptions): Promise<ServerHandle>
441442
await persistReview(state);
442443
return json(res, 200, { ok: true, empty: true, baseDiffHash: state.baseDiffHash });
443444
}
444-
Object.assign(state, mergeReviewState(base, state));
445-
// The merge carries the old guide forward; a provided guide replaces it,
446-
// stamped against the just-rebuilt diff so it isn't born stale.
447-
if (validatedGuide) state.guide = { ...validatedGuide, baseDiffHash: state.baseDiffHash };
445+
// The merge carries the old guide forward; a provided guide replaces it, stamped
446+
// against the just-rebuilt diff so it isn't born stale. Resolve skim spans + reject a
447+
// bad NEW guide (strict) BEFORE committing the merge onto the live state, so a rejected
448+
// reload leaves the desk untouched. A guide carried forward re-resolves leniently —
449+
// stale spans drop, they never fail a reload (see resolveSkim's strict/lenient split).
450+
const merged = mergeReviewState(base, state);
451+
if (validatedGuide) {
452+
merged.guide = { ...validatedGuide, baseDiffHash: merged.baseDiffHash };
453+
const skim = resolveSkim(merged.rawDiff, merged.changes, merged.guide, { strict: true });
454+
if (!skim.ok)
455+
return fail(
456+
res,
457+
422,
458+
"INVALID_GUIDE",
459+
`Invalid guide: ${skim.reason}.`,
460+
"Run `galley spec` for the guided-review schema.",
461+
);
462+
} else if (merged.guide) {
463+
resolveSkim(merged.rawDiff, merged.changes, merged.guide, { strict: false });
464+
}
465+
Object.assign(state, merged);
448466
await syncGitState(state);
449467
await persistReview(state);
450468
return json(res, 200, { ok: true, empty: false, baseDiffHash: state.baseDiffHash });

src/spec.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ const ANCHORS = [
3838
"files (required, non-empty array)",
3939
"repo-relative; must be a file in the diff",
4040
"Orientation, not a changelog",
41+
// skimmable review (issue 06) — the fields and the "only on request / opposite of flag" rule
42+
"skimBlocks?",
43+
"new-file-side [start, end] span",
44+
"Skim LOWERS attention",
45+
"When to skim",
46+
// fully-skimmed files leave the flow (issue 07)
47+
"drops into a collapsed",
48+
"genuinely needs no eyes",
4149
// the rest of the operational contract
4250
"reload vs restart",
4351
"desk.lock",

src/spec.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,27 @@ One JSON object:
143143
label repeated non-adjacently makes a second section — keep a category's files together.
144144
- flag? — raises a flag on the file for closer scrutiny; the text is the note (what to
145145
double-check, what's risky). Omit unless the file genuinely warrants it.
146+
- skim? / skimReason? — mark the WHOLE file skimmable: the desk collapses its diff behind a
147+
one-click "expand" strip and shows a muted indicator in the tree/walkthrough. skimReason is a
148+
short why ("generated", "lockfile churn"). The reviewer can always expand — nothing is hidden.
149+
- skimBlocks? — collapse PARTS of the file: an array of { lines, reason? } where lines is a
150+
new-file-side [start, end] span (or a single line number) of the diff you read. The server
151+
resolves each span to the enclosing change block(s) and collapses them behind an expandable
152+
strip. reason is a short label ("import-only"). A span that resolves to no change block aborts
153+
the launch (see Validation).
154+
When to skim: ONLY when the reviewer asked for a focused review ("ignore the import churn, show
155+
me the real changes"). Skim LOWERS attention — it is the opposite of flag, which raises it. Never
156+
skim your own risky or non-obvious changes to slip them past review; skim boilerplate the
157+
reviewer told you they don't want to see. A file skimmed whole (or every block skimmed) leaves
158+
the reviewer's default flow entirely — it drops into a collapsed "Skimmed" group and carries no
159+
progress or completion weight — so skim only what genuinely needs no eyes. On reload, skimBlocks
160+
re-resolve against the new diff; a span that no longer resolves is dropped (a block you rewrote
161+
deserves fresh attention).
146162
Validation: overview a non-empty string, files a non-empty array, every file a non-empty
147-
path+orientation; an unreadable file / invalid JSON / schema violation aborts the launch naming
148-
the offending field.
163+
path+orientation; skimBlocks (if present) an array of { lines: number | [start, end], reason? };
164+
an unreadable file / invalid JSON / schema violation — or a skimBlocks span that matches no change
165+
block in the file (or names a file absent from the diff) — aborts the launch naming the offending
166+
field.
149167
150168
## Between rounds — reload vs restart, and the desk lock
151169
- Don't edit tracked files mid-round: the reviewer wouldn't see the edits and their in-flight

0 commit comments

Comments
 (0)