1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416import { test } from "node:test";
import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import { join } from "node:path";
import { parse } from "yaml";
// The CI reviewer is told two things that pull against each other: flag docs prose that is not
// concise plain language, and do not flag style. Both are true only because the second names the
// first as its one exception — remove that sentence and the prompt holds a rule and its own
// prohibition, which is the shape that produces neither behaviour reliably.
//
// So this pins the reconciliation rather than the wording. It reads the prompt GitHub actually
// hands the model, not the file's bytes: the clause lives in a block scalar, so a byte-level grep
// matches indentation that is not part of the string, and would also match a copy of the sentence
// in a comment where it binds nothing.
//
// CONTRIBUTING.md and docs/ci.md are in here too. CONTRIBUTING.md is where the rules are stated for
// humans and the prompt only restates them; docs/ci.md is where this workflow's behaviour is
// documented. Three copies of one file set is what drifts: a PR that adds a documented directory to
// one and not the others leaves the reviewer enforcing a different scope than the one contributors
// are told about, and nothing else in this repo would notice.
const ROOT = join(import.meta.dirname, "..");
const WORKFLOW = join(ROOT, ".github", "workflows", "code-review.yml");
const CONTRIBUTING = join(ROOT, "CONTRIBUTING.md");
const CI_DOC = join(ROOT, "docs", "ci.md");
// The file set CONTRIBUTING.md's Documentation section binds, spelled as the inline code span both
// documents use — a bare `docs` would match the word in prose.
//
// Pinned here as well as compared between the two files, and that is the point: the comparison
// alone is a LOWER bound, so a PR adding a path to both lists at once widens what the reviewer
// enforces with nothing failing. The prompt names CONTRIBUTING.md as the authority, so that
// section is the enforced scope at PR head, and widening it should be a deliberate edit to this
// line rather than a side effect of a docs change. Adding a code directory here would put runtime
// code under a prose rule.
const BOUND_FILES = ["`README.md`", "`agents/`", "`config.example.yaml`", "`docs/`"];
// The same set without its backticks, for the one place that has to recognise a path written as
// plain prose.
const BOUND_BARE = BOUND_FILES.map((s) => s.replaceAll("`", "")).sort();
// Every inline code span, sorted, deduplicated. Both lists are prose, so this reads what a
// contributor reads rather than a structure neither file has.
function codeSpans(text: string): string[] {
return [...new Set(text.match(/`[^`\n]+`/g) ?? [])].sort();
}
// A file or a directory, however it is spelled: a known extension, a trailing slash, or the bare word
// `agents`. `docs/ci.md` matches as a file and not also as a directory, because the lookahead requires
// the slash to end the token.
//
// Bare `docs` is deliberately NOT matched, and that asymmetry is measured rather than assumed. Stripped
// of their code spans, the four regions this file reads use `docs` as an ordinary word in 4 of 4 —
// starting with the prompt's own "Docs prose that is not concise plain language" — and `agents` in 0 of
// 4, where it appears only inside `agents/` or as the singular "agent prompts". So matching bare `docs`
// would fire on prose that binds nothing and the pin would be deleted by the first person it annoyed,
// while matching bare `agents` costs nothing. Both halves of that measurement are asserted below — the
// `docs` half as a floor, the `agents` half exactly — so a document that starts using `agents` as
// English fails a test instead of quietly making this regex wrong.
//
// The limit that remains, stated because this is where someone reaches for a wider regex: a scope
// member with no extension, no slash and no name this regex knows — "anything under src" — is
// invisible, and both readers accept an empty result. Reaching it needs a commit that widens the scope
// AND spells the new member that way.
const PATH_RE =
/\b[A-Za-z0-9_.-]+\.(?:md|ya?ml|ts|tsx|json|sh|mjs)\b|\b[a-z0-9_.-]+\/(?![/\w])|\b[Aa]gents\b/g;
// Path-like tokens that are NOT inside a code span, minus CONTRIBUTING.md itself.
//
// This exists because THREE of the four assertions below compare CODE SPANS, and a path written without
// backticks is invisible to those three: a fifth path added as prose leaves a four-span list that
// `deepEqual` still accepts, while the reviewer reads the sentence and enforces five. So rather than
// teach each comparison to read prose — which would then have to tell a scope member apart from a
// mention — every one of these regions is required to keep its paths in backticks, and a bare one
// fails with a message saying to backtick it.
//
// The fourth, CONTRIBUTING.md's automated-review bullet, DOES read prose, so invisibility is not its
// reason for carrying this check; uniform spelling is. That is why the message takes a `why` — a shared
// helper that states one reason for four call sites states a false one for the odd caller.
//
// CONTRIBUTING.md is excluded by name: it is the authority these regions cite, it is never a member
// of the set, and the citation is what makes the scope traceable.
function barePaths(text: string): string[] {
const outsideSpans = text.replace(/`[^`\n]+`/g, " ");
const hits = [...new Set(outsideSpans.match(PATH_RE) ?? [])];
return hits.filter((t) => t !== "CONTRIBUTING.md").sort();
}
// Assert a region spells its paths as inline code spans, like the other three copies of this set.
//
// `why` is a parameter because the reason differs by region and a message that states the wrong one is
// a message a reader cannot act on. Three of the four regions are compared with `codeSpans`, where a
// bare path is genuinely invisible; the fourth reads prose too, so its complaint is not invisibility.
function assertNoBarePaths(
region: string,
where: string,
why = "A bare path is invisible to the set comparison in this test, so a widening written as prose " +
"would pass — backtick it.",
): void {
assert.deepEqual(barePaths(region), [], `${where} spells every path as an inline code span. ${why}\n${region}`);
}
function reviewPrompt(): string {
const doc = parse(readFileSync(WORKFLOW, "utf8")) as {
jobs: Record<string, { steps?: { name?: string; with?: { prompt?: string } }[] }>;
};
const steps = Object.values(doc.jobs).flatMap((j) => j.steps ?? []);
const step = steps.find((s) => s.name === "Claude review");
assert.ok(step, "code-review.yml still has a step named `Claude review`");
const prompt = step.with?.prompt;
assert.ok(typeof prompt === "string" && prompt.length > 0, "and that step still carries a prompt");
return prompt;
}
// A markdown section of the prompt, by heading text, so an assertion cannot be satisfied by the
// same sentence appearing under a different heading. `## Do NOT flag` and `## Should flag ...` are
// the two that have to agree.
function section(text: string, headingStartsWith: string): string {
const lines = text.split("\n");
const start = lines.findIndex(
(l) => l.startsWith("## ") && l.slice(3).trim().startsWith(headingStartsWith),
);
assert.notEqual(start, -1, `the prompt still has a \`## ${headingStartsWith}\` section`);
const rest = lines.slice(start + 1);
const end = rest.findIndex((l) => l.startsWith("## "));
return (end === -1 ? rest : rest.slice(0, end)).join("\n");
}
// A phrase in a wrapped prompt is broken by a newline and the block scalar's own indentation, so
// every match here runs against text with its whitespace collapsed. Matching the raw string makes
// an assertion pass or fail on where the line happened to wrap.
function flat(text: string): string {
return text.replace(/\s+/g, " ").trim();
}
// One list item, from its `- ` to the next one. The unit matters: both assertions below are about
// what a single bullet says, and a bullet that leans on its neighbour for a caveat is read alone.
function bullet(text: string, containing: string): string {
const items = text.split(/\n(?=- )/).map(flat);
const hit = items.filter((i) => i.includes(containing));
assert.equal(
hit.length,
1,
`exactly one bullet contains ${JSON.stringify(containing)}; found ${hit.length}`,
);
return hit[0]!;
}
// The four regions that state the scope. Named once each, because the last test compares them as a
// set and a region sliced twice is a region that can drift between two assertions.
//
// None of these locators may contain `docs` or `agents`. The last test asserts how those two words are
// used in each region, and a locator carrying one means rewording a region trips the locator FIRST —
// so the failure arrives as "step 4 is missing" instead of as the reconsider-the-regex message that
// test exists to print. Three of the four read that way before this was noticed.
function promptProseBullet(): string {
return bullet(section(reviewPrompt(), "Should flag"), "that is not concise plain language");
}
// CONTRIBUTING.md's Documentation section, OPENING paragraph only, ended by `\n\n`: that paragraph is
// where the requirement states what it covers, and the rest of the section is free to name a file as
// an example without binding it.
function contributingBinding(): string {
const text = readFileSync(CONTRIBUTING, "utf8");
const start = text.indexOf("\n## Documentation\n");
assert.notEqual(start, -1, "CONTRIBUTING.md still has a `## Documentation` section");
const afterHeading = text.slice(start + "\n## Documentation\n".length);
const binding = afterHeading.slice(0, afterHeading.indexOf("\n\n", afterHeading.indexOf("\n") + 1));
assert.ok(binding.length > 0, "and that section still opens with a paragraph");
return binding;
}
function contributingReviewBullet(): string {
return bullet(readFileSync(CONTRIBUTING, "utf8"), "One exception:");
}
// The one numbered step in docs/ci.md that describes this check, `4.` to `5.`. Read as a step rather
// than by searching for a sentence: a copy of the file set is what is being pinned, and a search that
// misses would pass on an empty match.
function ciDocStep(): string {
const text = readFileSync(CI_DOC, "utf8");
const start = text.indexOf("\n4. Checks ");
assert.notEqual(start, -1, "docs/ci.md still describes this check as review step 4");
const rest = text.slice(start + 1);
const end = rest.indexOf("\n5. ");
assert.notEqual(end, -1, "and step 5 still follows it");
const step = rest.slice(0, end);
// The locator above is deliberately word-free, so it would select whatever step 4 happens to be if
// the steps were reordered. This is what keeps it honest, and it names the rule rather than the
// subject so that it is not itself a locator carrying `docs`.
assert.match(
step,
/plain-language requirement/,
`and step 4 is still the one describing the plain-language check, not a renumbered neighbour:\n${step}`,
);
return step;
}
test("the reviewer's docs-prose rule is scoped to the files CONTRIBUTING.md binds", () => {
const prose = promptProseBullet();
// Equality, not `includes` per path. A missing path is a scope contributors are promised and the
// reviewer never applies; an extra one is a scope nothing in CONTRIBUTING.md justifies, and only
// an exact comparison catches the second.
assert.deepEqual(
codeSpans(prose),
BOUND_FILES,
`the docs-prose bullet's file set matches BOUND_FILES exactly. If this widening is intended, ` +
`edit BOUND_FILES and CONTRIBUTING.md's Documentation section in the same commit:\n${prose}`,
);
assertNoBarePaths(prose, "the prompt's docs-prose bullet");
// Without a quotable anchor the finding is unfalsifiable, and an unfalsifiable style note is the
// thing the rule below exists to keep out of reviews.
assert.match(
prose,
/quote the sentence and name the rule/i,
`the bullet still requires the sentence quoted and the rule named:\n${prose}`,
);
});
test("the prompt's do-not-flag-style rule names the docs-prose exception", () => {
const style = bullet(section(reviewPrompt(), "Do NOT flag"), "Style, formatting, or naming");
assert.match(
style,
/docs-prose bullet above/,
`the style bullet points at the docs-prose bullet as its exception. Without that pointer the ` +
`prompt forbids what it asks for two sections earlier:\n${style}`,
);
// The exception has to stay narrow in the same breath, or "prose" widens into the formatting
// review this repo has deliberately never had.
for (const outOfScope of ["heading style", "line length"]) {
assert.ok(
style.includes(outOfScope),
`the style bullet still holds ${outOfScope} out of scope inside those files:\n${style}`,
);
}
});
test("CONTRIBUTING.md's Documentation section binds the same files the prompt does", () => {
const binding = contributingBinding();
assert.deepEqual(
codeSpans(binding),
BOUND_FILES,
`CONTRIBUTING.md's Documentation section binds exactly BOUND_FILES. The prompt treats this ` +
`section as the authority, so a path added here widens what the reviewer enforces — which is ` +
`a change to make deliberately, in the same commit as BOUND_FILES and the prompt:\n${binding}`,
);
assertNoBarePaths(binding, "CONTRIBUTING.md's Documentation section");
});
// The fourth place a reader meets this scope, and the one a contributor is likeliest to read: the
// bullet under CONTRIBUTING.md's "What the automated review will say". It now points at the
// Documentation section instead of repeating the list, which is why this test allows NO paths as well
// as all of them. A partial copy is the failure — four lists agreeing today and a widening commit
// updating three of them leaves this bullet promising the old, narrower scope.
test("CONTRIBUTING.md's automated-review bullet does not keep its own copy of the scope", () => {
const item = contributingReviewBullet();
// Backticks stripped, so this reads a list written as prose as well as a backticked one. Unlike the
// other three regions, which compare `codeSpans` and cannot see a prose path at all.
const paths = [...new Set([...(item.replaceAll("`", "").match(PATH_RE) ?? [])])]
.filter((t) => t !== "CONTRIBUTING.md")
.sort();
if (paths.length > 0 && JSON.stringify(paths) !== JSON.stringify(BOUND_BARE)) {
// Composed from what is MISSING and what is EXTRA rather than chosen by a two-way test with an
// `else`. Four shapes reach here and a catch-all names the first one anybody thought of: a partial
// copy, a copy that is complete but for a spelling, an OVER-WIDE copy, and a path that is not a copy
// of this set at all — a cross-reference like `docs/ci.md`. "A partial copy" was printed for the last
// two, one of which is the opposite of partial, and it named `ci.md` as a member of a scope the
// bullet never claimed.
const trim = (xs: string[]): string[] => [...new Set(xs.map((x) => x.replace(/\/$/, "")))].sort();
const have = trim(paths);
const want = trim(BOUND_BARE);
const missing = want.filter((x) => !have.includes(x));
const extra = have.filter((x) => !want.includes(x));
// TWO of the four branches want the copy GONE — a partial one and a cross-reference — and two do not.
// A complete copy is tolerated here, so the spelling branch asks only for the spelling; and BOTH of the
// over-wide branch's own remedies (widen the set everywhere, or drop the extra path) leave a complete
// copy that this test then accepts, verified by running the remedy. So this sentence belongs to the two
// branches that mean it, and the other two say what they mean themselves. Appended to all four it read
// "add the trailing slash … and the link is how this bullet is meant to carry it", and a reader cannot
// tell whether the assertion wants a slash or wants the list deleted.
const home =
`The Documentation section is where this set is defined, and the [Documentation](#documentation) ` +
`link is how this bullet is meant to carry it. `;
let why: string;
if (missing.length === 0 && extra.length === 0) {
why =
`this bullet names every bound path but spells at least one differently ` +
`(${paths.join(", ")} against ${BOUND_BARE.join(", ")}). Nothing is missing — add the trailing ` +
`slash, and the backticks with it if the member is bare, which is how the other three copies of ` +
`this set are written. A complete copy spelled that way is accepted here, so nothing needs ` +
`deleting. `;
} else if (missing.length === 0) {
why =
`this bullet names every bound path AND ${extra.join(", ")}, so it is an OVER-WIDE copy, not a ` +
`partial one. If the scope really did widen, BOUND_FILES and the Documentation section are where ` +
`that happens and all four copies move together; if ${extra.join(", ")} is a cross-reference ` +
`rather than a scope member, drop it. Both of those leave a complete copy here, which this test ` +
`accepts, so neither asks you to delete the list — and if you did not mean to keep one, the ` +
`[Documentation](#documentation) link is how this bullet carries the set instead. `;
} else if (missing.length === want.length) {
why =
`this bullet names ${have.join(", ")} and no member of the scope, so this is not a copy of the ` +
`list — most likely a cross-reference. This test cannot tell a cross-reference from a copy, so ` +
`it permits neither: name every member or none. Reword without the path, or put the mention in ` +
`the Documentation section. ` +
home;
} else {
why =
`this bullet names ${have.join(", ")} and not ${missing.join(", ")}, which is a partial copy of ` +
`the scope — the shape that goes stale when the set widens. The fix is to delete the list, NOT ` +
`to backtick it` +
(extra.length > 0 ? `, and ${extra.join(", ")} is not in the scope either` : ``) +
`. ` +
// Only when `docs` is the member reported missing, because that is the one member this comparison
// can miss — and it can miss it two ways: `PATH_RE` does not match bare `docs`, on purpose, and it
// does not match `docs/` when a filename follows, because the lookahead requires the slash to end
// the token. `docs/ci.md` matches as a FILE, so it lands in `extra` while `docs` reads as missing.
(missing.includes("docs")
? `Count what the bullet names first: \`docs\` may be there spelled without its slash, or ` +
`\`docs/\` may be there only as the start of a longer path like \`docs/ci.md\` — PATH_RE sees ` +
`neither. `
: ``) +
home;
}
assert.fail(`${why}\n${item}`);
}
// AFTER the comparison above, deliberately, and the ordering is the whole point rather than a detail.
// Both fire on a partial list written as prose, and their remedies are not equal: deleting the copy
// leaves nothing to backtick, while backticking it leaves the copy partial and spends a second red run
// reaching the message that names the defect. So the comparison speaks first, and this assertion prints
// for the shape the comparison TOLERATES — a complete copy, where the content is accepted and the
// spelling is the only complaint left.
assertNoBarePaths(
item,
"CONTRIBUTING.md's automated-review bullet",
"The comparison above reads prose, so this list is not invisible the way a bare path is in the " +
"other three regions — the complaint is only the spelling, and those three write theirs as code " +
"spans. If you did not mean to keep a copy of the set here at all, delete it and let the " +
"Documentation link carry it.",
);
// Naming no files is only safe while the bullet says where the list does live. Without this, the
// pointer could be deleted and the assertion above would still pass on a bullet that promises a
// scope check and names no scope at all.
assert.match(
item,
/\(#documentation\)/,
`and it links the Documentation section, which is where the list it does not repeat lives:\n${item}`,
);
});
test("docs/ci.md describes the same scope it documents", () => {
const step = ciDocStep();
assert.deepEqual(
codeSpans(step),
BOUND_FILES,
`docs/ci.md's step 4 names exactly BOUND_FILES. This is the copy a deployer reads, so a path ` +
`here that the prompt does not enforce is a promise nothing keeps:\n${step}`,
);
assertNoBarePaths(step, "docs/ci.md's step 4");
});
// Which bare words `PATH_RE` may match is a measurement of these four documents, not a property of the
// regex, so it is checked here rather than asserted in a comment — a comment should not be what talks
// the next reader out of changing it.
//
// Per word, and in both directions, because an `assert.match(/\b(docs|agents)\b/)` passes on either and
// so cannot report that one half has become false. `docs` is bare in 4 of 4 regions today and `agents`
// in 0 of 4, which is what licenses matching one and not the other.
//
// The two halves are pinned differently, and the asymmetry is the point rather than an oversight. One
// region is enough to keep `docs` OUT; one region is enough to keep `agents` IN only until that region
// changes. So the `docs` half is existential and the `agents` half universal, and each fails with the
// change its own quantifier licenses.
test("PATH_RE's bare-word list still matches how these four documents use those words", () => {
const regions: [string, string][] = [
["the prompt's docs-prose bullet", promptProseBullet()],
["CONTRIBUTING.md's Documentation section", contributingBinding()],
["CONTRIBUTING.md's automated-review bullet", contributingReviewBullet()],
["docs/ci.md's step 4", ciDocStep()],
];
const outsideSpans = (region: string): string => region.replace(/`[^`\n]+`/g, " ");
// Existential, deliberately. Bare `docs` has to stay out of PATH_RE while ANY region uses it as an
// ordinary word, because `assertNoBarePaths` would then report "backtick it" on that region — checked
// rather than reasoned about: adding `\b[Dd]ocs\b` to PATH_RE today fails tests 1, 3 and 5 exactly
// that way. A per-region version of this assertion would therefore print the wrong remedy the moment
// the FIRST region was reworded, telling a reader to make a change that reddens three other tests.
const withBareDocs = regions.filter(([, region]) => /\bdocs\b/i.test(outsideSpans(region)));
assert.ok(
withBareDocs.length > 0,
`at least one of these ${regions.length} regions uses "docs" as an ordinary word outside a code ` +
`span, which is why PATH_RE does not match it: the match would fire on prose that binds nothing, ` +
`and a pin that cries wolf gets deleted. None does any more, so adding \`\\b[Dd]ocs\\b\` to ` +
`PATH_RE is now safe and closes another part of the bare-word gap.`,
);
// Universal, and for the opposite reason: one bare use is enough to make this match cry wolf.
//
// The lookahead is not decoration. `\b` sits between `s` and `/`, so a plain `/\bagents\b/` matches the
// `agents` inside a prose `agents/` — a path, not the English word, and its remedy is the "backtick it"
// that `assertNoBarePaths` prints, not either of the two this message offers. Excluding a following
// slash leaves exactly the readings named below.
for (const [where, region] of regions) {
assert.doesNotMatch(
outsideSpans(region),
/\bagents\b(?!\/)/i,
`${where} still keeps "agents" inside a code span or writes the singular "agent", which is why ` +
`PATH_RE DOES match it bare. Two things reach this, and they want opposite fixes: a region ` +
`has started using "agents" as an ordinary word, in which case the match has to come out of ` +
`PATH_RE — or a scope list spells \`agents/\` without its trailing slash, in which case add ` +
`the slash, and the backticks with it since a bare member fails its region's bare-path check ` +
`too, and leave PATH_RE alone. The failing text says which:\n${outsideSpans(region)}`,
);
}
});