Public: anyone can read this; the crew can change it.
Reviews go to the least-busy reviewer, and the README says how a reviewer is chosen #4734
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4732-reviews-go-to-the-least-busy-reviewer-an → main 1 commit fork 1 thread · 1 unresolved
3 files
changed +100
−12
README.md +6
| @@ -1176,6 +1176,12 @@ is one, and its review brief holds the change to `docs/DESIGN.md`. A security | ||
| 1176 | 1176 | change still goes to the security reviewer; a change only under `src/core/` is |
| 1177 | 1177 | not a design matter. |
| 1178 | 1178 | |
| 1179 | **How a reviewer is chosen.** Reviews go to the least-busy reviewer: the one | |
| 1180 | with the fewest review runs in flight (queued or running). A tie goes to the | |
| 1181 | experience reviewer. A pull about security goes only to a security reviewer | |
| 1182 | (the least busy of them). A `src/web/` pull goes to the design reviewer until | |
| 1183 | it has 2 review runs in flight, then spills to the least busy reviewer. | |
| 1184 | ||
| 1179 | 1185 | **Three rounds, then a person.** When the crew reviewer has asked for |
| 1180 | 1186 | changes three times on one pull with no approval and no word from a person |
| 1181 | 1187 | who may direct the crew between, no more feedback runs start on it. The |
src/crew/review.ts +45 −12
| @@ -53,27 +53,59 @@ const focusOf = (brief: string): string => | ||
| 53 | 53 | export const watchesSecurity = (agent: Agent): boolean => |
| 54 | 54 | /\bsecurity\b/i.test(focusOf(agent.brief)); |
| 55 | 55 | |
| 56 | /** Review runs a design reviewer holds in flight before a `src/web/` pull spills to the least busy. */ | |
| 57 | export const REVIEW_DESIGN_INFLIGHT = 2; | |
| 58 | ||
| 59 | /** Review runs in flight (queued or running) per reviewer, by user id; an idle reviewer is absent. */ | |
| 60 | export function reviewLoad(db: Database): Map<string, number> { | |
| 61 | const load = new Map<string, number>(); | |
| 62 | for (const r of db | |
| 63 | .query<{ actor_id: string; n: number }, []>( | |
| 64 | `SELECT actor_id, COUNT(*) AS n FROM run | |
| 65 | WHERE kind = 'agent' AND reason = 'review' AND actor_id IS NOT NULL | |
| 66 | AND state IN ('queued','running') | |
| 67 | GROUP BY actor_id`, | |
| 68 | ) | |
| 69 | .all()) | |
| 70 | load.set(r.actor_id, r.n); | |
| 71 | return load; | |
| 72 | } | |
| 73 | ||
| 56 | 74 | /** |
| 57 | * The reviewer for a change to these paths: the security reviewer when | |
| 58 | * the change touches what it watches, the experience reviewer otherwise, | |
| 59 | * and whichever reviewer there is when the crew has only one kind. A | |
| 60 | * change to `src/web/` that is not security's goes to a reviewer on the | |
| 61 | * design team (`design`, user ids) when the crew has one, and its brief | |
| 62 | * holds it to docs/DESIGN.md (`touchesDesign`). | |
| 75 | * The reviewer for a change to these paths, the least busy by `load` | |
| 76 | * (`reviewLoad`). A security change goes only to a security reviewer | |
| 77 | * (least busy among them) when the crew has one. Any other goes to the | |
| 78 | * least busy reviewer, a tie to the experience reviewer, then crew order. | |
| 79 | * A change to `src/web/` that is not security's goes first to a reviewer | |
| 80 | * on the design team (`design`, user ids) while it has fewer than | |
| 81 | * `REVIEW_DESIGN_INFLIGHT` in flight, then spills to the least busy. | |
| 63 | 82 | */ |
| 64 | 83 | export function pickReviewer( |
| 65 | 84 | reviewers: readonly Agent[], |
| 66 | 85 | paths: readonly string[], |
| 67 | 86 | design: readonly string[] = [], |
| 87 | load: ReadonlyMap<string, number> = new Map(), | |
| 68 | 88 | ): Agent | undefined { |
| 69 | if (touchesDesign(paths) && !touchesSecurity(paths)) { | |
| 70 | const member = reviewers.find((r) => design.includes(r.user.id)); | |
| 89 | const busy = (r: Agent) => load.get(r.user.id) ?? 0; | |
| 90 | const leastBusy = (from: readonly Agent[]): Agent | undefined => | |
| 91 | from.reduce<Agent | undefined>( | |
| 92 | (best, r) => | |
| 93 | best === undefined || | |
| 94 | busy(r) < busy(best) || | |
| 95 | (busy(r) === busy(best) && watchesSecurity(best) && !watchesSecurity(r)) | |
| 96 | ? r | |
| 97 | : best, | |
| 98 | undefined, | |
| 99 | ); | |
| 100 | const security = reviewers.filter(watchesSecurity); | |
| 101 | if (touchesSecurity(paths)) return leastBusy(security) ?? leastBusy(reviewers); | |
| 102 | if (touchesDesign(paths)) { | |
| 103 | const member = leastBusy( | |
| 104 | reviewers.filter((r) => design.includes(r.user.id) && busy(r) < REVIEW_DESIGN_INFLIGHT), | |
| 105 | ); | |
| 71 | 106 | if (member !== undefined) return member; |
| 72 | 107 | } |
| 73 | const security = reviewers.filter(watchesSecurity); | |
| 74 | const experience = reviewers.filter((r) => !watchesSecurity(r)); | |
| 75 | const preferred = touchesSecurity(paths) ? security : experience; | |
| 76 | return preferred[0] ?? reviewers[0]; | |
| 108 | return leastBusy(reviewers); | |
| 77 | 109 | } |
| 78 | 110 | |
| 79 | 111 | /** The crew's reviewers on a repository. */ |
| @@ -431,6 +463,7 @@ export async function sendToReview( | ||
| 431 | 463 | others.length > 0 ? others : reviewers, |
| 432 | 464 | diff.files.map((f) => f.path), |
| 433 | 465 | getTeam(app.db, "design")?.members, |
| 466 | reviewLoad(app.db), | |
| 434 | 467 | ); |
| 435 | 468 | if (reviewer === undefined) return "wait"; |
| 436 | 469 | if (queueAgentRun(app, repo, issue, reviewer, "review") === undefined) |
test/review-route.test.ts +49
| @@ -22,6 +22,7 @@ import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts"; | ||
| 22 | 22 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; |
| 23 | 23 | import { |
| 24 | 24 | pickReviewer, |
| 25 | reviewLoad, | |
| 25 | 26 | REVIEW_LAPSE_MS, |
| 26 | 27 | REVIEW_WAIT_MS, |
| 27 | 28 | reviewStage, |
| @@ -291,3 +292,51 @@ describe("the design team reviews a change to a page", () => { | ||
| 291 | 292 | expect(pickReviewer([juniper], ["src/web/pages/x.ts"], team)).toBe(juniper); |
| 292 | 293 | }); |
| 293 | 294 | }); |
| 295 | ||
| 296 | describe("the least-busy reviewer", () => { | |
| 297 | const exp = { user: { id: "e1" }, brief: `I am Exp. I read for the experience.\n\n${SHARED}` } as unknown as Agent; | |
| 298 | const exp2 = { user: { id: "e2" }, brief: `I am Exp2. I read for the experience.\n\n${SHARED}` } as unknown as Agent; | |
| 299 | const sec = { user: { id: "s1" }, brief: `I am Sec. I read for security.\n\n${SHARED}` } as unknown as Agent; | |
| 300 | const des = { user: { id: "d1" }, brief: `I am Des. I review the look.\n\n${SHARED}` } as unknown as Agent; | |
| 301 | const load = (o: Record<string, number>) => new Map(Object.entries(o)); | |
| 302 | ||
| 303 | test("the least busy wins, and a tie goes to the experience reviewer", () => { | |
| 304 | expect(pickReviewer([exp, exp2], ["README.md"], [], load({ e1: 3, e2: 1 }))).toBe(exp2); | |
| 305 | expect(pickReviewer([sec, exp], ["README.md"], [], load({ s1: 1, e1: 1 }))).toBe(exp); | |
| 306 | expect(pickReviewer([sec, exp], ["README.md"], [], load({ s1: 0, e1: 2 }))).toBe(sec); | |
| 307 | }); | |
| 308 | ||
| 309 | test("a security pull stays with security, least busy among them", () => { | |
| 310 | const sec2 = { ...sec, user: { id: "s2" } } as Agent; | |
| 311 | expect(pickReviewer([exp, sec], ["src/core/tokens.ts"], [], load({ s1: 9 }))).toBe(sec); | |
| 312 | expect(pickReviewer([sec, sec2, exp], ["src/core/tokens.ts"], [], load({ s1: 4, s2: 1 }))).toBe(sec2); | |
| 313 | }); | |
| 314 | ||
| 315 | test("design takes src/web/ until 2 are in flight, then it spills", () => { | |
| 316 | const crew = [exp, des]; | |
| 317 | const p = ["src/web/pages/x.ts"]; | |
| 318 | expect(pickReviewer(crew, p, ["d1"], load({ d1: 1 }))).toBe(des); | |
| 319 | expect(pickReviewer(crew, p, ["d1"], load({ d1: 2 }))).toBe(exp); | |
| 320 | expect(pickReviewer(crew, p, ["d1"], load({ d1: 2, e1: 5 }))).toBe(des); | |
| 321 | }); | |
| 322 | ||
| 323 | test("reviewLoad counts only review runs still in flight, per reviewer", async () => { | |
| 324 | const dir = await mkdtemp(join(tmpdir(), "forge-load-")); | |
| 325 | const app = createApp({ dataDir: dir, port: 0 }); | |
| 326 | try { | |
| 327 | expect(reviewLoad(app.db).size).toBe(0); | |
| 328 | app.db.run("PRAGMA foreign_keys = OFF"); | |
| 329 | const ins = app.db.prepare( | |
| 330 | "INSERT INTO run (id, repo_id, kind, state, reason, actor_id) VALUES (?, 'r', 'agent', ?, ?, ?)", | |
| 331 | ); | |
| 332 | ins.run("a", "queued", "review", "e1"); | |
| 333 | ins.run("b", "running", "review", "e1"); | |
| 334 | ins.run("c", "ok", "review", "e1"); | |
| 335 | ins.run("d", "running", "feedback", "e1"); | |
| 336 | ins.run("e", "queued", "review", "s1"); | |
| 337 | expect(Object.fromEntries(reviewLoad(app.db))).toEqual({ e1: 2, s1: 1 }); | |
| 338 | } finally { | |
| 339 | await rm(dir, { recursive: true, force: true }); | |
| 340 | } | |
| 341 | }); | |
| 342 | }); |
new above
offline
The security fallback to leastBusy(reviewers) when no security reviewer exists is right, but no test covers it. Add one: pickReviewer([exp], ["src/core/tokens.ts"]) returns exp.