Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: Clean up: see the line comments. #10692
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10603-automate-a-review-finding-as-a-lint-or-t → main 3 commits fork 2 threads · 2 unresolved
9 files
changed +84
−26
scripts/gate.ts +12 −5
| @@ -260,11 +260,18 @@ export function reviewRefusal(root = "."): string[] { | ||
| 260 | 260 | // Loaded by `require` so `treeGate`, which loads this file alone, never needs it. |
| 261 | 261 | const { reviewProblems } = require("../src/kernel/review_rules.ts") as typeof import("../src/kernel/review_rules.ts"); |
| 262 | 262 | const dir = join(root, "test"); |
| 263 | if (!existsSync(dir)) return []; | |
| 264 | const sources = readdirSync(dir) | |
| 265 | .filter((f) => f.endsWith(".test.ts")) | |
| 266 | .map((f) => ({ path: `test/${f}`, text: readFileSync(join(dir, f), "utf8") })); | |
| 267 | return reviewProblems(sources); | |
| 263 | const tests = existsSync(dir) | |
| 264 | ? readdirSync(dir) | |
| 265 | .filter((f) => f.endsWith(".test.ts")) | |
| 266 | .map((f) => ({ path: `test/${f}`, text: readFileSync(join(dir, f), "utf8") })) | |
| 267 | : []; | |
| 268 | const src = join(root, "src"); | |
| 269 | const code = existsSync(src) | |
| 270 | ? (readdirSync(src, { recursive: true }) as string[]) | |
| 271 | .filter((f) => f.endsWith(".ts") && !f.includes("vendor/")) | |
| 272 | .map((f) => ({ path: `src/${f}`, text: readFileSync(join(src, f), "utf8") })) | |
| 273 | : []; | |
| 274 | return reviewProblems([...tests, ...code]); | |
| 268 | 275 | } |
| 269 | 276 | |
| 270 | 277 | /** |
src/actions/proposals.ts +3 −3
| @@ -102,16 +102,16 @@ export function redirectProposal( | ||
| 102 | 102 | if (!direction) return fail("invalid", "this app has no direction issue to plan from"); |
| 103 | 103 | if (!moveProposal(app.db, proposal.id, ["pending", "later"], "redirected")) |
| 104 | 104 | return fail("conflict", "it was already decided"); |
| 105 | const said_ = addComment( | |
| 105 | const wrote = addComment( | |
| 106 | 106 | app, |
| 107 | 107 | found.value.actor, |
| 108 | 108 | repo, |
| 109 | 109 | direction, |
| 110 | 110 | `Redirect: not the wave "${proposal.title}" as proposed.\n\n${said}`, |
| 111 | 111 | ); |
| 112 | if (!said_.ok) { | |
| 112 | if (!wrote.ok) { | |
| 113 | 113 | moveProposal(app.db, proposal.id, ["redirected"], proposal.state); |
| 114 | return fail("invalid", `could not write to the direction issue: ${errorWords(said_.error)}`); | |
| 114 | return fail("invalid", `could not write to the direction issue: ${errorWords(wrote.error)}`); | |
| 115 | 115 | } |
| 116 | 116 | if (direction.assigneeId === planner.user.id) { |
| 117 | 117 | queueAgentRun(app, repo, direction, planner, "feedback", found.value.actor.id); |
src/kernel/review_rules.ts +17
| @@ -7,6 +7,12 @@ | ||
| 7 | 7 | * - "a test file does not import another test file" (#7717): importing |
| 8 | 8 | * `./x.test.ts` registers and re-runs every test in it. Share a helper |
| 9 | 9 | * module under `test/helpers/` instead. |
| 10 | * - "a name does not end in an underscore" (#9183, `split_`): a trailing | |
| 11 | * underscore dodges a clash instead of saying what the value holds. Name it | |
| 12 | * for what it is. Applies to `src/` (vendored code is not ours to rename). | |
| 13 | * Scope: names declared with const, let, var or function. Parameters, | |
| 14 | * destructured names and class members are not scanned (a regex cannot | |
| 15 | * tell them from a type or a call without parsing). | |
| 10 | 16 | */ |
| 11 | 17 | |
| 12 | 18 | export type Source = { path: string; text: string }; |
| @@ -15,10 +21,21 @@ const TEST_FILE = /^test\/.+\.test\.ts$/; | ||
| 15 | 21 | // `from "./x.test.ts"`, `import("../x.test")`, `import "./x.test.ts"`. |
| 16 | 22 | const TEST_IMPORT = /(?:\bfrom\s*|\bimport\s*\(?\s*)["'](\.[^"']*\.test(?:\.ts)?)["']/g; |
| 17 | 23 | |
| 24 | const SRC_FILE = /^src\/(?!.*\/vendor\/).+\.ts$/; | |
| 25 | // A const, let, var or function whose name ends in one underscore. | |
| 26 | const UNDERSCORED = /\b(?:const|let|var|function)\s+([A-Za-z][A-Za-z0-9]*_)(?![A-Za-z0-9_])/g; | |
| 27 | ||
| 18 | 28 | /** One line per problem, each naming its rule, file and line. */ |
| 19 | 29 | export function reviewProblems(sources: readonly Source[]): string[] { |
| 20 | 30 | const out: string[] = []; |
| 21 | 31 | for (const { path, text } of sources) { |
| 32 | if (SRC_FILE.test(path)) | |
| 33 | text.split("\n").forEach((line, i) => { | |
| 34 | for (const m of line.matchAll(UNDERSCORED)) | |
| 35 | out.push( | |
| 36 | `rule "a name does not end in an underscore": ${path}:${i + 1} declares ${m[1]}; name it for what it holds`, | |
| 37 | ); | |
| 38 | }); | |
| 22 | 39 | if (!TEST_FILE.test(path)) continue; |
| 23 | 40 | text.split("\n").forEach((line, i) => { |
| 24 | 41 | for (const m of line.matchAll(TEST_IMPORT)) |
src/system/live.ts +3 −3
| @@ -48,11 +48,11 @@ export function createLive(options: { paths?: Paths; roots?: () => Roots; self?: | ||
| 48 | 48 | const listeners = new Set<(view: SystemView) => void>(); |
| 49 | 49 | const schedule = options.schedule ?? realSchedule; |
| 50 | 50 | let timer: (() => void) | undefined; |
| 51 | const self_: Live = { | |
| 51 | const handle: Live = { | |
| 52 | 52 | subscribe(listener) { |
| 53 | 53 | listeners.add(listener); |
| 54 | 54 | if (timer === undefined) { |
| 55 | timer = schedule(() => self_.publish(self_.read()), options.pushMs ?? PUSH_MS); | |
| 55 | timer = schedule(() => handle.publish(handle.read()), options.pushMs ?? PUSH_MS); | |
| 56 | 56 | } |
| 57 | 57 | return () => { |
| 58 | 58 | listeners.delete(listener); |
| @@ -84,7 +84,7 @@ export function createLive(options: { paths?: Paths; roots?: () => Roots; self?: | ||
| 84 | 84 | return view; |
| 85 | 85 | }, |
| 86 | 86 | }; |
| 87 | return self_; | |
| 87 | return handle; | |
| 88 | 88 | } |
| 89 | 89 | |
| 90 | 90 | let shared = createLive(); |
src/tui/screens/devices.ts +23 −7
| @@ -19,7 +19,7 @@ import { style } from "../term/style.ts"; | ||
| 19 | 19 | import { wrap } from "../term/width.ts"; |
| 20 | 20 | import { fill, len, split } from "../ui/layout.ts"; |
| 21 | 21 | import type { Session } from "../session.ts"; |
| 22 | import { ListState, confirmScreen, drawList, promptScreen, type ListRow } from "./common.ts"; | |
| 22 | import { ListState, confirmScreen, drawList, later, promptScreen, type ListRow } from "./common.ts"; | |
| 23 | 23 | |
| 24 | 24 | const mayUse = (f: Session): boolean => mayManageDevices(f.user); |
| 25 | 25 | |
| @@ -57,19 +57,35 @@ export function devicesScreen(f: Session, ctx: Ctx): Screen { | ||
| 57 | 57 | placeholder: "Hall printer ipp://192.168.1.161:631/ipp/print", |
| 58 | 58 | onSubmit: declareOne, |
| 59 | 59 | }); |
| 60 | const change = (d: Device, act: typeof grant, done: string) => (line: string): string | undefined => { | |
| 61 | const slug = line.trim(); | |
| 62 | const made = act(f.app, f.user, d.id, slug); | |
| 60 | const said = (made: { ok: true; value: string } | { ok: false; error: keyof typeof DEVICE_ERRORS }, d: Device, done: string): string | undefined => { | |
| 63 | 61 | note = made.ok ? `${made.value} ${done} ${d.name}.` : DEVICE_ERRORS[made.error]; |
| 64 | 62 | if (made.ok) reload(); |
| 65 | 63 | return made.ok ? undefined : note; |
| 66 | 64 | }; |
| 67 | const askApp = (d: Device, act: typeof grant, title: string, done: string): Screen => | |
| 65 | const allow = (d: Device) => (line: string): string | undefined => | |
| 66 | said(grant(f.app, f.user, d.id, line.trim()), d, "may print on"); | |
| 67 | // Revoking asks first: the slug is typed, then a yes/no names the app and the printer. | |
| 68 | const stopOne = (d: Device) => (line: string): Promise<void> => { | |
| 69 | const slug = line.trim(); | |
| 70 | return later(ctx, () => | |
| 71 | confirmScreen(ctx, screen, { | |
| 72 | title: "Stop an app printing here", | |
| 73 | body: [`${slug} on ${d.name}`, "The app can no longer print there."], | |
| 74 | actions: [{ label: "Revoke", variant: "danger" }, { label: "Keep" }], | |
| 75 | yesNo: true, | |
| 76 | onChoose: (i) => { | |
| 77 | if (i !== 0) return; | |
| 78 | said(revoke(f.app, f.user, d.id, slug), d, "may no longer print on"); | |
| 79 | }, | |
| 80 | }), | |
| 81 | ); | |
| 82 | }; | |
| 83 | const askApp = (d: Device, title: string, onSubmit: (line: string) => string | undefined | Promise<void>): Screen => | |
| 68 | 84 | promptScreen(ctx, screen, { |
| 69 | 85 | title, |
| 70 | 86 | label: `App slug for ${d.name}`, |
| 71 | 87 | placeholder: allAppSlugs(f.db).join(" ") || "no apps yet", |
| 72 | onSubmit: change(d, act, done), | |
| 88 | onSubmit, | |
| 73 | 89 | }); |
| 74 | 90 | const confirmRemove = (d: Device): Screen => |
| 75 | 91 | confirmScreen(ctx, screen, { |
| @@ -119,7 +135,7 @@ export function devicesScreen(f: Session, ctx: Ctx): Screen { | ||
| 119 | 135 | const d = rows[state.index]; |
| 120 | 136 | if ((e.key === "a" || e.key === "r") && d !== undefined) { |
| 121 | 137 | note = ""; |
| 122 | ctx.push(e.key === "a" ? askApp(d, grant, "Let an app print here", "may print on") : askApp(d, revoke, "Stop an app printing here", "may no longer print on")); | |
| 138 | ctx.push(e.key === "a" ? askApp(d, "Let an app print here", allow(d)) : askApp(d, "Stop an app printing here", stopOne(d))); | |
| 123 | 139 | return true; |
| 124 | 140 | } |
| 125 | 141 | if (e.key === "x" && d !== undefined) { |
src/tui/screens/workspace.ts +2 −2
| @@ -752,7 +752,7 @@ export function workspaceScreen( | ||
| 752 | 752 | * The arrays are the live ones, so this is a pointer plus a clock rather |
| 753 | 753 | * than a copy — the cost of remembering is one `Map.set` a paint. |
| 754 | 754 | */ |
| 755 | const remember_ = (): void => { | |
| 755 | const rememberView = (): void => { | |
| 756 | 756 | f.scratch.set(key, { |
| 757 | 757 | tabs, |
| 758 | 758 | roots, |
| @@ -1841,7 +1841,7 @@ export function workspaceScreen( | ||
| 1841 | 1841 | }); |
| 1842 | 1842 | } |
| 1843 | 1843 | drawStatus(s, status!, c); |
| 1844 | remember_(); | |
| 1844 | rememberView(); | |
| 1845 | 1845 | }, |
| 1846 | 1846 | paste(text) { |
| 1847 | 1847 | const t = tab(); |
src/web/pages/backups.ts +6 −6
| @@ -490,7 +490,7 @@ function mirrorCard(ctx: Ctx): Html { | ||
| 490 | 490 | const configured = svc?.mirror !== undefined; |
| 491 | 491 | const need = mirrorNeed(state, Date.now()); |
| 492 | 492 | const set = mirrorSealedSet(ctx.app.db); |
| 493 | const field_ = (name: string, label: string, sealed: string, type = "text"): Html => | |
| 493 | const sealedField = (name: string, label: string, sealed: string, type = "text"): Html => | |
| 494 | 494 | html`<label class="field" |
| 495 | 495 | ><span>${label}</span |
| 496 | 496 | ><input |
| @@ -520,11 +520,11 @@ function mirrorCard(ctx: Ctx): Html { | ||
| 520 | 520 | <h2>Off-box mirror</h2> |
| 521 | 521 | ${status} |
| 522 | 522 | <form method="post" action="/settings/backups/mirror" class="stack"> |
| 523 | ${field_("endpoint", "Endpoint", MIRROR_SEALED.endpoint)} | |
| 524 | ${field_("bucket", "Bucket", MIRROR_SEALED.bucket)} | |
| 525 | ${field_("region", "Region (optional)", MIRROR_SEALED.region)} | |
| 526 | ${field_("access_key", "Access key", MIRROR_SEALED.accessKey)} | |
| 527 | ${field_("secret_key", "Secret key", MIRROR_SEALED.secretKey, "password")} | |
| 523 | ${sealedField("endpoint", "Endpoint", MIRROR_SEALED.endpoint)} | |
| 524 | ${sealedField("bucket", "Bucket", MIRROR_SEALED.bucket)} | |
| 525 | ${sealedField("region", "Region (optional)", MIRROR_SEALED.region)} | |
| 526 | ${sealedField("access_key", "Access key", MIRROR_SEALED.accessKey)} | |
| 527 | ${sealedField("secret_key", "Secret key", MIRROR_SEALED.secretKey, "password")} | |
| 528 | 528 | <div class="row-actions"> |
| 529 | 529 | ${btn("Seal mirror", { type: "submit", variant: "primary", icon: "lock" })} |
| 530 | 530 | <button class="btn" type="submit" formaction="/settings/backups/mirror/now" formnovalidate> |
test/review-rules.test.ts +10
| @@ -29,6 +29,16 @@ test("a helper import, a source import and a non-test file pass", () => { | ||
| 29 | 29 | ).toEqual([]); |
| 30 | 30 | }); |
| 31 | 31 | |
| 32 | test("a declared name ending in an underscore is refused in src, naming the rule", () => { | |
| 33 | const [p, ...rest] = reviewProblems([ | |
| 34 | { path: "src/a.ts", text: "const ok = 1;\nconst split_ = 2;\nlet _x = 3;\nfunction f_() {}" }, | |
| 35 | ]); | |
| 36 | expect(p).toContain('rule "a name does not end in an underscore"'); | |
| 37 | expect(p).toContain("src/a.ts:2"); | |
| 38 | expect(rest).toHaveLength(1); | |
| 39 | expect(reviewProblems([{ path: "src/web/render/vendor/y.ts", text: "const v_ = 1;" }])).toEqual([]); | |
| 40 | }); | |
| 41 | ||
| 32 | 42 | test("the repository's own tests pass the rules", () => { |
| 33 | 43 | expect(reviewRefusal()).toEqual([]); |
| 34 | 44 | expect(readdirSync("test").length).toBeGreaterThan(0); |
test/tui-devices.test.ts +8
| @@ -119,6 +119,14 @@ describe("the devices screen", () => { | ||
| 119 | 119 | expect(text()).toContain("apps: labels"); |
| 120 | 120 | await feed("r"); |
| 121 | 121 | await feed("labels\r"); |
| 122 | expect(text()).toContain("The app can no longer print there."); | |
| 123 | expect(mine()).toEqual(["labels"]); | |
| 124 | await feed("n"); | |
| 125 | expect(mine()).toEqual(["labels"]); | |
| 126 | expect(kinds.includes("device.revoked")).toBe(false); | |
| 127 | await feed("r"); | |
| 128 | await feed("labels\r"); | |
| 129 | await feed("y"); | |
| 122 | 130 | expect(mine()).toEqual([]); |
| 123 | 131 | expect(kinds.includes("device.revoked")).toBe(true); |
| 124 | 132 | }); |
Only declarations via const/let/var/function are caught; destructured names, parameters and class members ending in
_slip through. Either widen the pattern or state in the rule comment that this is the scope.