trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRunsg rEventsFlows

The crew's reviewer checks a pull that touches the UI against docs/DESIGN.md's principles, and the crew brief points to them #9611

Merged masoncrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/9589-the-crew-s-reviewer-checks-a-pull-that-t → main 1 commit fork

  • Merged. mason/9589-the-crew-s-reviewer-checks-a-pull-that-t was merged into main as c0f275c.

Review

Sign in to review this change.
5 files changed +146 −1
.dockerignore +1
@@ -5,5 +5,6 @@ data/
55 dist/
66 docs/
77 !docs/PRINCIPLES.md
8 !docs/DESIGN.md
89 node_modules/
910 test/
Dockerfile +2
@@ -19,6 +19,8 @@ COPY package.json tsconfig.json ./
1919 COPY src ./src
2020 # The principles the forge's own agents are briefed with (src/crew/principles.ts).
2121 COPY docs/PRINCIPLES.md ./docs/PRINCIPLES.md
22 # The design principles a src/web/ review is held to (src/crew/design_principles.ts).
23 COPY docs/DESIGN.md ./docs/DESIGN.md
2224 # src/ alone: nothing under it imports scripts/ or test/ (test/src-imports.test.ts),
2325 # and the boot proof below fails the build if that ever stops being so (#2652).
2426
src/crew/brief.ts +18 −1
@@ -16,6 +16,7 @@ import type { Pull } from "../core/pulls.ts";
1616 import { saysFromApp } from "../actions/app_notes.ts";
1717 import { OUTSIDE_LABEL, OUTSIDE_TITLE, saysOutside } from "../core/reports.ts";
1818 import type { Repo } from "../core/repos.ts";
19 import { DESIGN_POINTER, DESIGN_PRINCIPLES } from "./design_principles.ts";
1920 import { READER } from "./direct.ts";
2021 import { hotFiles, hotWords } from "./hot_files.ts";
2122 import { newsSince } from "./news.ts";
@@ -184,7 +185,9 @@ export function systemPrompt(input: BriefInput): string {
184185 "- Keep state under `FORGE_APP_DATA`. Never write elsewhere at runtime.",
185186 "- The app's terminal (`tty` in forge.json) is reached with the exact command in `FORGE_APP_SSH` (for example `ssh -p 2222 notes@localhost`: the app is the login name, the forge is the host). When a page or the terminal tells people how to connect, print that variable verbatim and never invent a form like `[email protected]`; when it is unset, say the terminal is off.",
186187 "- Run the tests (`bun test`, or whatever `forge.json` names) before you push: the deploy runs them, and a failing test fails the preview. When a test fails, fix the code; change the test only when the test was wrong, and say which in the pull.",
187 "- Touching src/web/: run `bun scripts/design-lint.ts` before you push; it names the file:line and what to use instead. A new component class is named in your own docs/design/areas/<area>.md, never in docs/DESIGN.md, which every pull would otherwise edit.",
188 "- Touching src/web/: " +
189 (isForge(repo) && DESIGN_PRINCIPLES.length > 0 ? `review holds it to ${DESIGN_POINTER}; ` : "") +
190 "run `bun scripts/design-lint.ts` before you push; it names what to fix. A new component class goes in your docs/design/areas/<area>.md, never docs/DESIGN.md.",
188191 "- The bar is the forge's own: small modules named for one concept, every input validated at the boundary and every output escaped, the security floor in " +
189192 (isForge(repo) ? "`src/http/respond.ts` (`securityFloor`)" : "`src/server.ts`") +
190193 " kept on every response, no dependency without a reason in the pull, and a page that answers in milliseconds. `AGENTS.md` in the repository says how this app is shaped; keep its shape.",
@@ -360,6 +363,19 @@ const MCP_SIZE_RULE =
360363 const PLAN_RULE =
361364 "Keep a short plan on this issue with the `keep_plan` tool (up to 12 items, replaced whole each time): call it once you have read the code and again as items finish, so a person sees where you are without a log.";
362365
366 /**
367 * For the review of a forge pull that touches src/web/: the principles that
368 * open docs/DESIGN.md, and a finding asked for each one the change breaks.
369 * Another repository's docs/DESIGN.md is its own, so it gets none of these.
370 */
371 function designPrincipleLines(repo: { readonly name: string }): string[] {
372 if (!isForge(repo) || DESIGN_PRINCIPLES.length === 0) return [];
373 return [
374 ` Hold it to ${DESIGN_POINTER}, each one in turn, and for every principle the change breaks leave one finding — a line comment where it breaks, naming the principle by its number:`,
375 ...DESIGN_PRINCIPLES.map((p) => ` ${p}`),
376 ];
377 }
378
363379 /** The forge's own repository: its briefs name paths in the forge's tree, not an app scaffold's. */
364380 function isForge(repo: { readonly name: string }): boolean {
365381 return repo.name === "forge";
@@ -490,6 +506,7 @@ function taskBody(input: TaskInput): string {
490506 ...(input.design === true
491507 ? [
492508 "- 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.",
509 ...designPrincipleLines(input.repo),
493510 ]
494511 : []),
495512 "- True to the direction: it does what the issue asked and nothing the direction did not, and the preview shows it working.",
src/crew/design_principles.ts added +42
@@ -0,0 +1,42 @@
1 /**
2 * The principles that open docs/DESIGN.md, as the crew's briefs carry them:
3 * the numbered list under "The principles, in short.", one line each, read
4 * once at load. A reviewer of a `src/web/` pull is given them and asked for a
5 * finding per principle broken; the builder brief points to the same lines.
6 * The image ships `docs/DESIGN.md` beside `src/` (Dockerfile); a missing file
7 * reads as no principles rather than a boot failure.
8 */
9
10 import { readFileSync } from "node:fs";
11 import { join } from "node:path";
12
13 export const DESIGN_PATH = "docs/DESIGN.md";
14
15 /** How a brief names the lines, so the builder and the reviewer read the same ones. */
16 export const DESIGN_POINTER = "the principles that open docs/DESIGN.md";
17
18 /** The numbered lines of the first list after "The principles, in short.", in order. */
19 export function designPrinciples(md: string): string[] {
20 const lines = md.split("\n");
21 const start = lines.findIndex((l) => l.startsWith("**The principles, in short.**"));
22 if (start < 0) return [];
23 const out: string[] = [];
24 for (const line of lines.slice(start + 1)) {
25 if (/^\d+\. /.test(line)) out.push(line);
26 else if (out.length > 0) break;
27 }
28 return out;
29 }
30
31 /** The principles in the file at `path`; none when it is missing or unreadable. */
32 export function loadDesignPrinciples(path: string): string[] {
33 try {
34 return designPrinciples(readFileSync(path, "utf8"));
35 } catch {
36 return [];
37 }
38 }
39
40 export const DESIGN_PRINCIPLES: readonly string[] = loadDesignPrinciples(
41 join(import.meta.dir, "..", "..", DESIGN_PATH),
42 );
test/crew-design-principles.test.ts added +83
@@ -0,0 +1,83 @@
1 /**
2 * A forge pull that touches src/web/ is reviewed against the principles that
3 * open docs/DESIGN.md, a finding per principle broken, and the builder brief
4 * points to the same lines (#9589). A pull that touches no UI is not.
5 */
6 import { describe, expect, test } from "bun:test";
7 import { readFileSync } from "node:fs";
8 import { join } from "node:path";
9 import { systemPrompt, taskPrompt, type BriefInput, type TaskInput } from "../src/crew/brief.ts";
10 import {
11 DESIGN_POINTER,
12 DESIGN_PRINCIPLES,
13 designPrinciples,
14 loadDesignPrinciples,
15 } from "../src/crew/design_principles.ts";
16
17 const ROOT = join(import.meta.dir, "..");
18 const md = readFileSync(join(ROOT, "docs", "DESIGN.md"), "utf8");
19
20 const base = (name: string) => ({
21 agent: { user: { username: "bot", displayName: "" }, brief: "" },
22 repo: { ownerName: "trevato", name, defaultBranch: "main", autonomyDirection: "" },
23 issue: { number: 7, title: "Widen", body: "", authorName: "trevato" },
24 branch: "bot/7",
25 forgeUrl: "http://forge.test",
26 isApp: true,
27 comments: [],
28 reviews: [],
29 reviewDiff: "",
30 });
31
32 const review = (design: boolean, name = "forge"): string =>
33 taskPrompt({ ...base(name), reason: "review", design } as unknown as TaskInput);
34 const builder = (name = "forge"): string => systemPrompt(base(name) as unknown as BriefInput);
35
36 describe("the principles that open docs/DESIGN.md", () => {
37 test("are read from the file, numbered, in order", () => {
38 expect(DESIGN_PRINCIPLES.length).toBeGreaterThan(0);
39 DESIGN_PRINCIPLES.forEach((p, i) => {
40 expect(p.startsWith(`${i + 1}. `)).toBe(true);
41 expect(md).toContain(p);
42 });
43 });
44
45 test("are the list under the heading and nothing after it", () => {
46 expect(designPrinciples("# T\n\n**The principles, in short.** x\n\n1. One.\n2. Two.\n\n- not this\n3. Nor this.")).toEqual([
47 "1. One.",
48 "2. Two.",
49 ]);
50 expect(designPrinciples("1. No heading.")).toEqual([]);
51 expect(loadDesignPrinciples(join(ROOT, "no-such-file.md"))).toEqual([]);
52 });
53 });
54
55 describe("a pull that touches src/web/", () => {
56 test("the review brief carries every principle and asks a finding per one broken", () => {
57 const out = review(true);
58 for (const p of DESIGN_PRINCIPLES) expect(out).toContain(p);
59 expect(out).toContain(DESIGN_POINTER);
60 expect(out).toContain("for every principle the change breaks leave one finding");
61 });
62
63 test("the builder brief points to the same lines", () => {
64 const out = builder();
65 expect(out).toContain(DESIGN_POINTER);
66 expect(md).toContain("**The principles, in short.**");
67 });
68 });
69
70 describe("a pull that touches no UI", () => {
71 test("the review brief carries none of them", () => {
72 const out = review(false);
73 for (const p of DESIGN_PRINCIPLES) expect(out).not.toContain(p);
74 expect(out).not.toContain(DESIGN_POINTER);
75 });
76 });
77
78 describe("another repository", () => {
79 test("keeps its own docs/DESIGN.md: neither brief carries the forge's principles", () => {
80 expect(review(true, "notes")).not.toContain(DESIGN_POINTER);
81 expect(builder("notes")).not.toContain(DESIGN_POINTER);
82 });
83 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs