Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: Merge the second import from perf/live.ts into the existing one. #11594
scout/forge:scout/11517-automate-a-review-finding-as-a-lint-or-t → main 1 commit fork
4 files
changed +264
−3
scripts/duplicate-imports-allow.json added +189
| @@ -0,0 +1,189 @@ | ||
| 1 | [ | |
| 2 | "src/actions/apps.ts|../core/pulls.ts", | |
| 3 | "src/actions/asks_decide.ts|./asks_words.ts", | |
| 4 | "src/actions/fleet.ts|../core/runs.ts", | |
| 5 | "src/actions/holds.ts|../core/repos.ts", | |
| 6 | "src/actions/pulls.ts|../core/issues.ts", | |
| 7 | "src/actions/pulls.ts|../core/repos.ts", | |
| 8 | "src/actions/repos.ts|../core/storage.ts", | |
| 9 | "src/app.ts|./core/schema.ts", | |
| 10 | "src/apps/runtime.ts|../core/pulls.ts", | |
| 11 | "src/apps/scaffold.ts|./actions.ts", | |
| 12 | "src/apps/scaffold.ts|./audit.ts", | |
| 13 | "src/apps/scaffold.ts|./can.ts", | |
| 14 | "src/apps/scaffold.ts|./compress.ts", | |
| 15 | "src/apps/scaffold.ts|./contract.ts", | |
| 16 | "src/apps/scaffold.ts|./db.ts", | |
| 17 | "src/apps/scaffold.ts|./env.ts", | |
| 18 | "src/apps/scaffold.ts|./http.ts", | |
| 19 | "src/apps/scaffold.ts|./live.ts", | |
| 20 | "src/apps/scaffold.ts|./page.ts", | |
| 21 | "src/apps/scaffold.ts|./routes.ts", | |
| 22 | "src/apps/scaffold.ts|./security.ts", | |
| 23 | "src/apps/scaffold.ts|./server.ts", | |
| 24 | "src/apps/scaffold.ts|./signin.ts", | |
| 25 | "src/apps/scaffold.ts|./site.ts", | |
| 26 | "src/apps/scaffold.ts|./tokens.ts", | |
| 27 | "src/apps/scaffold.ts|./who.ts", | |
| 28 | "src/apps/scaffold.ts|bun:test", | |
| 29 | "src/apps/scaffold.ts|node:crypto", | |
| 30 | "src/apps/scaffold.ts|node:fs", | |
| 31 | "src/apps/scaffold.ts|node:os", | |
| 32 | "src/apps/scaffold.ts|node:path", | |
| 33 | "src/apps/scaffold.ts|type ./who.ts", | |
| 34 | "src/apps/scaffold.ts|type bun:sqlite", | |
| 35 | "src/checks/service.ts|../core/capacity.ts", | |
| 36 | "src/checks/service.ts|../core/pulls.ts", | |
| 37 | "src/core/reconcile.ts|../git/repos.ts", | |
| 38 | "src/crew/autonomy.ts|../core/done_on_main.ts", | |
| 39 | "src/crew/dispatch.ts|../core/issues.ts", | |
| 40 | "src/crew/dispatch.ts|../runs/log.ts", | |
| 41 | "src/crew/runner.ts|../git/env.ts", | |
| 42 | "src/crew/vision.ts|../core/pulls.ts", | |
| 43 | "src/http/api.ts|../actions/issues.ts", | |
| 44 | "src/http/api.ts|../actions/runs.ts", | |
| 45 | "src/http/api.ts|../core/apps.ts", | |
| 46 | "src/http/api.ts|../core/repos.ts", | |
| 47 | "src/http/api.ts|./limits.ts", | |
| 48 | "src/http/api/admin.ts|../respond.ts", | |
| 49 | "src/http/api/app.ts|../../actions/build_preview.ts", | |
| 50 | "src/http/api/upstream.ts|../../core/repos.ts", | |
| 51 | "src/http/rp.ts|../web/pages/site.ts", | |
| 52 | "src/http/server.ts|../core/users.ts", | |
| 53 | "src/http/server.ts|../web/pages/thread.ts", | |
| 54 | "src/tui/forge.ts|./screens/repo.ts", | |
| 55 | "src/tui/screens/dashboard.ts|../../flow/kpi_words.ts", | |
| 56 | "src/tui/screens/dashboard.ts|../../web/studio/asks.ts", | |
| 57 | "src/tui/screens/display.ts|../../display/items.ts", | |
| 58 | "src/tui/screens/external.ts|type ../app/app.ts", | |
| 59 | "src/tui/screens/forges.ts|../../spawn/clients.ts", | |
| 60 | "src/tui/screens/forges.ts|./common.ts", | |
| 61 | "src/tui/screens/forks.ts|../../actions/follow_words.ts", | |
| 62 | "src/tui/screens/issue.ts|../../actions/dependencies.ts", | |
| 63 | "src/tui/screens/packs.ts|./common.ts", | |
| 64 | "src/tui/screens/system.ts|../../actions/telemetry.ts", | |
| 65 | "src/tui/session.ts|../actions/users.ts", | |
| 66 | "src/web/chrome.ts|../core/repos.ts", | |
| 67 | "src/web/pages/board.ts|../../core/efficiency.ts", | |
| 68 | "src/web/pages/board.ts|../../core/looks.ts", | |
| 69 | "src/web/pages/flow-panel.ts|./boards-held.ts", | |
| 70 | "src/web/pages/following.ts|../../actions/follow_words.ts", | |
| 71 | "src/web/pages/forges.ts|../../spawn/door.ts", | |
| 72 | "src/web/pages/repo.ts|../../actions/follow.ts", | |
| 73 | "src/web/pages/repo.ts|../../core/repos.ts", | |
| 74 | "src/web/pages/runs.ts|../ui.ts", | |
| 75 | "src/web/pages/settings.ts|../../core/repos.ts", | |
| 76 | "src/web/pages/site.ts|../../actions/users.ts", | |
| 77 | "src/web/pages/sources.ts|../../core/apps.ts", | |
| 78 | "src/web/pages/talk.ts|../../core/fence.ts", | |
| 79 | "src/web/pages/teams.ts|../ui.ts", | |
| 80 | "src/web/pages/thread.ts|../../actions/issues.ts", | |
| 81 | "src/web/pages/thread.ts|../../core/issues.ts", | |
| 82 | "src/web/studio/home.ts|../../core/users.ts", | |
| 83 | "test/api-forge-view.test.ts|../src/core/runs.ts", | |
| 84 | "test/api-reader-marks.test.ts|../src/crew/direct.ts", | |
| 85 | "test/app-variants.test.ts|node:path", | |
| 86 | "test/apps-build-preview.test.ts|node:path", | |
| 87 | "test/apps-crew.test.ts|../src/actions/repos.ts", | |
| 88 | "test/apps-measure.test.ts|../src/actions/apps.ts", | |
| 89 | "test/apps-proxy-unavailable.test.ts|../src/apps/proxy.ts", | |
| 90 | "test/apps.test.ts|../src/core/repos.ts", | |
| 91 | "test/asks-bulk.test.ts|../src/actions/issues.ts", | |
| 92 | "test/asks-counts.test.ts|../src/core/reviews.ts", | |
| 93 | "test/autonomy.test.ts|../src/actions/issues.ts", | |
| 94 | "test/autonomy.test.ts|../src/core/issues.ts", | |
| 95 | "test/bets-card-fit.test.ts|../src/spawn/preview-seed.ts", | |
| 96 | "test/bets.test.ts|../src/core/bets.ts", | |
| 97 | "test/bets.test.ts|../src/core/users.ts", | |
| 98 | "test/billing-never-deletes.test.ts|node:fs", | |
| 99 | "test/board-pins.test.ts|../src/boards/pins.ts", | |
| 100 | "test/board-windows.test.ts|../src/core/efficiency.ts", | |
| 101 | "test/boards-query.test.ts|bun:test", | |
| 102 | "test/boards-starting.test.ts|../src/boards/starting.ts", | |
| 103 | "test/builds-rows.test.ts|./helpers/admin.ts", | |
| 104 | "test/business-plan.test.ts|../src/core/issues.ts", | |
| 105 | "test/check-order-dependent.test.ts|../src/actions/repos.ts", | |
| 106 | "test/checks-placement.test.ts|../src/checks/placement.ts", | |
| 107 | "test/child-credit.test.ts|../src/core/users.ts", | |
| 108 | "test/child-credit.test.ts|node:fs/promises", | |
| 109 | "test/client-list.test.ts|../src/core/users.ts", | |
| 110 | "test/conversation-talk.test.ts|../src/core/conversations.ts", | |
| 111 | "test/conversation-talk.test.ts|../src/crew/conversation.ts", | |
| 112 | "test/crew-direction.test.ts|../src/core/repos.ts", | |
| 113 | "test/crew-loop.test.ts|../src/actions/issues.ts", | |
| 114 | "test/crew-loop.test.ts|../src/actions/repos.ts", | |
| 115 | "test/crew-loop.test.ts|../src/core/agents.ts", | |
| 116 | "test/crew-network.test.ts|../src/crew/limits.ts", | |
| 117 | "test/crew-pin-hook.test.ts|node:fs", | |
| 118 | "test/crew-sandbox-escape.test.ts|node:fs", | |
| 119 | "test/crew-sandbox-real.test.ts|node:fs", | |
| 120 | "test/crew-sandbox.test.ts|../src/actions/repos.ts", | |
| 121 | "test/crew-sandbox.test.ts|../src/core/runs.ts", | |
| 122 | "test/crew-sandbox.test.ts|node:fs", | |
| 123 | "test/everyday.test.ts|../src/core/tokens.ts", | |
| 124 | "test/explore-unknown-words.test.ts|../src/actions/explore.ts", | |
| 125 | "test/explore.test.ts|../src/actions/explore.ts", | |
| 126 | "test/fleet-graph-href.test.ts|../src/actions/fleet-graph.ts", | |
| 127 | "test/flow-card.test.ts|../src/tui/screens/stalled.ts", | |
| 128 | "test/following-cache.test.ts|../src/core/users.ts", | |
| 129 | "test/following.test.ts|../src/core/repos.ts", | |
| 130 | "test/forge-budget-verdict.test.ts|../src/tui/screens/crewbudgets.ts", | |
| 131 | "test/guests.test.ts|../src/actions/issues.ts", | |
| 132 | "test/guests.test.ts|../src/core/issues.ts", | |
| 133 | "test/guests.test.ts|../src/core/repos.ts", | |
| 134 | "test/home-builders-idle.test.ts|../src/core/users.ts", | |
| 135 | "test/keys.test.ts|../src/actions/users.ts", | |
| 136 | "test/knobs.test.ts|../src/actions/capacity.ts", | |
| 137 | "test/machine-rc.test.ts|../src/core/users.ts", | |
| 138 | "test/machine-rc.test.ts|node:fs", | |
| 139 | "test/machine-shell.test.ts|../src/machines/shell.ts", | |
| 140 | "test/machine-shell.test.ts|node:fs", | |
| 141 | "test/mcp-org-teams-waiting.test.ts|../src/actions/issue_teams.ts", | |
| 142 | "test/mcp-releases-card.test.ts|../src/web/studio/cards.ts", | |
| 143 | "test/migrate.test.ts|../src/core/issues.ts", | |
| 144 | "test/no-polling.test.ts|bun:test", | |
| 145 | "test/notify.test.ts|../src/crew/notify.ts", | |
| 146 | "test/now-links.test.ts|../src/core/users.ts", | |
| 147 | "test/orgs.test.ts|../src/core/repos.ts", | |
| 148 | "test/origin-pull-state.test.ts|../src/actions/repos.ts", | |
| 149 | "test/packs-up.test.ts|../src/core/repos.ts", | |
| 150 | "test/packs-up.test.ts|../src/core/subscriptions.ts", | |
| 151 | "test/phase-profile.test.ts|../src/perf/phase_profile.ts", | |
| 152 | "test/planner-closes-pull.test.ts|../src/core/issues.ts", | |
| 153 | "test/preview-looks.test.ts|../src/http/server.ts", | |
| 154 | "test/preview-spec.test.ts|../src/core/preview_findings.ts", | |
| 155 | "test/preview-spec.test.ts|../src/http/server.ts", | |
| 156 | "test/private-app.test.ts|../src/web/studio/words.ts", | |
| 157 | "test/proposal-run.test.ts|../src/core/pulls.ts", | |
| 158 | "test/proposal-run.test.ts|../src/core/runs.ts", | |
| 159 | "test/release-boot.test.ts|node:fs", | |
| 160 | "test/repo-transfer.test.ts|../src/core/users.ts", | |
| 161 | "test/repo-transfer.test.ts|node:fs", | |
| 162 | "test/request-access-limits.test.ts|node:fs", | |
| 163 | "test/resilience.test.ts|../src/tui/forge.ts", | |
| 164 | "test/search-is-palette.test.ts|../src/web/palette.ts", | |
| 165 | "test/shards-run.test.ts|node:fs", | |
| 166 | "test/shards-run.test.ts|node:os", | |
| 167 | "test/signup-password.test.ts|../src/core/users.ts", | |
| 168 | "test/spawn-cell.test.ts|../src/apps/contain.ts", | |
| 169 | "test/spawn-self-use.test.ts|../src/core/users.ts", | |
| 170 | "test/starter-crew.test.ts|../src/core/users.ts", | |
| 171 | "test/system-feature-health.test.ts|../src/core/users.ts", | |
| 172 | "test/system-telemetry.test.ts|../src/core/telemetry.ts", | |
| 173 | "test/talk-conversations.test.ts|../src/core/schema.ts", | |
| 174 | "test/talk-steps.test.ts|../src/core/talk_steps.ts", | |
| 175 | "test/tree-window.test.ts|bun:test", | |
| 176 | "test/tui-about-detach.test.ts|../src/actions/detach.ts", | |
| 177 | "test/tui-archive.test.ts|../src/actions/repos.ts", | |
| 178 | "test/tui-ask-data.test.ts|../src/tui/screens/ask_data.ts", | |
| 179 | "test/tui-claim-start.test.ts|../src/tui/screens/claim_start.ts", | |
| 180 | "test/tui-data.test.ts|../src/tui/forge.ts", | |
| 181 | "test/tui-transfer-repo.test.ts|../src/core/repos.ts", | |
| 182 | "test/upstream_follow.test.ts|../src/core/repos.ts", | |
| 183 | "test/upstream_follow.test.ts|../src/core/runs.ts", | |
| 184 | "test/upstream_follow.test.ts|../src/crew/autonomy.ts", | |
| 185 | "test/vitals.test.ts|../src/kernel/vitals.ts", | |
| 186 | "test/web.test.ts|../src/web/asset.ts", | |
| 187 | "test/webhook-failing.test.ts|../src/core/users.ts", | |
| 188 | "test/week-sprint.test.ts|../src/flow/week.ts" | |
| 189 | ] |
scripts/gate.ts +3 −1
| @@ -238,7 +238,9 @@ export function reviewRefusal(root = "."): string[] { | ||
| 238 | 238 | .filter((f) => f.endsWith(".ts") && !f.includes("vendor/")) |
| 239 | 239 | .map((f) => ({ path: `src/${f}`, text: readFileSync(join(src, f), "utf8") })) |
| 240 | 240 | : []; |
| 241 | return reviewProblems([...tests, ...code]); | |
| 241 | const allowFile = join(root, "scripts/duplicate-imports-allow.json"); | |
| 242 | const allowed = existsSync(allowFile) ? (JSON.parse(readFileSync(allowFile, "utf8")) as string[]) : []; | |
| 243 | return reviewProblems([...tests, ...code], allowed); | |
| 242 | 244 | } |
| 243 | 245 | |
| 244 | 246 | /** |
src/kernel/review_rules.ts +48 −2
| @@ -13,6 +13,14 @@ | ||
| 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 | * - "a module is imported once per file" (#11517, four reviews in a week: | |
| 17 | * perf/live.ts, ../chrome.ts, ../core/runs.ts): a second `import {…} from | |
| 18 | * "x"` of the same module, in the same kind (value or `import type`), is | |
| 19 | * merged into the first. Namespace and side-effect imports cannot merge and | |
| 20 | * are skipped. Applies to `src/` and `test/`. Offenders that predate the | |
| 21 | * rule sit in `scripts/duplicate-imports-allow.json` (`path|module`): a new | |
| 22 | * one fails, and so does an entry whose duplicate was fixed (delete it), so | |
| 23 | * the list only shrinks. | |
| 16 | 24 | */ |
| 17 | 25 | |
| 18 | 26 | export type Source = { path: string; text: string }; |
| @@ -25,10 +33,42 @@ const SRC_FILE = /^src\/(?!.*\/vendor\/).+\.ts$/; | ||
| 25 | 33 | // A const, let, var or function whose name ends in one underscore. |
| 26 | 34 | const UNDERSCORED = /\b(?:const|let|var|function)\s+([A-Za-z][A-Za-z0-9]*_)(?![A-Za-z0-9_])/g; |
| 27 | 35 | |
| 28 | /** One line per problem, each naming its rule, file and line. */ | |
| 29 | export function reviewProblems(sources: readonly Source[]): string[] { | |
| 36 | // `import X from "m"`, `import { a } from "m"`, `import type { T } from "m"`; not `* as`, not bare. | |
| 37 | const NAMED_IMPORT = /^import\s+(type\s+)?(?!\*)([^;"'\n][^;"']*?)\s+from\s+"([^"]+)"/gm; | |
| 38 | ||
| 39 | /** Every second-or-later import of a module already imported in the same kind: `path|module`, with its line. */ | |
| 40 | export function duplicateImports(path: string, text: string): { key: string; line: number; module: string }[] { | |
| 41 | if (!/^(?:src|test)\/.+\.ts$/.test(path) || /\/vendor\//.test(path)) return []; | |
| 42 | const seen = new Set<string>(); | |
| 43 | const out: { key: string; line: number; module: string }[] = []; | |
| 44 | for (const m of text.matchAll(NAMED_IMPORT)) { | |
| 45 | const kind = `${m[1] ? "type " : ""}${m[3]}`; | |
| 46 | if (seen.has(kind)) { | |
| 47 | const key = `${path}|${kind}`; | |
| 48 | if (!out.some((o) => o.key === key)) | |
| 49 | out.push({ key, line: text.slice(0, m.index).split("\n").length, module: m[3]! }); | |
| 50 | } | |
| 51 | seen.add(kind); | |
| 52 | } | |
| 53 | return out; | |
| 54 | } | |
| 55 | ||
| 56 | /** | |
| 57 | * One line per problem, each naming its rule, file and line. `allowed` is the | |
| 58 | * baseline of duplicate imports that predate their rule. | |
| 59 | */ | |
| 60 | export function reviewProblems(sources: readonly Source[], allowed: readonly string[] = []): string[] { | |
| 30 | 61 | const out: string[] = []; |
| 62 | const found = new Set<string>(); | |
| 63 | const ok = new Set(allowed); | |
| 31 | 64 | for (const { path, text } of sources) { |
| 65 | for (const d of duplicateImports(path, text)) { | |
| 66 | found.add(d.key); | |
| 67 | if (!ok.has(d.key)) | |
| 68 | out.push( | |
| 69 | `rule "a module is imported once per file": ${path}:${d.line} imports ${d.module} again; merge it into the first import of that module`, | |
| 70 | ); | |
| 71 | } | |
| 32 | 72 | if (SRC_FILE.test(path)) |
| 33 | 73 | text.split("\n").forEach((line, i) => { |
| 34 | 74 | for (const m of line.matchAll(UNDERSCORED)) |
| @@ -44,5 +84,11 @@ export function reviewProblems(sources: readonly Source[]): string[] { | ||
| 44 | 84 | ); |
| 45 | 85 | }); |
| 46 | 86 | } |
| 87 | const present = new Set(sources.map((x) => x.path)); | |
| 88 | for (const key of allowed) | |
| 89 | if (present.has(key.split("|")[0]!) && !found.has(key)) | |
| 90 | out.push( | |
| 91 | `rule "a module is imported once per file": ${key.split("|")[0]} no longer repeats ${key.split("|")[1]}; delete "${key}" from scripts/duplicate-imports-allow.json`, | |
| 92 | ); | |
| 47 | 93 | return out; |
| 48 | 94 | } |
test/review-rules.test.ts +24
| @@ -39,6 +39,30 @@ test("a declared name ending in an underscore is refused in src, naming the rule | ||
| 39 | 39 | expect(reviewProblems([{ path: "src/web/render/vendor/y.ts", text: "const v_ = 1;" }])).toEqual([]); |
| 40 | 40 | }); |
| 41 | 41 | |
| 42 | test("a second import of a module is refused, naming the rule, file and line", () => { | |
| 43 | const text = 'import { a } from "./perf/live.ts";\nimport { b } from "./perf/traffic.ts";\nimport {\n c,\n} from "./perf/live.ts";\n'; | |
| 44 | const [p, ...rest] = reviewProblems([{ path: "src/a.ts", text }]); | |
| 45 | expect(rest).toEqual([]); | |
| 46 | expect(p).toContain('rule "a module is imported once per file"'); | |
| 47 | expect(p).toContain("src/a.ts:3"); | |
| 48 | expect(p).toContain("perf/live.ts"); | |
| 49 | expect(reviewProblems([{ path: "test/a.test.ts", text }])).toHaveLength(1); | |
| 50 | }); | |
| 51 | ||
| 52 | test("imports that cannot merge, or differ in kind, pass", () => { | |
| 53 | const text = | |
| 54 | 'import type { T } from "./x.ts";\nimport { v } from "./x.ts";\nimport * as ns from "./y.ts";\nimport * as ns2 from "./y.ts";\nimport "./z.ts";\nimport "./z.ts";\n'; | |
| 55 | expect(reviewProblems([{ path: "src/a.ts", text }])).toEqual([]); | |
| 56 | }); | |
| 57 | ||
| 58 | test("a baselined duplicate passes, and a baseline entry that no longer repeats is refused", () => { | |
| 59 | const text = 'import { a } from "./x.ts";\nimport { b } from "./x.ts";\n'; | |
| 60 | expect(reviewProblems([{ path: "src/a.ts", text }], ["src/a.ts|./x.ts"])).toEqual([]); | |
| 61 | const [p] = reviewProblems([{ path: "src/a.ts", text: 'import { a, b } from "./x.ts";\n' }], ["src/a.ts|./x.ts"]); | |
| 62 | expect(p).toContain("delete"); | |
| 63 | expect(p).toContain("scripts/duplicate-imports-allow.json"); | |
| 64 | }); | |
| 65 | ||
| 42 | 66 | test("the repository's own tests pass the rules", () => { |
| 43 | 67 | expect(reviewRefusal()).toEqual([]); |
| 44 | 68 | expect(readdirSync("test").length).toBeGreaterThan(0); |
new above
offline