Public: anyone can read this; the crew can change it.
The design team reviews every pull that changes a page against docs/DESIGN.md #2840
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2683-the-design-team-reviews-every-pull-that → main 2 commits fork 3 threads · 3 unresolved
6 files
changed +144
−4
README.md +6
| @@ -864,6 +864,12 @@ feedback on the pull — a comment, a request for changes, a line comment — an | ||
| 864 | 864 | the agent picks it up in the background, resuming its session. A failed check re-engages it too, a bounded number of |
| 865 | 865 | times before a person must speak. |
| 866 | 866 | |
| 867 | **Design review.** A pull that changes anything under `src/web/` (a page, the ui | |
| 868 | vocabulary, the style) is sent to a crew reviewer on the Design team when there | |
| 869 | is one, and its review brief holds the change to `docs/DESIGN.md`. A security | |
| 870 | change still goes to the security reviewer; a change only under `src/core/` is | |
| 871 | not a design matter. | |
| 872 | ||
| 867 | 873 | **Three rounds, then a person.** When the crew reviewer has asked for |
| 868 | 874 | changes three times on one pull with no approval and no word from a person |
| 869 | 875 | who may direct the crew between, no more feedback runs start on it. The |
src/crew/brief.ts +7
| @@ -194,6 +194,8 @@ export interface TaskInput extends BriefInput { | ||
| 194 | 194 | | undefined; |
| 195 | 195 | /** For `review`: the pull's diff against its base, capped. */ |
| 196 | 196 | readonly reviewDiff?: string | undefined; |
| 197 | /** For `review`: the change touches `src/web/`, so the review is held to docs/DESIGN.md. */ | |
| 198 | readonly design?: boolean | undefined; | |
| 197 | 199 | /** For `review`: an earlier review run on this pull ended with no verdict. */ |
| 198 | 200 | readonly retry?: boolean | undefined; |
| 199 | 201 | /** For `review`: pictures of the preview at this head, phone and desktop. */ |
| @@ -352,6 +354,11 @@ function taskBody(input: TaskInput): string { | ||
| 352 | 354 | "- Tests: every behaviour the change adds has a test that would fail without it, and `bun test` is green in the diff.", |
| 353 | 355 | "- Both doors: what a person can do on the page they can do in `src/tty.ts` too, and the README says how to use it.", |
| 354 | 356 | "- Clean: small modules named after one concept, no dead code, no commented-out code, no TODOs left as work, no new dependency without a reason in the pull.", |
| 357 | ...(input.design === true | |
| 358 | ? [ | |
| 359 | "- The design system: this change touches src/web/, so you read it as the design team. Read docs/DESIGN.md first, then hold the change to it strictly — tokens and the named components rather than hand-rolled markup, one accent per page, anything that navigates an <a> and anything that mutates a <button> in a post form, everything working with JavaScript off. A page that departs from docs/DESIGN.md is a change request, whatever else it does well.", | |
| 360 | ] | |
| 361 | : []), | |
| 355 | 362 | "- True to the direction: it does what the issue asked and nothing the direction did not, and the preview shows it working.", |
| 356 | 363 | ...(input.policy === undefined |
| 357 | 364 | ? [] |
src/crew/review.ts +15 −1
| @@ -26,6 +26,7 @@ import { listPulls, pullDiff, type Pull, pullDir} from "../core/pulls.ts"; | ||
| 26 | 26 | import { getRepoById, type Repo } from "../core/repos.ts"; |
| 27 | 27 | import { reviewSummary } from "../core/reviews.ts"; |
| 28 | 28 | import { checksForHead } from "../core/runs.ts"; |
| 29 | import { getTeam } from "../core/teams.ts"; | |
| 29 | 30 | import { repoDir } from "../git/repos.ts"; |
| 30 | 31 | import { createLog } from "../kernel/log.ts"; |
| 31 | 32 | import { queueAgentRun } from "./dispatch.ts"; |
| @@ -39,6 +40,10 @@ const SECURITY_PATH = /auth|token|secret|sandbox|http/i; | ||
| 39 | 40 | export const touchesSecurity = (paths: readonly string[]): boolean => |
| 40 | 41 | paths.some((p) => SECURITY_PATH.test(p)); |
| 41 | 42 | |
| 43 | /** Paths whose change is a page's look: a page, the ui vocabulary or the style. */ | |
| 44 | export const touchesDesign = (paths: readonly string[]): boolean => | |
| 45 | paths.some((p) => p.startsWith("src/web/")); | |
| 46 | ||
| 42 | 47 | /** What an agent says it is: its brief's first paragraph. */ |
| 43 | 48 | const focusOf = (brief: string): string => |
| 44 | 49 | brief.trim().split(/\n\s*\n/)[0] ?? ""; |
| @@ -50,12 +55,20 @@ export const watchesSecurity = (agent: Agent): boolean => | ||
| 50 | 55 | /** |
| 51 | 56 | * The reviewer for a change to these paths: the security reviewer when |
| 52 | 57 | * the change touches what it watches, the experience reviewer otherwise, |
| 53 | * and whichever reviewer there is when the crew has only one kind. | |
| 58 | * and whichever reviewer there is when the crew has only one kind. A | |
| 59 | * change to `src/web/` that is not security's goes to a reviewer on the | |
| 60 | * design team (`design`, user ids) when the crew has one, and its brief | |
| 61 | * holds it to docs/DESIGN.md (`touchesDesign`). | |
| 54 | 62 | */ |
| 55 | 63 | export function pickReviewer( |
| 56 | 64 | reviewers: readonly Agent[], |
| 57 | 65 | paths: readonly string[], |
| 66 | design: readonly string[] = [], | |
| 58 | 67 | ): Agent | undefined { |
| 68 | if (touchesDesign(paths) && !touchesSecurity(paths)) { | |
| 69 | const member = reviewers.find((r) => design.includes(r.user.id)); | |
| 70 | if (member !== undefined) return member; | |
| 71 | } | |
| 59 | 72 | const security = reviewers.filter(watchesSecurity); |
| 60 | 73 | const experience = reviewers.filter((r) => !watchesSecurity(r)); |
| 61 | 74 | const preferred = touchesSecurity(paths) ? security : experience; |
| @@ -326,6 +339,7 @@ export async function sendToReview( | ||
| 326 | 339 | const reviewer = pickReviewer( |
| 327 | 340 | others.length > 0 ? others : reviewers, |
| 328 | 341 | diff.files.map((f) => f.path), |
| 342 | getTeam(app.db, "design")?.members, | |
| 329 | 343 | ); |
| 330 | 344 | if (reviewer === undefined) return "wait"; |
| 331 | 345 | if (queueAgentRun(app, repo, issue, reviewer, "review") === undefined) |
src/crew/runner.ts +10 −3
| @@ -77,6 +77,7 @@ import { bar } from "../cloud/gate.ts"; | ||
| 77 | 77 | import { declarationsOf } from "../cloud/declared.ts"; |
| 78 | 78 | import { accountOfSecret } from "../core/cloud_account.ts"; |
| 79 | 79 | import { alreadyDoneSummary, namedCommits, onBase } from "./already.ts"; |
| 80 | import { touchesDesign } from "./review.ts"; | |
| 80 | 81 | import { BUDGETS, budgetSummary } from "../kernel/budgets.ts"; |
| 81 | 82 | import { createLog, redactValues } from "../kernel/log.ts"; |
| 82 | 83 | import { err, ok, type Result } from "../kernel/result.ts"; |
| @@ -190,16 +191,22 @@ export function steerRun(app: App, runId: string, text: string): boolean { | ||
| 190 | 191 | } |
| 191 | 192 | |
| 192 | 193 | /** What a reviewer reads: the pull's diff against its base, and its pictures. */ |
| 193 | async function reviewInputs( | |
| 194 | export async function reviewInputs( | |
| 194 | 195 | app: App, |
| 195 | 196 | repo: Repo, |
| 196 | 197 | pull: Pull, |
| 197 | 198 | record: AppRecord | undefined, |
| 198 | 199 | bare: string, |
| 199 | ): Promise<{ reviewDiff: string; looks: string[] }> { | |
| 200 | ): Promise<{ reviewDiff: string; looks: string[]; design: boolean }> { | |
| 200 | 201 | const base = (await resolveOid(bare, `refs/heads/${pull.baseBranch}`)) ?? ""; |
| 201 | 202 | let reviewDiff = "(no diff available)"; |
| 203 | let design = false; | |
| 202 | 204 | if (base !== "") { |
| 205 | const names = await runGitCapture( | |
A second git diff just for names. Fold it into the existing diff call to save a process per review. |
||
| 206 | ["diff", "--name-only", `${base}...${pull.headOid}`], | |
| 207 | { cwd: bare }, | |
| 208 | ); | |
| 209 | design = touchesDesign(names.stdout.toString().split("\n").filter((n) => n !== "")); | |
outdated
The design detection from git diff --name-only is untested; cover it (a src/web/ change gives design true, src/core/ only gives false). The empty trailing entry from split is harmless but filter it for clarity. |
||
| 203 | 210 | const out = await runGitCapture( |
| 204 | 211 | ["diff", "--stat", "-p", `${base}...${pull.headOid}`], |
| 205 | 212 | { cwd: bare }, |
| @@ -227,7 +234,7 @@ async function reviewInputs( | ||
| 227 | 234 | .map((l) => |
| 228 | 235 | l.path.startsWith("/") ? l.path : join(app.config.dataDir, l.path), |
| 229 | 236 | ); |
| 230 | return { reviewDiff, looks }; | |
| 237 | return { reviewDiff, looks, design }; | |
| 231 | 238 | } |
| 232 | 239 | |
| 233 | 240 | const REVIEW_DIFF_MAX = 60 * 1024; |
test/review-design.test.ts added +79
| @@ -0,0 +1,79 @@ | ||
| 1 | /** | |
| 2 | * The design half of review: the brief holds a src/web/ change to | |
| 3 | * docs/DESIGN.md, and the runner works out from the diff that it is one. | |
| 4 | */ | |
| 5 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 6 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 7 | import { tmpdir } from "node:os"; | |
| 8 | import { join } from "node:path"; | |
| 9 | import { taskPrompt } from "../src/crew/brief.ts"; | |
| 10 | import { reviewInputs } from "../src/crew/runner.ts"; | |
| 11 | import { runGit } from "../src/git/env.ts"; | |
| 12 | ||
| 13 | const ITEM = "Read docs/DESIGN.md first"; | |
| 14 | ||
| 15 | describe("the design item in the review brief", () => { | |
| 16 | const base = { | |
| 17 | agent: { user: { username: "rev", displayName: "" } }, | |
| 18 | repo: { ownerName: "alice", name: "notes", autonomyDirection: "" }, | |
| 19 | issue: { number: 7, title: "Widen", body: "" }, | |
| 20 | branch: "rev/7", | |
| 21 | forgeUrl: "http://forge.test", | |
| 22 | isApp: true, | |
| 23 | resuming: false, | |
| 24 | reason: "review" as const, | |
| 25 | comments: [], | |
| 26 | reviews: [], | |
| 27 | reviewDiff: "", | |
| 28 | }; | |
| 29 | const prompt = (extra: object) => | |
| 30 | taskPrompt({ ...base, ...extra } as unknown as Parameters<typeof taskPrompt>[0]); | |
| 31 | ||
| 32 | test("is there when the change touches src/web/, and only then", () => { | |
| 33 | expect(prompt({ design: true })).toContain(ITEM); | |
| 34 | expect(prompt({ design: false })).not.toContain(ITEM); | |
| 35 | expect(prompt({})).not.toContain(ITEM); | |
| 36 | }); | |
| 37 | }); | |
| 38 | ||
| 39 | describe("reviewInputs reads design from the diff", () => { | |
| 40 | let dir = ""; | |
| 41 | beforeAll(async () => { | |
| 42 | dir = await mkdtemp(join(tmpdir(), "forge-design-")); | |
| 43 | const git = (...args: string[]) => runGit(args, { cwd: dir }); | |
| 44 | await git("init", "-q", "-b", "main"); | |
| 45 | await git("config", "user.email", "[email protected]"); | |
| 46 | await git("config", "user.name", "t"); | |
| 47 | await Bun.write(join(dir, "README.md"), "x\n"); | |
| 48 | await git("add", "."); | |
| 49 | await git("commit", "-qm", "base"); | |
| 50 | for (const [branch, file] of [ | |
| 51 | ["page", "src/web/pages/x.ts"], | |
| 52 | ["core", "src/core/x.ts"], | |
| 53 | ] as const) { | |
| 54 | await git("checkout", "-q", "-b", branch, "main"); | |
| 55 | await Bun.write(join(dir, file), "export {};\n"); | |
| 56 | await git("add", "."); | |
| 57 | await git("commit", "-qm", branch); | |
| 58 | } | |
| 59 | }); | |
| 60 | afterAll(() => rm(dir, { recursive: true, force: true })); | |
| 61 | ||
| 62 | const inputs = async (branch: string) => { | |
| 63 | const head = ( | |
| 64 | await Bun.$`git -C ${dir} rev-parse ${branch}`.text() | |
| 65 | ).trim(); | |
| 66 | return reviewInputs( | |
| 67 | {} as never, | |
| 68 | {} as never, | |
| 69 | { baseBranch: "main", headOid: head } as never, | |
| 70 | undefined, | |
| 71 | dir, | |
| 72 | ); | |
| 73 | }; | |
| 74 | ||
| 75 | test("a src/web/ change is design, a src/core/ one is not", async () => { | |
| 76 | expect((await inputs("page")).design).toBe(true); | |
| 77 | expect((await inputs("core")).design).toBe(false); | |
| 78 | }); | |
| 79 | }); |
test/review-route.test.ts +27
| @@ -27,6 +27,7 @@ import { | ||
| 27 | 27 | reviewStage, |
| 28 | 28 | SWEEP_SENDS, |
| 29 | 29 | sweepReviews, |
| 30 | touchesDesign, | |
| 30 | 31 | touchesSecurity, |
| 31 | 32 | watchesSecurity, |
| 32 | 33 | } from "../src/crew/review.ts"; |
| @@ -264,3 +265,29 @@ describe("every green pull an agent wrote is sent to review", () => { | ||
| 264 | 265 | expect(reviewer(pulls[59]!)).toEqual([]); |
| 265 | 266 | }, 120_000); |
| 266 | 267 | }); |
| 268 | ||
| 269 | describe("the design team reviews a change to a page", () => { | |
| 270 | const designer = { | |
| 271 | user: { id: "d1", username: "designer" }, | |
| 272 | brief: `I am Designer. I review the look.\n\n${SHARED}`, | |
| 273 | } as unknown as Agent; | |
| 274 | const team = ["d1"]; | |
| 275 | ||
| 276 | test("src/web/ is design's; src/core/ is not", () => { | |
| 277 | expect(touchesDesign(["src/web/pages/x.ts"])).toBe(true); | |
| 278 | expect(touchesDesign(["src/web/ui/button.ts"])).toBe(true); | |
| 279 | expect(touchesDesign(["src/core/x.ts", "docs/DESIGN.md"])).toBe(false); | |
| 280 | }); | |
| 281 | ||
| 282 | test("a pull changing src/web/pages/x.ts goes to a design-team member; src/core/ only does not", () => { | |
| 283 | const crew = [{ user: { id: "j1" }, brief: juniper.brief } as unknown as Agent, designer]; | |
| 284 | expect(pickReviewer(crew, ["src/web/pages/x.ts"], team)).toBe(designer); | |
| 285 | expect(pickReviewer(crew, ["src/core/x.ts"], team)).toBe(crew[0]); | |
| 286 | }); | |
| 287 | ||
| 288 | test("security still outranks design, and a crew with no designer keeps its reviewer", () => { | |
| 289 | const crew = [sentinel, designer]; | |
| 290 | expect(pickReviewer(crew, ["src/web/auth.ts"], team)).toBe(sentinel); | |
| 291 | expect(pickReviewer([juniper], ["src/web/pages/x.ts"], team)).toBe(juniper); | |
| 292 | }); | |
| 293 | }); |
new above
offline
This checklist item is the point of the change and nothing fails if it is removed. Add a test in the brief tests asserting it appears only when design is true.