Fix five papercuts found by running the tool on itself

commit · HEAD^@f77caf8 → HEAD@77c6208 · claude-opus-5 (cursor cloud agent)

94%84% verified

Five unrelated small fixes, all found by using this tool on its own output rather than by reading the code again.

This review predates the rename to whymark, and the diff below is left as it was recorded: the modules appear at their old paths under src/lib/crev/, and the blob hashes pin to the commit as it was made. Rewriting a diff to match a later name would break the one property that makes a review worth reading.

Writing the first example review surfaced two output bugs: a plain modified file was serialised with from=<same path>, which made a round-trip through the parser report it as renamed; and crev prompt was emitting the human-facing preamble of the prompt template to an agent that only needed the instructions. The CLI also printed "Try --staged" when the user had just passed --staged, and --staged reviews silently included untracked files, which are by definition not staged. Finally, npm run lint rejected the useMediaQuery hook, so it is now built on useSyncExternalStore.

No behaviour in the format itself changed; the parser and serializer fixes only affect what a generated document says about a file.

intentwhole change90%

This review exists to show what the format looks like when it is written about real, boring work rather than a showcase change. Every source: below is either a command whose output I read or an honest inference.

"either create file directly with AI, so I need a prompt or a skill, or generate it just based on code changes"
— the request this repo answers

passedcommand
$ npm test
exit 0 in 0.6s

Explained

48 of 51 added lines

84%

verified

sourced

inferred

stubs

high risk

todos

questions

npm test
exit 0 in 0.6s
pass
npm run lint
exit 0 in 2.4s
pass
npm run typecheck
exit 0 in 1.4s
pass

files

cli/crev.ts+22 −791%crev/use-media-query.ts+15 −1093%crev/git.ts+8 −5100%crev/parse.ts+3 −2100%crev/serialize.ts+3 −1100%

annotations15 of 15 shown · j k to step through

src/cli/crev.ts

+22 −7read-only591%

