Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: The line and the tints work and the tests are real, but three things need fixing #11894
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/11884-automate-a-review-finding-as-a-lint-or-t → main 5 commits fork 3 threads · 3 unresolved
4 files
changed +192
−2
scripts/gate.ts +13 −1
| @@ -244,6 +244,18 @@ export function reviewRefusal(root = "."): string[] { | ||
| 244 | 244 | return reviewProblems([...tests, ...code]); |
| 245 | 245 | } |
| 246 | 246 | |
| 247 | /** What the pull adds, judged by `addedProblems` (#11884). Passes when no base resolves, like the rest. */ | |
| 248 | export async function addedRefusal(cwd: string = process.cwd()): Promise<string[]> { | |
| 249 | const { addedProblems, parseAdded } = await import("../src/kernel/review_rules.ts"); | |
| 250 | const base = | |
| 251 | process.env.FORGE_CHECK_BASE || | |
| 252 | git(["merge-base", "origin/main", "HEAD"], cwd).out || | |
| 253 | git(["merge-base", "main", "HEAD"], cwd).out; | |
| 254 | if (!base) return []; | |
| 255 | const diff = git(["diff", "-U0", "--no-color", "--no-ext-diff", `${base}..HEAD`, "--", "src", "test"], cwd); | |
| 256 | return diff.ok ? addedProblems(parseAdded(diff.out)) : []; | |
| 257 | } | |
| 258 | ||
| 247 | 259 | /** |
| 248 | 260 | * A pull that changes a reachable surface must say how to try it on its |
| 249 | 261 | * preview (#8889). Same base and same pass-when-unknown as `pinRefusal`. |
| @@ -295,7 +307,7 @@ export async function directionRefusal(cwd: string = process.cwd()): Promise<str | ||
| 295 | 307 | // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so |
| 296 | 308 | // a stale exclusion fails in under a second — not after install and tsc. |
| 297 | 309 | if (import.meta.main) { |
| 298 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await tryItRefusal()), ...(await bothDoorsRefusal()), ...(await directionRefusal()), ...strayTestRefusal(), ...reviewRefusal(), ...billingDeletesRefusal()]; | |
| 310 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await tryItRefusal()), ...(await bothDoorsRefusal()), ...(await directionRefusal()), ...strayTestRefusal(), ...reviewRefusal(), ...(await addedRefusal()), ...billingDeletesRefusal()]; | |
| 299 | 311 | for (const line of refusal) console.error(`gate: ${line}`); |
| 300 | 312 | process.exit(refusal.length === 0 ? 0 : 2); |
| 301 | 313 | } |
src/kernel/review_rules.ts +76
| @@ -13,6 +13,20 @@ | ||
| 13 | 13 | * Scope: names declared with const, let, var or function. Parameters, |
| 14 | 14 | * destructured names and class members are not scanned (a regex cannot |
| 15 | 15 | * tell them from a type or a call without parsing). |
| 16 | * | |
| 17 | * Rules on what a pull ADDS (#11884, after #11074, #11532, #11723, #11761: | |
| 18 | * reviewers asked again and again for these). The tree already holds old | |
| 19 | * uses, so they are judged on added lines only (`addedProblems`, fed by | |
| 20 | * `git diff -U0` through `parseAdded`); main's own lines are never refused. | |
| 21 | * | |
| 22 | * - "a test builds the real input, not `as never`" (#11074): the cast hides | |
| 23 | * the real type, and a made-up record broke a foreign key the cast hid. | |
| 24 | * - "a test does not wait on a real timer" (#11723): `Bun.sleep` or a | |
| 25 | * `setTimeout` promise makes the test slow and time-dependent. Inject the | |
| 26 | * clock and step it; where a real wait is the point, end the line with | |
| 27 | * `// real-timer: <why>`. | |
| 28 | * - "a comment line is wrapped at 100 columns" (#11074): in `src/` and | |
| 29 | * `test/`, a longer comment line is rewrapped. | |
| 16 | 30 | */ |
| 17 | 31 | |
| 18 | 32 | export type Source = { path: string; text: string }; |
| @@ -46,3 +60,65 @@ export function reviewProblems(sources: readonly Source[]): string[] { | ||
| 46 | 60 | } |
| 47 | 61 | return out; |
| 48 | 62 | } |
| 63 | ||
| 64 | export type Added = { path: string; line: number; text: string }; | |
| 65 | ||
| 66 | /** The lines a `git diff -U0` adds, with their new-file line numbers. */ | |
| 67 | export function parseAdded(diff: string): Added[] { | |
| 68 | const out: Added[] = []; | |
| 69 | let path = ""; | |
| 70 | let line = 0; | |
| 71 | let inHunk = false; | |
| 72 | for (const row of diff.split("\n")) { | |
outdated
An added line whose text is |
||
| 73 | if (row.startsWith("diff --git ")) { | |
| 74 | inHunk = false; | |
| 75 | path = ""; | |
| 76 | } else if (!inHunk && row.startsWith("+++ ")) { | |
| 77 | path = row.startsWith("+++ b/") ? row.slice(6) : ""; | |
| 78 | } else if (row.startsWith("@@")) { | |
| 79 | inHunk = true; | |
| 80 | const m = row.match(/\+(\d+)/); | |
| 81 | line = m ? Number(m[1]) : 0; | |
| 82 | } else if (inHunk && row.startsWith("+") && path) { | |
| 83 | out.push({ path, line, text: row.slice(1) }); | |
| 84 | line++; | |
| 85 | } | |
| 86 | } | |
| 87 | return out; | |
outdated
The bare |
||
| 88 | } | |
| 89 | ||
| 90 | const AS_NEVER = /\bas never\b/; | |
| 91 | const REAL_TIMER = /\bBun\.sleep(?:Sync)?\s*\(|\bsetTimeout\s*\(\s*(?:r|res|resolve)\b/; | |
| 92 | const COMMENT_LINE = /^\s*(?:\/\/|\/\*)/; | |
| 93 | // A `*` line is a comment only inside a block comment opened on an added line just before it; | |
| 94 | // otherwise ` * 1000 * rate` is a multiplication continuation. | |
| 95 | const BLOCK_STAR = /^\s*\*(?:\s|\/|$)/; | |
| 96 | const WRAP = 100; | |
| 97 | const CODE_FILE = /^(?:src|test)\/(?!.*\/vendor\/).+\.ts$/; | |
| 98 | ||
| 99 | /** One line per problem among the lines a pull adds, each naming its rule. */ | |
| 100 | export function addedProblems(added: readonly Added[]): string[] { | |
| 101 | const out: string[] = []; | |
| 102 | let block = false; | |
| 103 | let last = ""; | |
| 104 | let lastLine = 0; | |
| 105 | for (const { path, line, text } of added) { | |
| 106 | if (path !== last || line !== lastLine + 1) block = false; | |
| 107 | last = path; | |
| 108 | lastLine = line; | |
| 109 | const comment = COMMENT_LINE.test(text) || (block && BLOCK_STAR.test(text)); | |
| 110 | if (text.includes("/*") && !text.includes("*/")) block = true; | |
| 111 | else if (text.includes("*/")) block = false; | |
| 112 | if (!CODE_FILE.test(path)) continue; | |
| 113 | const at = `${path}:${line}`; | |
| 114 | if (TEST_FILE.test(path)) { | |
| 115 | if (AS_NEVER.test(text)) | |
| 116 | out.push(`rule "a test builds the real input, not \`as never\`": ${at} casts; build the proper record so the type checks`); | |
| 117 | if (REAL_TIMER.test(text) && !text.includes("// real-timer:")) | |
| 118 | out.push(`rule "a test does not wait on a real timer": ${at} sleeps; inject a clock and step it, or end the line with \`// real-timer: <why>\``); | |
| 119 | } | |
| 120 | if (comment && text.length > WRAP) | |
| 121 | out.push(`rule "a comment line is wrapped at ${WRAP} columns": ${at} is ${text.length} long; rewrap it`); | |
| 122 | } | |
| 123 | return out; | |
| 124 | } |
test/gate-added-refusal.test.ts added +50
| @@ -0,0 +1,50 @@ | ||
| 1 | import { afterEach, expect, test } from "bun:test"; | |
| 2 | import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; | |
| 3 | import { tmpdir } from "node:os"; | |
| 4 | import { join } from "node:path"; | |
| 5 | import { addedRefusal } from "../scripts/gate.ts"; | |
| 6 | ||
| 7 | const CAST = "f(row as" + " never);"; | |
| 8 | const dirs: string[] = []; | |
| 9 | const saved = process.env.FORGE_CHECK_BASE; | |
| 10 | afterEach(() => { | |
| 11 | for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); | |
| 12 | if (saved === undefined) delete process.env.FORGE_CHECK_BASE; | |
| 13 | else process.env.FORGE_CHECK_BASE = saved; | |
| 14 | }); | |
| 15 | ||
| 16 | function sh(cwd: string, ...args: string[]): string { | |
| 17 | const r = Bun.spawnSync(["git", "-c", "user.email=t@t", "-c", "user.name=t", "-c", "commit.gpgsign=false", ...args], { cwd }); | |
| 18 | if (r.exitCode !== 0) throw new Error(r.stderr.toString()); | |
| 19 | return r.stdout.toString().trim(); | |
| 20 | } | |
| 21 | ||
| 22 | /** A repository with a base commit and a head commit that adds a cast in a test file. */ | |
| 23 | function repo(): { dir: string; base: string } { | |
| 24 | const dir = mkdtempSync(join(tmpdir(), "forge-added-")); | |
| 25 | dirs.push(dir); | |
| 26 | sh(dir, "init", "-q", "-b", "trunk"); | |
| 27 | mkdirSync(join(dir, "test")); | |
| 28 | writeFileSync(join(dir, "test/a.test.ts"), "const a = 1;\n"); | |
| 29 | sh(dir, "add", "-A"); | |
| 30 | sh(dir, "commit", "-qm", "base"); | |
| 31 | const base = sh(dir, "rev-parse", "HEAD"); | |
| 32 | writeFileSync(join(dir, "test/a.test.ts"), `const a = 1;\n${CAST}\n`); | |
| 33 | sh(dir, "commit", "-qam", "head"); | |
| 34 | return { dir, base }; | |
| 35 | } | |
| 36 | ||
| 37 | test("the gate names the rule for a cast the head commit adds, judged from FORGE_CHECK_BASE", async () => { | |
| 38 | const { dir, base } = repo(); | |
| 39 | process.env.FORGE_CHECK_BASE = base; | |
| 40 | const out = await addedRefusal(dir); | |
| 41 | expect(out).toHaveLength(1); | |
| 42 | expect(out[0]).toContain("rule \"a test builds the real input"); | |
| 43 | expect(out[0]).toContain("test/a.test.ts:2"); | |
| 44 | }); | |
| 45 | ||
| 46 | test("the gate passes when no base resolves", async () => { | |
| 47 | const { dir } = repo(); | |
| 48 | delete process.env.FORGE_CHECK_BASE; | |
| 49 | expect(await addedRefusal(dir)).toEqual([]); | |
| 50 | }); |
test/review-rules.test.ts +53 −1
| @@ -1,6 +1,6 @@ | ||
| 1 | 1 | import { expect, test } from "bun:test"; |
| 2 | 2 | import { readdirSync, readFileSync } from "node:fs"; |
| 3 | import { reviewProblems } from "../src/kernel/review_rules.ts"; | |
| 3 | import { addedProblems, parseAdded, reviewProblems } from "../src/kernel/review_rules.ts"; | |
| 4 | 4 | import { reviewRefusal } from "../scripts/gate.ts"; |
| 5 | 5 | |
| 6 | 6 | // Built apart so this file does not itself import a test file. |
| @@ -44,3 +44,55 @@ test("the repository's own tests pass the rules", () => { | ||
| 44 | 44 | expect(readdirSync("test").length).toBeGreaterThan(0); |
| 45 | 45 | expect(readFileSync("test/toolbar-camera.test.ts", "utf8")).toContain("helpers/toolbar-boot.ts"); |
| 46 | 46 | }); |
| 47 | ||
| 48 | // Built apart so this file does not itself trip the rules it tests. | |
| 49 | const CAST = "f(row as" + " never);"; | |
| 50 | const SLEEP = "await Bun." + "sleep(20);"; | |
| 51 | const TIMEOUT = "await new Promise((r) => set" + "Timeout(r, 5));"; | |
| 52 | const diffOf = (path: string, ...lines: string[]) => | |
| 53 | `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -3,0 +4,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}\n`; | |
| 54 | ||
| 55 | test("parseAdded gives the added lines with their new line numbers", () => { | |
| 56 | expect(parseAdded(diffOf("test/a.test.ts", "x", "y"))).toEqual([ | |
| 57 | { path: "test/a.test.ts", line: 4, text: "x" }, | |
| 58 | { path: "test/a.test.ts", line: 5, text: "y" }, | |
| 59 | ]); | |
| 60 | }); | |
| 61 | ||
| 62 | test("an added cast to never in a test is refused, naming the rule and line; in src it is not", () => { | |
| 63 | const [p, ...rest] = addedProblems(parseAdded(diffOf("test/a.test.ts", "ok();", CAST))); | |
| 64 | expect(rest).toEqual([]); | |
| 65 | expect(p).toContain("a test builds the real input"); | |
| 66 | expect(p).toContain("test/a.test.ts:5"); | |
| 67 | expect(addedProblems(parseAdded(diffOf("src/a.ts", CAST)))).toEqual([]); | |
| 68 | }); | |
| 69 | ||
| 70 | test("an added real-timer wait in a test is refused unless it says why", () => { | |
| 71 | const bad = addedProblems(parseAdded(diffOf("test/a.test.ts", SLEEP, TIMEOUT))); | |
| 72 | expect(bad).toHaveLength(2); | |
| 73 | expect(bad[0]).toContain('rule "a test does not wait on a real timer"'); | |
| 74 | expect(addedProblems(parseAdded(diffOf("test/a.test.ts", `${SLEEP} // real-timer: the socket needs it`)))).toEqual([]); | |
| 75 | }); | |
| 76 | ||
| 77 | test("an added comment line over 100 columns is refused", () => { | |
| 78 | const long = `// ${"word ".repeat(21)}`; | |
| 79 | expect(long.length).toBeGreaterThan(100); | |
| 80 | const [p] = addedProblems(parseAdded(diffOf("src/a.ts", long))); | |
| 81 | expect(p).toContain('rule "a comment line is wrapped at 100 columns"'); | |
| 82 | expect(addedProblems(parseAdded(diffOf("src/a.ts", "// short")))).toEqual([]); | |
| 83 | expect(addedProblems(parseAdded(diffOf("docs/a.md", long)))).toEqual([]); | |
| 84 | }); | |
| 85 | ||
| 86 | test("a `++ x` line added is read as a line, not as a file header", () => { | |
| 87 | const diff = diffOf("test/a.test.ts", "++ " + CAST); | |
| 88 | expect(parseAdded(diff)).toEqual([{ path: "test/a.test.ts", line: 4, text: "++ " + CAST }]); | |
| 89 | }); | |
| 90 | ||
| 91 | test("a long multiplication continuation is code, a long block-comment line is a comment", () => { | |
| 92 | const long = " * 1000 * rate " + "+ x".repeat(40); | |
| 93 | expect(addedProblems(parseAdded(diffOf("src/a.ts", "const t = a", long)))).toEqual([]); | |
| 94 | const wrapped = ["/**", " * " + "word ".repeat(25), " */"]; | |
| 95 | const out = addedProblems(parseAdded(diffOf("src/a.ts", ...wrapped))); | |
| 96 | expect(out).toHaveLength(1); | |
| 97 | expect(out[0]).toContain("src/a.ts:5"); | |
| 98 | }); |
new above
offline
No test calls addedRefusal; the gate wiring can break silently. Add a temp-repo test (see body).