Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: I did not run the preview; this review is from the diff and `src/main.ts`. #8891
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/8889-automate-a-review-finding-as-a-lint-or-t → main 1 commit fork 1 thread · 1 unresolved
3 files
changed +123
−1
scripts/gate.ts +19 −1
| @@ -274,10 +274,28 @@ export async function whyNowRefusal(cwd: string = process.cwd()): Promise<string | ||
| 274 | 274 | return whyNowProblems(parseNameStatus(diff.out), log.out.split("\0").map((m) => m.trim()).filter(Boolean)); |
| 275 | 275 | } |
| 276 | 276 | |
| 277 | /** | |
| 278 | * A pull that changes a reachable surface must say how to try it on its | |
| 279 | * preview (#8889). Same base and same pass-when-unknown as `whyNowRefusal`. | |
| 280 | */ | |
| 281 | export async function tryItRefusal(cwd: string = process.cwd()): Promise<string[]> { | |
| 282 | const { parseNameStatus } = await import("../src/kernel/pin_edits.ts"); | |
| 283 | const { tryItProblems } = await import("../src/kernel/try_it.ts"); | |
| 284 | const base = | |
| 285 | process.env.FORGE_CHECK_BASE || | |
| 286 | git(["merge-base", "origin/main", "HEAD"], cwd).out || | |
| 287 | git(["merge-base", "main", "HEAD"], cwd).out; | |
| 288 | if (!base) return []; | |
| 289 | const diff = git(["diff", "--name-status", "-M", `${base}..HEAD`], cwd); | |
| 290 | const log = git(["log", "--format=%B%x00", `${base}..HEAD`], cwd); | |
| 291 | if (!diff.ok || !log.ok) return []; | |
| 292 | return tryItProblems(parseNameStatus(diff.out), log.out.split("\0").map((m) => m.trim()).filter(Boolean)); | |
| 293 | } | |
| 294 | ||
| 277 | 295 | // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so |
| 278 | 296 | // a stale exclusion fails in under a second — not after install and tsc. |
| 279 | 297 | if (import.meta.main) { |
| 280 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await whyNowRefusal())]; | |
| 298 | const refusal = [...gateRefusal(presentTests()), ...(await pinRefusal()), ...(await whyNowRefusal()), ...(await tryItRefusal())]; | |
| 281 | 299 | for (const line of refusal) console.error(`gate: ${line}`); |
| 282 | 300 | process.exit(refusal.length === 0 ? 0 : 2); |
| 283 | 301 | } |
src/kernel/try_it.ts added +30
| @@ -0,0 +1,30 @@ | ||
| 1 | /** | |
| 2 | * Four reviews in a week opened with "I did not run the preview" (#8889): | |
| 3 | * the reviewer could not tell what to exercise there. A pull that changes | |
| 4 | * what a person, an agent or a terminal can reach (a page, an API route, an | |
| 5 | * MCP tool, an action, a terminal screen) therefore says, in a commit | |
| 6 | * message, `Try: <what to open or call on the preview>`. The check cannot | |
| 7 | * read the pull description, so the line lives in a commit. It checks that | |
| 8 | * a way to try the change was written, not that it works. | |
| 9 | */ | |
| 10 | ||
| 11 | import type { Change } from "./pin_edits.ts"; | |
| 12 | ||
| 13 | const SURFACE = /^src\/(actions|web|http|tui)\/.+\.ts$/; | |
| 14 | const TRY = /^Try:\s*\S[^\n]{9,}/im; | |
| 15 | ||
| 16 | /** The reachable files a pull adds or edits (a deletion has nothing to try). */ | |
| 17 | export function surfaceChanged(changes: readonly Change[]): string[] { | |
| 18 | return changes.filter((c) => !c.status.startsWith("D") && SURFACE.test(c.path)).map((c) => c.path); | |
| 19 | } | |
| 20 | ||
| 21 | /** One line per problem: none when no surface changes or a commit says how to try it. */ | |
| 22 | export function tryItProblems(changes: readonly Change[], messages: readonly string[]): string[] { | |
| 23 | const touched = surfaceChanged(changes); | |
| 24 | if (touched.length === 0) return []; | |
| 25 | if (messages.some((m) => TRY.test(m))) return []; | |
| 26 | const shown = touched.slice(0, 3).join(", ") + (touched.length > 3 ? ` and ${touched.length - 3} more` : ""); | |
| 27 | 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)`, | |
| 29 | ]; | |
| 30 | } |
test/try-it.test.ts added +74
| @@ -0,0 +1,74 @@ | ||
| 1 | import { afterAll, expect, test } from "bun:test"; | |
| 2 | import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; | |
| 3 | import { join } from "node:path"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { tryItRefusal } from "../scripts/gate.ts"; | |
| 6 | import { surfaceChanged, tryItProblems } from "../src/kernel/try_it.ts"; | |
| 7 | ||
| 8 | const ch = (status: string, path: string) => ({ status, path }); | |
| 9 | ||
| 10 | test("pages, routes, tools, actions and screens are surfaces; core, tests and deletions are not", () => { | |
| 11 | expect( | |
| 12 | surfaceChanged([ch("M", "src/web/pages/a.ts"), ch("A", "src/http/api/b.ts"), ch("M", "src/actions/c.ts"), ch("M", "src/tui/d.ts")]), | |
| 13 | ).toHaveLength(4); | |
| 14 | expect(surfaceChanged([ch("M", "src/core/x.ts"), ch("A", "test/x.test.ts"), ch("D", "src/web/pages/a.ts")])).toEqual([]); | |
| 15 | }); | |
| 16 | ||
| 17 | test("an edited surface with no Try line is refused, naming the rule and the file", () => { | |
| 18 | const [p] = tryItProblems([ch("M", "src/web/pages/a.ts")], ["Fix a"]); | |
| 19 | expect(p).toContain("say how to try it on the preview"); | |
| 20 | expect(p).toContain("src/web/pages/a.ts"); | |
| 21 | }); | |
| 22 | ||
| 23 | test("a Try line answers it; a thin one does not", () => { | |
| 24 | const changes = [ch("M", "src/http/api/b.ts")]; | |
| 25 | expect(tryItProblems(changes, ["Fix b\n\nTry: GET /api/v1/b on the preview and see the count"])).toEqual([]); | |
| 26 | expect(tryItProblems(changes, ["Fix b\n\nTry: it"])).toHaveLength(1); | |
| 27 | }); | |
| 28 | ||
| 29 | test("a pull that changes no surface is never asked", () => { | |
| 30 | expect(tryItProblems([ch("M", "src/core/x.ts")], ["Fix x"])).toEqual([]); | |
| 31 | }); | |
| 32 | ||
| 33 | const root = mkdtempSync(join(tmpdir(), "try-it-")); | |
| 34 | afterAll(() => rmSync(root, { recursive: true, force: true })); | |
| 35 | ||
| 36 | const git = (cwd: string, ...args: string[]) => { | |
| 37 | const r = Bun.spawnSync(["git", "-c", "user.name=t", "-c", "user.email=t@t", ...args], { cwd }); | |
| 38 | if (r.exitCode !== 0) throw new Error(r.stderr.toString()); | |
| 39 | return r.stdout.toString().trim(); | |
| 40 | }; | |
| 41 | ||
| 42 | function pull(name: string, message: string) { | |
| 43 | const dir = join(root, name); | |
| 44 | mkdirSync(join(dir, "src/web"), { recursive: true }); | |
| 45 | git(dir, "init", "-q", "-b", "work"); | |
| 46 | writeFileSync(join(dir, "README"), "a"); | |
| 47 | git(dir, "add", "-A"); | |
| 48 | git(dir, "commit", "-qm", "base"); | |
| 49 | const base = git(dir, "rev-parse", "HEAD"); | |
| 50 | writeFileSync(join(dir, "src/web/x.ts"), "export {};\n"); | |
| 51 | git(dir, "add", "-A"); | |
| 52 | git(dir, "commit", "-qm", message); | |
| 53 | return { dir, base }; | |
| 54 | } | |
| 55 | ||
| 56 | const withBase = async (base: string, f: () => Promise<string[]>) => { | |
| 57 | const old = process.env.FORGE_CHECK_BASE; | |
| 58 | process.env.FORGE_CHECK_BASE = base; | |
| 59 | try { | |
| 60 | return await f(); | |
| 61 | } finally { | |
| 62 | if (old === undefined) delete process.env.FORGE_CHECK_BASE; | |
| 63 | else process.env.FORGE_CHECK_BASE = old; | |
| 64 | } | |
| 65 | }; | |
| 66 | ||
| 67 | test("tryItRefusal reads a real repo: refused bare, passed with a Try line", async () => { | |
| 68 | const bare = pull("bare", "Add x"); | |
| 69 | const out = await withBase(bare.base, () => tryItRefusal(bare.dir)); | |
| 70 | expect(out).toHaveLength(1); | |
| 71 | expect(out[0]).toContain("say how to try it on the preview"); | |
| 72 | const ok = pull("ok", "Add x\n\nTry: open /x on the preview and read the heading"); | |
| 73 | expect(await withBase(ok.base, () => tryItRefusal(ok.dir))).toEqual([]); | |
| 74 | }); |
new above
offline
This repeats the base, diff and log code from whyNowRefusal above. Share one helper that returns {changes, messages} so the two rules cannot drift apart.