Public: anyone can read this; the crew can change it.
A review's opening count ("Two things to fix, first one first.") is a lead-in, not a finding, so no issue asks to automate it #9335
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9322-a-review-s-opening-count-two-things-to-f → main 1 commit fork
2 files
changed +44
−3
src/flow/repeats.ts +18 −2
| @@ -68,17 +68,33 @@ export function wordsOf(text: string): string[] { | ||
| 68 | 68 | export const isLeadIn = (line: string): boolean => |
| 69 | 69 | line.replace(/^[\s*_]+|[\s*_]+$/g, "").endsWith(":"); |
| 70 | 70 | |
| 71 | const COUNT = /^(?:one|two|three|four|five|six|seven|eight|nine|ten|\d+)\s+(?:things?|points?|issues?|fix(?:es)?|notes?)\b/i; | |
| 72 | ||
| 73 | /** | |
| 74 | * Rule "a count is not a finding" (#9322), beside #8838's: a line that only | |
| 75 | * counts or orders what follows opens a review and asks for nothing: "Two | |
| 76 | * things to fix, first one first.", "Three points, one round." It starts with a | |
| 77 | * number word or digit and things/points/issues/fixes/notes, names no code (no | |
| 78 | * backtick, no path) and is under 80 characters. "Two tests share a port; give | |
| 79 | * each its own" is kept: it says what to change. | |
| 80 | */ | |
| 81 | export const isCount = (line: string): boolean => { | |
| 82 | const bare = line.replace(/^[\s*_]+|[\s*_]+$/g, ""); | |
| 83 | return bare.length < 80 && !bare.includes("`") && !/[\w-]\/[\w-]|\.(?:ts|js|md|json|css)\b/.test(bare) && COUNT.test(bare); | |
| 84 | }; | |
| 85 | ||
| 71 | 86 | /** |
| 72 | 87 | * The findings in one review body: its list items or lines, each a request. |
| 73 | 88 | * A line that ends in a colon introduces the list that follows ("Smaller |
| 74 | 89 | * points, fix in the same round:"), asks for nothing, and is dropped: as a |
| 75 | * finding it repeated across every review that used it (#8838). | |
| 90 | * finding it repeated across every review that used it (#8838). So is a line | |
| 91 | * that only counts what follows ("Two things to fix, first one first.", #9322). | |
| 76 | 92 | */ |
| 77 | 93 | export function linesOf(body: string): string[] { |
| 78 | 94 | return body |
| 79 | 95 | .split("\n") |
| 80 | 96 | .map((l) => l.replace(/^\s*(?:[-*]|\d+[.)])\s+/, "").trim()) |
| 81 | .filter((l) => l.length >= 12 && !isLeadIn(l)) | |
| 97 | .filter((l) => l.length >= 12 && !isLeadIn(l) && !isCount(l)) | |
| 82 | 98 | .slice(0, MAX_LINES) |
| 83 | 99 | .map((l) => l.slice(0, MAX_LINE)); |
| 84 | 100 | } |
test/review-repeats.test.ts +26 −1
| @@ -10,7 +10,7 @@ import type { Repo } from "../src/core/repos.ts"; | ||
| 10 | 10 | import { createUser, type User } from "../src/core/users.ts"; |
| 11 | 11 | import { promote } from "./helpers/members.ts"; |
| 12 | 12 | import { sweepRepeats } from "../src/crew/repeat_checks.ts"; |
| 13 | import { group, linesOf, readRepeats, REPEATS, wordsOf } from "../src/flow/repeats.ts"; | |
| 13 | import { dueGroups, group, linesOf, readRepeats, REPEATS, wordsOf } from "../src/flow/repeats.ts"; | |
| 14 | 14 | import { flowFor } from "../src/system/telemetry.ts"; |
| 15 | 15 | import { repeatLines } from "../src/tui/screens/flow.ts"; |
| 16 | 16 | |
| @@ -193,3 +193,28 @@ describe("a lead-in line is not a finding (#8838)", () => { | ||
| 193 | 193 | expect(linesOf("**High:** `flowOnly` is missing from the agent refusal.")).toEqual(["**High:** `flowOnly` is missing from the agent refusal."]); |
| 194 | 194 | }); |
| 195 | 195 | }); |
| 196 | ||
| 197 | describe("a count is not a finding (#9322)", () => { | |
| 198 | const openers = ["Two things to fix, first one first.", "Two things to fix in one round.", "Two things to fix, one round.", "Two things to fix, in one round."]; | |
| 199 | ||
| 200 | test("linesOf drops a line that only counts what follows, and keeps a request that starts with a count", () => { | |
| 201 | for (const line of [...openers, "Three points, one round.", "**Two things to fix, first one first.**", "2 issues, both small."]) | |
| 202 | expect(linesOf(line)).toEqual([]); | |
| 203 | expect(linesOf("Two things to fix, first one first.\n- Escape the user name in the page.")).toEqual(["Escape the user name in the page."]); | |
| 204 | for (const kept of ["Two tests share a port; give each its own", "Two things in `src/flow/repeats.ts` need a test.", "Two fixes in src/flow/repeats.ts must land together."]) | |
| 205 | expect(linesOf(kept)).toEqual([kept]); | |
| 206 | }); | |
| 207 | ||
| 208 | test("a week of reviews opening with those lines makes no due group", async () => { | |
| 209 | const r = await createRepo(app, builder, { name: "counts", visibility: "public" }); | |
| 210 | if (!r.ok) throw new Error(String(r.error)); | |
| 211 | for (const [i, body] of openers.entries()) { | |
| 212 | const pull = createIssue(app.db, { repoId: r.value.id, authorId: builder.id, title: "work", kind: "pull" }); | |
| 213 | if (!pull.ok) throw new Error(pull.error); | |
| 214 | app.db | |
| 215 | .query(`INSERT INTO review (id, issue_id, author_id, state, body, created_at) VALUES (?1, ?2, ?3, 'changes_requested', ?4, ?5)`) | |
| 216 | .run(crypto.randomUUID(), pull.value.id, reviewer.id, body, new Date(now.getTime() - (i + 1) * 3_600_000).toISOString()); | |
| 217 | } | |
| 218 | expect(await dueGroups(app.db, r.value.id, now)).toEqual([]); | |
| 219 | }); | |
| 220 | }); |
new above
offline