trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns18g rEventsFlowsAPI

Automate a review finding as a lint or test: Screenshots: the pull has no before/after of the open prompt in light and dark ( #12364

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/12362-automate-a-review-finding-as-a-lint-or-t → main 2 commits fork

  • Merged. scout/12362-automate-a-review-finding-as-a-lint-or-t was merged into main as dc8790b.

Review

Sign in to review this change.
4 files changed +88 −1
AGENTS.md +1
@@ -4,6 +4,7 @@ Read `CLAUDE.md` first: it is the rulebook for agents editing this repo.
44
55 README.md is capped: a new feature's how-to goes in `docs/features/<name>.md`, not a new README section.
66 A pull that adds a page (`src/web/pages/*.ts`) also adds a terminal screen under `src/tui/` and a `docs/features/<name>.md`, or a commit line `No door: <why>`; `bun scripts/gate.ts` refuses it otherwise ("both doors", #10628).
7 A pull that changes `src/web/` (a `.ts` or `.css`) names, in a commit line `Looks: <path>`, the page of `app.looks` in forge.json whose before/after pictures show it, or says `No look: <why>`; `bun scripts/gate.ts` refuses it otherwise ("before/after looks", #12362).
78
89 When you write an issue, end it with a `Files:` line naming the files the work touches (`Files: src/a.ts, docs/b.md`). The hot-file hold reads only that line when it is there; without it, every path the issue mentions counts.
910
scripts/gate.ts +20 −1
@@ -337,6 +337,25 @@ export async function bothDoorsRefusal(cwd: string = process.cwd()): Promise<str
337337 return bothDoorsProblems(parseNameStatus(diff.out), log.out.split("\0").map((m) => m.trim()).filter(Boolean));
338338 }
339339
340 /** A pull that changes a screen says which pictured page shows it (#12362). */
341 export async function lookRefusal(cwd: string = process.cwd()): Promise<string[]> {
342 const { parseNameStatus } = await import("../src/kernel/pin_edits.ts");
343 const { lookProblems } = await import("../src/kernel/look_rule.ts");
344 const base =
345 process.env.FORGE_CHECK_BASE ||
346 git(["merge-base", "origin/main", "HEAD"], cwd).out ||
347 git(["merge-base", "main", "HEAD"], cwd).out;
348 if (!base) return [];
349 const diff = git(["diff", "--name-status", "-M", `${base}..HEAD`], cwd);
350 const log = git(["log", "--format=%B%x00", `${base}..HEAD`], cwd);
351 if (!diff.ok || !log.ok) return [];
352 let looks: string[] = [];
353 try {
354 looks = JSON.parse(readFileSync(join(cwd, "forge.json"), "utf8"))?.app?.looks ?? [];
355 } catch {}
356 return lookProblems(parseNameStatus(diff.out), log.out.split("\0").map((m) => m.trim()).filter(Boolean), looks);
357 }
358
340359 /** A pull that adds a feature without touching the pipeline says why (#11036). */
341360 export async function directionRefusal(cwd: string = process.cwd()): Promise<string[]> {
342361 const { parseNameStatus } = await import("../src/kernel/pin_edits.ts");
@@ -373,7 +392,7 @@ export async function dependencyRefusal(cwd: string = process.cwd()): Promise<st
373392 // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so
374393 // a stale exclusion fails in under a second — not after install and tsc.
375394 if (import.meta.main) {
376 const refusal = [...gateRefusal(presentTests()), ...slowRefusal(presentTests()), ...(await pinRefusal()), ...(await tryItRefusal()), ...(await bothDoorsRefusal()), ...(await directionRefusal()), ...strayTestRefusal(), ...reviewRefusal(), ...(await addedRefusal()), ...(await dependencyRefusal()), ...billingDeletesRefusal()];
395 const refusal = [...gateRefusal(presentTests()), ...slowRefusal(presentTests()), ...(await pinRefusal()), ...(await tryItRefusal()), ...(await bothDoorsRefusal()), ...(await lookRefusal()), ...(await directionRefusal()), ...strayTestRefusal(), ...reviewRefusal(), ...(await addedRefusal()), ...(await dependencyRefusal()), ...billingDeletesRefusal()];
377396 for (const line of refusal) console.error(`gate: ${line}`);
378397 process.exit(refusal.length === 0 ? 0 : 2);
379398 }
src/kernel/look_rule.ts added +38
@@ -0,0 +1,38 @@
1 /**
2 * Four reviews in a week asked for before/after pictures of a changed screen,
3 * light and dark (#12362). The forge takes those itself, from the pages
4 * `app.looks` in `forge.json` names, on every pull's preview; a pull that
5 * changes what `src/web/` renders but is pictured nowhere leaves the
6 * reviewer with nothing to compare. So such a pull says, in a commit
7 * message, `Looks: <path>` naming a page of `app.looks` that shows the
8 * change, or `No look: <why>` (10+ characters). The check cannot read the
9 * pull description, so the line lives in a commit.
10 */
11
12 import type { Change } from "./pin_edits.ts";
13
14 const RENDERS = /^src\/web\/.+\.(ts|css)$/;
15 const LOOKS = /^Looks:\s*(\/\S*)/im;
16 const WAIVER = /^No look:\s*\S[^\n]{9,}/im;
17
18 /** The files a pull adds or edits that change what a page shows. */
19 export function renderChanged(changes: readonly Change[]): string[] {
20 return changes.filter((c) => !c.status.startsWith("D") && RENDERS.test(c.path)).map((c) => c.path);
21 }
22
23 /** One line per problem: none when nothing rendered changes, or a commit names a pictured page
24 * or waives it. */
25 export function lookProblems(changes: readonly Change[], messages: readonly string[], looks: readonly string[]): string[] {
26 const touched = renderChanged(changes);
27 if (touched.length === 0) return [];
28 if (messages.some((m) => WAIVER.test(m))) return [];
29 const named = messages.flatMap((m) => [...m.matchAll(new RegExp(LOOKS.source, "gim"))].map((x) => x[1]!));
30 if (named.some((p) => looks.includes(p))) return [];
31 const shown = touched.slice(0, 3).join(", ") + (touched.length > 3 ? ` and ${touched.length - 3} more` : "");
32 const why = named.length
33 ? `"Looks: ${named[0]}" is not a page of app.looks in forge.json (add it there, or name one that is)`
34 : `no commit message has a "Looks: <path>" line naming the page of app.looks that shows it`;
35 return [
36 `rule "before/after looks": this pull changes ${shown} but ${why}; the forge takes the before/after pictures (light and dark) of that page on the preview. Or put a "No look: <why>" line (10+ characters) in a commit message`,
37 ];
38 }
test/look-rule.test.ts added +29
@@ -0,0 +1,29 @@
1 import { expect, test } from "bun:test";
2 import { lookProblems } from "../src/kernel/look_rule.ts";
3
4 const ch = (status: string, path: string) => ({ status, path });
5 const looks = ["/", "/flow", "/settings/crew?help"];
6 const edit = ch("M", "src/web/pages/talk.ts");
7
8 test("a changed screen with no Looks line is refused, naming the rule", () => {
9 const [p] = lookProblems([edit], ["Open prompt"], looks);
10 expect(p).toContain('rule "before/after looks"');
11 expect(p).toContain("src/web/pages/talk.ts");
12 expect(p).toContain('"Looks: <path>"');
13 });
14
15 test("a Looks line must name a page of app.looks", () => {
16 expect(lookProblems([edit], ["x\n\nLooks: /flow"], looks)).toEqual([]);
17 expect(lookProblems([edit], ["Looks: /settings/crew?help open prompt"], looks)).toEqual([]);
18 expect(lookProblems([edit], ["Looks: /nowhere"], looks)[0]).toContain("is not a page of app.looks");
19 });
20
21 test("No look waives with a reason; short reasons do not", () => {
22 expect(lookProblems([edit], ["No look: copy change inside a hidden admin form"], looks)).toEqual([]);
23 expect(lookProblems([edit], ["No look: no"], looks)).toHaveLength(1);
24 });
25
26 test("css counts; deletions, tests and other files are never asked", () => {
27 expect(lookProblems([ch("A", "src/web/theme.css")], [], looks)).toHaveLength(1);
28 expect(lookProblems([ch("D", "src/web/pages/a.ts"), ch("M", "src/core/x.ts"), ch("M", "test/a.test.ts")], [], looks)).toEqual([]);
29 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs