Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: I did not exercise the preview beyond reading the diff. #11048
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/11047-automate-a-review-finding-as-a-lint-or-t → main 2 commits fork 4 threads · 4 unresolved
3 files
changed +53
−5
CLAUDE.md +1 −1
| @@ -8,7 +8,7 @@ rulebook: the invariants you must not break, and how to work here. | ||
| 8 | 8 | |
| 9 | 9 | **Read docs/VISION.md first: it says what the forge is for and the order of work.** |
| 10 | 10 | |
| 11 | **A pull that changes `src/actions`, `src/web`, `src/http` or `src/tui` needs a commit message line `Try: <what to open or call on the preview>`, or its check is red (`src/kernel/try_it.ts`).** | |
| 11 | **A pull that changes `src/actions`, `src/web`, `src/http`, `src/tui`, `src/feed`, `src/view`, `src/cli`, `src/boards`, `src/search`, `src/display`, `src/story` or `src/devices` needs a commit message line `Try: <what to open or call on the preview>`, or its check is red (`src/kernel/try_it.ts`).** | |
| 12 | 12 | |
| 13 | 13 | **Done means reachable**, and the rest of what the forge holds to is in |
| 14 | 14 | docs/PRINCIPLES.md with what enforces each; read it. This file does not repeat it. When |
src/kernel/try_it.ts +19 −4
| @@ -10,8 +10,23 @@ | ||
| 10 | 10 | |
| 11 | 11 | import type { Change } from "./pin_edits.ts"; |
| 12 | 12 | |
| 13 | const SURFACE = /^src\/(actions|web|http|tui)\/.+\.ts$/; | |
| 14 | const TRY = /^Try:\s*\S[^\n]{9,}/im; | |
| 13 | const SURFACE = /^src\/(actions|web|http|tui|feed|view|cli|boards|search|display|story|devices)\/.+\.ts$/; | |
| 14 | const TRY = /^Try:\s*\S[^\n]{9,}$/; | |
| 15 | // A Try line that says nothing was tried, or nothing can be, is no line. A | |
| 16 | // single word is hollow only when it is the whole clause (`Try: todo list…` | |
outdated
The trailing \b makes this over-match: |
||
| 17 | // is a real line about a todo page); the two confessions are hollow at the start. | |
| 18 | const HOLLOW = | |
| 19 | /^Try:\s*(?:(?:n\/?a|none|nothing|tbd|todo|untested|not applicable|see (?:the )?(?:diff|code))(?=\s*(?:[,.;:\u2014]|$))|did not\b|didn't\b|not tried\b)/i; | |
| 20 | ||
| 21 | /** The `Try:` lines of a message, one string each: each is judged alone. */ | |
| 22 | function tryLines(message: string): string[] { | |
| 23 | return message.split("\n").map((l) => l.trim()).filter((l) => /^Try:/i.test(l)); | |
| 24 | } | |
| 25 | ||
| 26 | /** True when some line of the message is a real Try line. */ | |
| 27 | function hasRealTry(message: string): boolean { | |
outdated
HOLLOW.test(m) runs on the whole message with the m flag, so a message with a good Try line and a second hollow one fails, and the check cannot tell which line it judged. Extract the Try line first, then test that line only. |
||
| 28 | return tryLines(message).some((l) => TRY.test(l) && !HOLLOW.test(l)); | |
| 29 | } | |
| 15 | 30 | |
| 16 | 31 | /** The reachable files a pull adds or edits (a deletion has nothing to try). */ |
| 17 | 32 | export function surfaceChanged(changes: readonly Change[]): string[] { |
| @@ -22,9 +37,9 @@ export function surfaceChanged(changes: readonly Change[]): string[] { | ||
| 22 | 37 | export function tryItProblems(changes: readonly Change[], messages: readonly string[]): string[] { |
| 23 | 38 | const touched = surfaceChanged(changes); |
| 24 | 39 | if (touched.length === 0) return []; |
| 25 | if (messages.some((m) => TRY.test(m))) return []; | |
| 40 | if (messages.some(hasRealTry)) return []; | |
| 26 | 41 | const shown = touched.slice(0, 3).join(", ") + (touched.length > 3 ? ` and ${touched.length - 3} more` : ""); |
| 27 | 42 | return [ |
| 28 | `rule "say how to try it on the preview": this pull changes ${shown} but no commit message has a "Try: <what to open or call on the preview>" line (10+ characters)`, | |
| 43 | `rule "say how to try it on the preview": this pull changes ${shown} but no commit message has a "Try: <what to open or call on the preview>" line (10+ characters) that names something to open or call, not "n/a" or "not tried"`, | |
| 29 | 44 | ]; |
| 30 | 45 | } |
test/try-it.test.ts +33
| @@ -26,6 +26,39 @@ test("a Try line answers it; a thin one does not", () => { | ||
| 26 | 26 | expect(tryItProblems(changes, ["Fix b\n\nTry: it"])).toHaveLength(1); |
| 27 | 27 | }); |
| 28 | 28 | |
| 29 | test("a hollow Try line is no line, and the feed, view and cli dirs are surfaces", () => { | |
Only the rejecting side of HOLLOW is tested. Add cases that must pass: |
||
| 30 | const changes = [ch("M", "src/feed/a.ts")]; | |
| 31 | expect(tryItProblems(changes, ["Fix\n\nTry: n/a, internal change only"])).toHaveLength(1); | |
| 32 | expect(tryItProblems(changes, ["Fix\n\nTry: did not run it on the preview"])).toHaveLength(1); | |
| 33 | expect(tryItProblems(changes, ["Fix\n\nTry: open / on the preview and see the new row"])).toEqual([]); | |
| 34 | expect(surfaceChanged([ch("M", "src/view/a.ts"), ch("A", "src/cli/b.ts")])).toHaveLength(2); | |
| 35 | }); | |
| 36 | ||
| 37 | test("a legitimate Try line is never mistaken for a hollow one", () => { | |
| 38 | const changes = [ch("M", "src/feed/a.ts")]; | |
| 39 | for (const line of [ | |
| 40 | "Try: todo list: open /todos on the preview", | |
| 41 | "Try: todo list page, open /todos", | |
| 42 | "Try: nothing-found state: open /x on the preview", | |
| 43 | "Try: see diff view at /review on the preview", | |
| 44 | "Try: none-selected state: open /x and press j", | |
| 45 | ]) | |
| 46 | expect(tryItProblems(changes, [`Fix\n\n${line}`])).toEqual([]); | |
| 47 | }); | |
| 48 | ||
| 49 | test("each Try line is judged alone: a real one wins over a hollow one, and other lines are not read", () => { | |
| 50 | const changes = [ch("M", "src/feed/a.ts")]; | |
| 51 | expect(tryItProblems(changes, ["Fix\n\nTry: open /x on the preview and see the row\nTry: n/a"])).toEqual([]); | |
| 52 | expect(tryItProblems(changes, ["Fix\n\nTry: open /x on the preview and see the row\nI did nothing else here."])).toEqual([]); | |
| 53 | expect(tryItProblems(changes, ["Fix\n\nnothing to try here, honestly\nTry: n/a"])).toHaveLength(1); | |
| 54 | }); | |
| 55 | ||
| 56 | test("every surface dir is a surface", () => { | |
| 57 | for (const d of ["actions", "web", "http", "tui", "feed", "view", "cli", "boards", "search", "display", "story", "devices"]) | |
| 58 | expect(surfaceChanged([ch("M", `src/${d}/x.ts`)])).toEqual([`src/${d}/x.ts`]); | |
| 59 | expect(surfaceChanged([ch("M", "src/kernel/x.ts")])).toEqual([]); | |
| 60 | }); | |
| 61 | ||
| 29 | 62 | test("a pull that changes no surface is never asked", () => { |
| 30 | 63 | expect(tryItProblems([ch("M", "src/core/x.ts")], ["Fix x"])).toEqual([]); |
| 31 | 64 | }); |
Non-blocking: TRY has no
iflag now (the old one hadim), sotry: ...in lowercase fails. Addi.