Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: The pull is a billing feature while the direction is pipeline-first; keep it sma #12365
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/12363-automate-a-review-finding-as-a-lint-or-t → main 4 commits fork 2 threads · 2 unresolved
3 files
changed +37
−3
src/kernel/direction.ts +22 −3
| @@ -5,12 +5,20 @@ | ||
| 5 | 5 | * touches no pipeline code (`scripts/`, `src/flow/`, `src/nightly/`, |
| 6 | 6 | * `src/checks/`, `forge.json`) must say why it goes ahead of the pipeline |
| 7 | 7 | * items, as `Direction: <why>` in a commit message. |
| 8 | * | |
| 9 | * Four more reviews (#11289, #11696, #12177, #12307) accepted that waiver and | |
| 10 | * still said "keep it small and single-purpose" (#12363). So the waiver buys | |
| 11 | * a small pull only: at most `MAX_FEATURES` added feature files and | |
| 12 | * `MAX_FILES` changed files in all. | |
| 8 | 13 | */ |
| 9 | 14 | |
| 10 | 15 | import type { Change } from "./pin_edits.ts"; |
| 11 | 16 | |
| 12 | 17 | const FEATURE = /^(src\/web\/pages\/[^/]+\.ts|src\/tui\/.+\.ts|docs\/features\/[^/]+\.md)$/; |
| 13 | 18 | const PIPELINE = /^(scripts\/|src\/flow\/|src\/nightly\/|src\/checks\/|forge\.json$)/; |
| 19 | /** A feature pull that goes ahead of the pipeline: one page and its how-to. */ | |
| 20 | export const MAX_FEATURES = 2; | |
| 21 | export const MAX_FILES = 20; | |
| 14 | 22 | const WAIVER = /^Direction:[ \t]*\S[^\n]{9,}/im; |
| 15 | 23 | |
| 16 | 24 | /** One line per problem: none unless a feature is added with no pipeline change and no Direction line. */ |
| @@ -18,9 +26,20 @@ export function directionProblems(changes: readonly Change[], messages: readonly | ||
| 18 | 26 | const features = changes.filter((c) => c.status.startsWith("A") && FEATURE.test(c.path)).map((c) => c.path); |
| 19 | 27 | if (features.length === 0) return []; |
| 20 | 28 | if (changes.some((c) => PIPELINE.test(c.path))) return []; |
| 21 | if (messages.some((m) => WAIVER.test(m))) return []; | |
| 22 | const shown = features.slice(0, 3).join(", ") + (features.length > 3 ? ` and ${features.length - 3} more` : ""); | |
| 29 | if (!messages.some((m) => WAIVER.test(m))) return [problem(features)]; | |
| 30 | const big: string[] = []; | |
| 31 | if (features.length > MAX_FEATURES) | |
| 32 | big.push(`it adds ${features.length} feature files (at most ${MAX_FEATURES})`); | |
| 33 | if (changes.length > MAX_FILES) big.push(`it changes ${changes.length} files (at most ${MAX_FILES})`); | |
| 34 | if (big.length === 0) return []; | |
| 23 | 35 | return [ |
| 24 | `rule "pipeline first": this pull adds a feature (${shown}) and no pipeline change (scripts/, src/flow/, src/nightly/, src/checks/, forge.json); change the pipeline, or put a "Direction: <why this goes ahead of the pipeline items>" line (10+ characters) in a commit message`, | |
| 36 | `rule "small and single-purpose": a feature that goes ahead of the pipeline stays small, and ${big.join(" and ")}; split it into pulls of one purpose each, or change the pipeline`, | |
| 25 | 37 | ]; |
| 26 | 38 | } |
| 39 | ||
| 40 | function problem(features: readonly string[]): string { | |
| 41 | const shown = features.slice(0, 3).join(", ") + (features.length > 3 ? ` and ${features.length - 3} more` : ""); | |
| 42 | return ( | |
| 43 | `rule "pipeline first": this pull adds a feature (${shown}) and no pipeline change (scripts/, src/flow/, src/nightly/, src/checks/, forge.json); change the pipeline, or put a "Direction: <why this goes ahead of the pipeline items>" line (10+ characters) in a commit message` | |
| 44 | ); | |
| 45 | } |
test/data-browser.test.ts +2
| @@ -23,6 +23,7 @@ quietLogs(); | ||
| 23 | 23 | |
| 24 | 24 | const AUDITED_CLEAR: string[] = [ |
| 25 | 25 | "app_domain.said", |
| 26 | "app_env.value_plain", | |
| 26 | 27 | "app_feature_setting.value", |
| 27 | 28 | "ask_filter.value", |
| 28 | 29 | // Which hosted forge an ask or a grant is for (`core/child_reach.ts`): a spawn's body id, or `*` for every one. |
| @@ -45,6 +46,7 @@ const AUDITED_CLEAR: string[] = [ | ||
| 45 | 46 | "report.body", |
| 46 | 47 | "review.body", |
| 47 | 48 | "run.check_detail", |
| 49 | "secret_start.value_updated_at", | |
| 48 | 50 | "subscription.resting_said", |
| 49 | 51 | "subscription_limit.said", |
| 50 | 52 | "user_key.blob", |
test/direction.test.ts +13
| @@ -26,3 +26,16 @@ test("edits and non-feature files are never asked", () => { | ||
| 26 | 26 | test("the reason must sit on the Direction line itself, not a later paragraph", () => { |
| 27 | 27 | expect(directionProblems([page], ["Direction:\n\nsome later paragraph of text"])).toHaveLength(1); |
| 28 | 28 | }); |
| 29 | ||
| 30 | test("a waived feature pull stays small and single-purpose, naming the rule", () => { | |
| 31 | const why = ["Direction: a small door onto the spawn path, owner asked on #7642"]; | |
| 32 | const pages = ["a", "b", "c"].map((n) => ch("A", `src/web/pages/${n}.ts`)); | |
| 33 | const many = pages.slice(0, 1).concat(Array.from({ length: 20 }, (_, i) => ch("M", `src/core/m${i}.ts`))); | |
| 34 | expect(directionProblems([page, ch("A", "docs/features/x.md")], why)).toEqual([]); | |
| 35 | const wide = directionProblems(pages, why); | |
| 36 | expect(wide).toHaveLength(1); | |
| 37 | expect(wide[0]).toContain('rule "small and single-purpose"'); | |
| 38 | expect(wide[0]).toContain("3 feature files"); | |
| 39 | expect(directionProblems(many, why)[0]).toContain("changes 21 files"); | |
| 40 | expect(directionProblems([...many, ch("M", "scripts/gate.ts")], why)).toEqual([]); | |
Add a case at exactly MAX_FILES (20 changes passes) so the boundary is pinned, not only 21. |
||
| 41 | }); |
new above
offline
Unrelated to this rule: it comes from the main merge. Mention it in the pull description.