@@ -211,9 +211,7 @@function buildDoc(args: Args) {

211211

212212 function cmdNew(args: Args) {

213213 const { doc, diff, cwd } = buildDoc(args);

214− if (!diff.files.length) {

215− fail(`No changes found for scope \`${doc.meta.scope}\`. Try --staged or --branch.`);

216− }

214+ if (!diff.files.length) noChanges(doc.meta.scope);

217215

218216 const text = serializeCrev(doc);

219217 const out = str(args.flags.get("out")) ?? str(args.flags.get("o"));

@@ -248,15 +246,24 @@function cmdNew(args: Args) {

248246 );

249247 }

250248

249+function noChanges(scope: Scope | undefined): never {

250+ const others = ["--staged", "--unstaged", "--branch main", "--commit HEAD"].filter(

251+ (flag) => !flag.includes(String(scope)),

252+ );

253+ fail(

254+ `No changes in scope \`${scope}\`${

255+ "" // keep the hint on one line

256+ }. Nothing to review — try ${others.slice(0, 3).join(", ")}, or pass --path.`,

257+ );

258+}

259+

251260 function cmdPrompt(args: Args) {

252261 const { doc, diff, cwd } = buildDoc(args);

253− if (!diff.files.length) {

254− fail(`No changes found for scope \`${doc.meta.scope}\`. Try --staged or --branch.`);

255− }

262+ if (!diff.files.length) noChanges(doc.meta.scope);

256263 const skeleton = serializeCrev(doc);

257264 const templatePath = join(cwd, "prompts", "crev-author.md");

258265 const template = existsSync(templatePath)

259− ? readFileSync(templatePath, "utf8")

266+ ? stripPreamble(readFileSync(templatePath, "utf8"))

260267 : FALLBACK_PROMPT;

261268 process.stdout.write(

262269 template.replace("{{SKELETON}}", skeleton.trimEnd()).replace(

@@ -266,6 +273,14 @@function cmdPrompt(args: Args) {

266273 );

267274 }

268275

276+/** The template file explains itself to a human above the first `---`; the

277+ * agent only needs what comes after it. */

278+function stripPreamble(template: string): string {

279+ const lines = template.split("\n");

280+ const separator = lines.findIndex((line) => line.trim() === "---");

281+ return separator === -1 ? template : lines.slice(separator + 1).join("\n").trimStart();

282+}

283+

269284 const FALLBACK_PROMPT = `Fill in this CREV review of your own changes. Replace every TODO.

270285 For each annotation give: why the code is that way, a \`source:\` for the evidence

271286 (use \`inference\` when there was none), and a \`verify:\` claim naming the exact

intent+21495%

Both commands failed with the same hand-written message, and that message told the reader to "Try --staged or --branch" even when they had just passed --staged — the one suggestion guaranteed to be useless. Routing both through one helper means the advice can be computed from the scope that actually failed.

no external source — inferred
I hit this myself running crev new --staged in a clean tree

passedcommand
$ npm run typecheck
exit 0 in 1.4s
unverifiedchecked by hand
"crev new --staged in a clean tree now suggests --branch main, --commit HEAD, --path" => pass

intent+249..25890%

The suggestions are filtered against the scope that failed, so the error never recommends the thing the user just tried. Three are shown rather than all four because a one-line hint that lists every possibility reads as noise.

no external source — inferred
no convention for CLI error copy exists in this repo yet; this is the first place that needed one

passedchecked by hand
error text for --staged omits --staged

rejected throwing a typed error and formatting it at the top level — rejected, the CLI has exactly two call sites and no other error needs the treatment

note+26295%

The second call site, identical to the first. Kept as a one-liner so the two commands visibly share the behaviour.

passedcommand
$ npm run typecheck
exit 0 in 1.4s

intent+266low risk85%

crev prompt pipes its output straight into an agent, but the template file starts with a paragraph addressed to a human ("Paste everything below into any coding agent"), which the agent would then have read as part of its instructions.

prompts/crev-author.md:1
the preamble, ending at the first ---

unverifiedchecked by hand
"crev prompt --commit <sha> now begins at 'Below is a CREV skeleton'" => pass

intent+276..28280%

The split is on the first line that is exactly --- because the template is markdown, where a horizontal rule is the natural boundary between "notes for the reader" and "the prompt itself". Falling back to the whole file keeps a template with no rule usable rather than emitting nothing.

prompts/crev-author.md:11
the separator this relies on

passedcommand
$ npm run typecheck
exit 0 in 1.4s

if a second consumer of the template appears, this belongs next to the template rather than in the CLI

src/components/crev/use-media-query.ts

+15 −10read-only293%

@@ -1,17 +1,22 @@

11 "use client";

3−import { useEffect, useState } from "react";

3+import { useCallback, useSyncExternalStore } from "react";

5+/**

6+ * `fallback` is what the server renders, so pick the value that gives the

7+ * layout the review page should have before hydration.

8+ */

59 export function useMediaQuery(query: string, fallback = false): boolean {

6− const [matches, setMatches] = useState(fallback);

10+ const subscribe = useCallback(

11+ (onChange: () => void) => {

12+ const list = window.matchMedia(query);

13+ list.addEventListener("change", onChange);

14+ return () => list.removeEventListener("change", onChange);

15+ },

16+ [query],

17+ );

718

8− useEffect(() => {

9− const list = window.matchMedia(query);

10− setMatches(list.matches);

11− const handler = (event: MediaQueryListEvent) => setMatches(event.matches);

12− list.addEventListener("change", handler);

13− return () => list.removeEventListener("change", handler);

14− }, [query]);

19+ const getSnapshot = useCallback(() => window.matchMedia(query).matches, [query]);

1520

16− return matches;

21+ return useSyncExternalStore(subscribe, getSnapshot, () => fallback);

1722 }

assumption+5..860%

The comment is the whole point of the third argument: the server renders fallback, so a wrong fallback shows the mobile layout for a frame on a desktop. The review page passes true for that reason.

no external source — inferred
no hydration convention is written down in this repo

unverifiedchecked by hand
no hydration warning in the dev console on the review page
pass

intent+10..21low risk90%

The first version set state inside an effect to seed the initial match, which npm run lint rejects (react-hooks/set-state-in-effect) because it causes a second render pass on every mount. useSyncExternalStore is the API built for exactly this: subscribe, read, and a separate server snapshot.

react.dev/reference/react/useSyncExternalStore
the getServerSnapshot argument is what makes this SSR-safe
src/components/crev/review-view.tsx:38
the only caller, which needs a value on the very first render to choose the layout

passedcommand
$ npm run lint
exit 0 in 2.4s
passedcommand
$ npm run build
exit 0 in 3.6s

rejected keeping useState and reading matchMedia lazily in the initialiser — rejected, that runs on the server where matchMedia does not exist

src/lib/crev/git.ts

+8 −5read-only4100%

@@ -2,7 +2,6 @@import { execFileSync, spawnSync } from "node:child_process";

22 import {

33 type CrevDocument,

44 type FileSection,

5− type FileStatus,

65 type Hunk,

76 type Note,

87 type Scope,

@@ -155,6 +154,8 @@export function collectDiff(request: DiffRequest): DiffResult {

155154 const raw = git(args, { cwd });

156155 const files = parseUnifiedDiff(raw);

157156

158− if (request.untracked && request.scope !== "commit" && request.scope !== "branch") {

157+ // Untracked files live in the working tree only, so they belong to the

158+ // working-tree scopes and would be a lie in a `--staged` or `--branch` review.

159+ if (request.untracked && (request.scope === "unstaged" || request.scope === "worktree")) {

159160 files.push(...collectUntracked(context, cwd, request.paths));

160161 }

@@ -192,7 +193,7 @@function collectUntracked(

192193 section.path = path;

193194 section.oldPath = undefined;

194195 section.status = "added";

195− section.newSha = hashObject(path, cwd) ?? undefined;

196+ section.newSha = hashObject(path, cwd, 7) ?? undefined;

196197 out.push(section);

197198 }

198199 return out;

@@ -480,7 +481,9 @@function titleFor(scope: Scope, diff: DiffResult): string {

480481 }

481482

482483 /** Current blob hash of a working-tree file, for staleness detection. */

483−export function hashObject(path: string, cwd?: string): string | null {

484+export function hashObject(path: string, cwd?: string, abbrev = 0): string | null {

484485 const out = tryGit(["hash-object", "--", path], { cwd });

485− return out ? out.trim() : null;

486+ if (!out) return null;

487+ const sha = out.trim();

488+ return abbrev ? sha.slice(0, abbrev) : sha;

486489 }

note-595%

Unused since the file's FileSection factory stopped taking a status argument; npm run lint reported it.

passedcommand
$ npm run lint
exit 0 in 2.4s

intent+157..159medium risk90%

Untracked files exist only in the working tree, so including them in a --staged review claimed that files which are not in the index are part of the staged change. The condition now names the two scopes where they belong instead of excluding the two where they do not, which is also correct for any scope added later.

no external source — inferred
found by running crev new --staged while three new files were untracked and seeing them in the output

passedcommand
$ npm test
exit 0 in 0.6s
unverifiedchecked by hand
"crev new --staged in a tree with untracked files reports no changes; --worktree lists them" => pass

an agent reviewing its own staged work no longer has to explain files it has not staged

note+19685%

git's own index line gives abbreviated hashes, so a document mixing the two sources had 7-character hashes for tracked files and 40-character ones for untracked files. Staleness comparison is prefix-based either way; this is purely so the generated file reads consistently.

src/lib/crev/validate.ts:104
the comparison that tolerates either length

passedcommand
$ npm test
exit 0 in 0.6s

intent+484..48890%

abbrev defaults to 0 so the existing caller in the validator keeps getting the full hash it compares against, and only the generator asks for a short one.

src/lib/crev/validate.ts:104
the caller that must not be truncated

passedcommand
$ npm test
exit 0 in 0.6s
passedtypes

src/lib/crev/parse.ts

+3 −2read-only2100%

@@ -3,7 +3,6 @@import {

33 CREV_VERSION,

44 NOTE_KINDS,

55 VERIFY_METHODS,

6− type Check,

76 type CrevDocument,

87 type Diagnostic,

98 type DiffLine,

@@ -477,6 +476,8 @@function parseFileDirective(

477476 });

478477 }

479478

480− if (!sawStatus && section.oldPath) section.status = "renamed";

479+ if (!sawStatus && section.oldPath && section.oldPath !== section.path) {

480+ section.status = "renamed";

481+ }

481482 return section;

482483 }

note-695%

Check moved to the serializer when @check lines started being merged into frontmatter; the import outlived the use. Reported by lint.

passedcommand
$ npm run lint
exit 0 in 2.4s

intent+479..481medium risk90%

This is the parser half of a round-trip bug. parseUnifiedDiff fills oldPath from the --- a/path line of every diff, including plain modifications, the serializer then wrote from=<same path>, and re-parsing that file inferred renamed from the presence of from=. A file that was only edited came back as a rename.

no external source — inferred
noticed in the @file lines of a generated skeleton, not from a failing test
tests/parse.test.ts:112
the round-trip test, which passed throughout because its fixture has no rename

passedcommand
$ npm test
exit 0 in 0.6s
passedchecked by hand
crev new --worktree emits no from= for modified files

add a fixture that round-trips a rename and a modification together, so the next regression here fails a test instead of being spotted by eye

src/lib/crev/serialize.ts

+3 −1read-only1100%

@@ -85,7 +85,9 @@export function serializeFileHeader(file: FileSection): string {

8585 const parts = ["@file", quoteIfNeeded(file.path), file.status];

8686 if (file.added) parts.push(`+${file.added}`);

8787 if (file.removed) parts.push(`-${file.removed}`);

88− if (file.oldPath) parts.push(`from=${quoteIfNeeded(file.oldPath)}`);

88+ if (file.oldPath && file.oldPath !== file.path) {

89+ parts.push(`from=${quoteIfNeeded(file.oldPath)}`);

90+ }

8991 if (file.oldSha) parts.push(`oldsha=${file.oldSha}`);

9092 if (file.newSha) parts.push(`newsha=${file.newSha}`);

9193 if (file.lang) parts.push(`lang=${file.lang}`);

intent+88..9095%

The serializer half of the same bug. Both halves are needed: the guard here stops the bad output being written, and the guard in the parser stops documents that already contain it being misread.

passedcommand
$ npm test
exit 0 in 0.6s

→ src/lib/crev/parse.ts:479