trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlowsAPI

An agent's pull cannot land without a review by another identity: one table test over every landing door #12427

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/12403-an-agent-s-pull-cannot-land-without-a-re → main 2 commits fork

  • Merged. scout/12403-an-agent-s-pull-cannot-land-without-a-re was merged into main as 9105665.

Review

Sign in to review this change.
3 files changed +300 −2
src/core/pulls.ts +2 −1
@@ -872,7 +872,8 @@ export function reviewGate(
872872 headOid: string,
873873 baseOid?: string,
874874 ): PullReviewStatus {
875 const summary = reviewSummary(gate.db, pull.id, headOid);
875 // Only another identity's verdict gates: the author approving its own pull is no review (#12403).
876 const summary = reviewSummary(gate.db, pull.id, headOid, pull.authorId);
876877 return {
877878 approval:
878879 baseOid === undefined
src/core/reviews.ts +9 −1
@@ -372,8 +372,16 @@ export function reviewSummary(
372372 db: Database,
373373 issueId: string,
374374 headOid: string,
375 /** The pull's author: their own verdict is not a review of it (#12403), however it was
376 * recorded. */
377 authorId?: string,
375378 ): ReviewSummary {
376 return summarize(listReviews(db, issueId), verdictsOf(agentVerdictRows(db, [issueId])), headOid);
379 const reviews = listReviews(db, issueId);
380 return summarize(
381 authorId === undefined ? reviews : reviews.filter((r) => !(r.originAuthor === null && r.authorId === authorId)),
382 verdictsOf(agentVerdictRows(db, [issueId])),
383 headOid,
384 );
377385 }
378386
379387 /**
test/landing-doors-review.test.ts added +289
@@ -0,0 +1,289 @@
1 /**
2 * An agent's pull cannot land without a review by another identity, at any
3 * door (#12403). Every door ends in one of three landing actions —
4 * `mergePull`, `landSlice` (both behind the review verdict `reviewGate`) and
5 * `landPastGate` (which is `mergePull` past the checks, never past reviews).
6 *
7 * The table below names each door: every source file that calls a landing
8 * action, every contract route that lands and every MCP tool that lands. A
9 * door found in the source, the contract or the tool list and missing from
10 * the table fails by name, so a new door cannot be added unnoticed.
11 *
12 * Each door is then tried, with `requireReview` on, on an agent's pull three
13 * times: with no review, with an approval by the agent itself (recorded
14 * straight into the table: `submitReview` refuses it, and the gate must not
15 * count it however it arrived), and with an approval by a person. Only the
16 * last lands. The one exception is the owner's: with reviews off and no
17 * reviewer seated, a green agent pull lands with no review
18 * (docs/features/reviews-off-lands-on-green.md).
19 *
20 * The API and MCP doors are tried over HTTP. The rest are tried through the
21 * landing action with exactly the options their call site passes, listed in
22 * the row; the scan pins that each still calls it.
23 */
24 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
25 import { readdirSync, readFileSync, statSync } from "node:fs";
26 import { mkdtemp, rm } from "node:fs/promises";
27 import { tmpdir } from "node:os";
28 import { join } from "node:path";
29 import { createApp, type App } from "../src/app.ts";
30 import { mergePull, openPull, submitReview } from "../src/actions/pulls.ts";
31 import { landSlice } from "../src/actions/pull_slices.ts";
32 import { commitToBranch, createRepo, forkRepo } from "../src/actions/repos.ts";
33 import { ROUTES } from "../src/contract/openapi.ts";
34 import { addCrew, createAgentRow, setCrewRole } from "../src/core/agents.ts";
35 import { getPull, type Pull } from "../src/core/pulls.ts";
36 import { getRepo, type Repo } from "../src/core/repos.ts";
37 import { submitReview as recordReview } from "../src/core/reviews.ts";
38 import { createToken } from "../src/core/tokens.ts";
39 import { createUser, type User } from "../src/core/users.ts";
40 import { TOOLS } from "../src/http/mcp/tools.ts";
41 import { serve, type Servers } from "../src/http/server.ts";
42 import { askForReviews } from "./helpers/reviews.ts";
43 import { promote } from "./helpers/members.ts";
44
45 const ACTIONS = ["mergePull", "landSlice", "landPastGate"] as const;
46
47 /** How a door is tried: over HTTP, or through the landing action with its call site's options. */
48 type Way =
49 | { readonly http: "api-merge" | "api-slices" | "mcp-merge" }
50 | { readonly action: "mergePull"; readonly opts?: { pastGate?: true; pastReviews?: true } }
51 | { readonly action: "landSlice" };
52
53 interface Door {
54 readonly name: string;
55 /** `file:action` call sites this door is. */
56 readonly sites: readonly string[];
57 readonly route?: string;
58 readonly tool?: string;
59 readonly way: Way;
60 }
61
62 const DOORS: readonly Door[] = [
63 { name: "API merge", sites: ["src/http/api.ts:mergePull"], route: "post /api/v1/repos/{owner}/{name}/pulls/{number}/merge", way: { http: "api-merge" } },
64 { name: "API slices", sites: ["src/http/api.ts:landSlice"], route: "post /api/v1/repos/{owner}/{name}/pulls/{number}/slices", way: { http: "api-slices" } },
65 { name: "MCP merge", sites: ["src/http/mcp/tools/merge.ts:mergePull"], tool: "merge", way: { http: "mcp-merge" } },
66 { name: "MCP land_past_gate", sites: ["src/http/mcp/tools/land_past_gate.ts:landPastGate"], tool: "land_past_gate", way: { action: "mergePull", opts: { pastGate: true } } },
67 { name: "web Merge button", sites: ["src/web/pages/pull.ts:mergePull"], way: { action: "mergePull" } },
68 { name: "web Land slice", sites: ["src/web/pages/pull.ts:landSlice"], way: { action: "landSlice" } },
69 { name: "web Land past the gate", sites: ["src/web/pages/pull.ts:landPastGate"], way: { action: "mergePull", opts: { pastGate: true } } },
70 { name: "studio Ship", sites: ["src/web/studio/actions.ts:mergePull"], way: { action: "mergePull" } },
71 { name: "studio review act", sites: ["src/web/studio/review_act.ts:mergePull"], way: { action: "mergePull" } },
72 { name: "terminal pull Merge", sites: ["src/tui/screens/pull.ts:mergePull"], way: { action: "mergePull" } },
73 { name: "terminal Land slice", sites: ["src/tui/screens/pull.ts:landSlice"], way: { action: "landSlice" } },
74 { name: "terminal Land past the gate", sites: ["src/tui/screens/pull.ts:landPastGate"], way: { action: "mergePull", opts: { pastGate: true } } },
75 { name: "terminal Land as is", sites: ["src/tui/screens/landasis.ts:mergePull"], way: { action: "mergePull" } },
76 { name: "CLI fleet verbs", sites: ["src/fleet/verbs.ts:mergePull"], way: { action: "mergePull" } },
77 { name: "build", sites: ["src/actions/build.ts:mergePull"], way: { action: "mergePull" } },
78 { name: "engine landing", sites: ["src/crew/autonomy.ts:mergePull"], way: { action: "mergePull", opts: { pastReviews: true } } },
79 { name: "owner bypass", sites: ["src/checks/bypass.ts:mergePull"], way: { action: "mergePull", opts: { pastGate: true } } },
80 { name: "slice at the head", sites: ["src/actions/pull_slices.ts:mergePull"], way: { action: "mergePull" } },
81 { name: "preview seed", sites: ["src/spawn/preview-seed.ts:mergePull"], way: { action: "mergePull", opts: { pastGate: true } } },
82 ];
83
84 const ROOT = join(import.meta.dir, "..");
85
86 function sourceFiles(dir: string): string[] {
87 return readdirSync(join(ROOT, dir)).flatMap((name) => {
88 const rel = `${dir}/${name}`;
89 if (statSync(join(ROOT, rel)).isDirectory()) return sourceFiles(rel);
90 return rel.endsWith(".ts") ? [rel] : [];
91 });
92 }
93
94 /** Every `file:action` in src that calls a landing action, comments and definitions aside. */
95 function callSites(): Set<string> {
96 const found = new Set<string>();
97 const call = new RegExp(`\\b(${ACTIONS.join("|")})\\(`, "g");
98 for (const file of sourceFiles("src")) {
99 for (const line of readFileSync(join(ROOT, file), "utf8").split("\n")) {
100 const code = line.trim();
101 if (code.startsWith("*") || code.startsWith("//") || code.startsWith("/*")) continue;
102 for (const m of code.matchAll(call)) {
103 if (new RegExp(`function\\s+${m[1]}\\(`).test(code)) continue;
104 found.add(`${file}:${m[1]}`);
105 }
106 }
107 }
108 return found;
109 }
110
111 describe("every landing door is in the table", () => {
112 const listed = new Set(DOORS.flatMap((d) => d.sites));
113
114 test("each call of a landing action in src is a door the table names", () => {
115 const sites = callSites();
116 expect([...sites].filter((s) => !listed.has(s)).sort()).toEqual([]);
117 // And no row names a door that is gone.
118 expect([...listed].filter((s) => !sites.has(s)).sort()).toEqual([]);
119 });
120
121 test("each contract route that lands is a door the table names", () => {
122 const routes = new Set(DOORS.flatMap((d) => (d.route ? [d.route] : [])));
123 const landing = ROUTES.filter((r) => r.method !== "get" && /\/(merge|slices|ship|land[\w-]*|bypass)$/.test(r.path)).map(
124 (r) => `${r.method} ${r.path}`,
125 );
126 expect(landing.filter((r) => !routes.has(r)).sort()).toEqual([]);
127 expect([...routes].filter((r) => !landing.includes(r))).toEqual([]);
128 });
129
130 test("each MCP tool that lands is a door the table names", () => {
131 const tools = new Set(DOORS.flatMap((d) => (d.tool ? [d.tool] : [])));
132 const call = new RegExp(`\\b(${ACTIONS.join("|")})\\(`);
133 const landing = TOOLS.map((t) => t.name).filter((name) => {
134 const file = join(ROOT, "src/http/mcp/tools", `${name}.ts`);
135 return call.test(readFileSync(file, "utf8"));
136 });
137 expect(landing.filter((t) => !tools.has(t)).sort()).toEqual([]);
138 expect([...tools].filter((t) => !landing.includes(t))).toEqual([]);
139 });
140 });
141
142 describe("an agent's pull lands only on another identity's review, at every door", () => {
143 let dataDir: string;
144 let app: App;
145 let servers: Servers;
146 let repo: Repo;
147 /** Slices are a fork's: the slice doors try a pull from here. */
148 let fork: Repo;
149 let alice: User;
150 let wright: User;
151 let aliceRw: string;
152 let n = 0;
153
154 /** An agent's pull of two commits; the first is the slice a slice door lands. */
155 const agentPull = async (fromFork = false): Promise<{ pull: Pull; first: string }> => {
156 const branch = `agent-${++n}`;
157 const at = fromFork ? fork : repo;
158 const one = await commitToBranch(app, alice, at, { branch, message: "one", files: [{ path: `${branch}-1.txt`, content: "1\n" }] });
159 if (!one.ok) throw new Error(JSON.stringify(one.error));
160 const two = await commitToBranch(app, alice, at, { branch, message: "two", files: [{ path: `${branch}-2.txt`, content: "2\n" }] });
161 if (!two.ok) throw new Error(JSON.stringify(two.error));
162 const opened = await openPull(app, alice, repo, { title: branch, baseBranch: "main", headBranch: branch, ...(fromFork ? { headRepo: fork } : {}) });
163 if (!opened.ok) throw new Error(String(opened.error));
164 // The agent's pull: a crew agent pushes over git, so the row is written as its,
165 // as that push would.
166 app.db.query("UPDATE issue SET author_id = ?1 WHERE id = ?2").run(wright.id, opened.value.id);
167 return { pull: opened.value, first: one.value.commitOid };
168 };
169
170 const post = (path: string, body: unknown) =>
171 fetch(`${servers.url}${path}`, {
172 method: "POST",
173 headers: { "content-type": "application/json", accept: "application/json, text/event-stream", authorization: `Bearer ${aliceRw}` },
174 body: JSON.stringify(body),
175 });
176
177 /** Try to land through the door as alice; whether it landed, and the refusal when not. */
178 const tryLand = async (door: Door, pull: Pull, first: string): Promise<{ landed: boolean; error?: string }> => {
179 const live = getPull(app.db, repo.id, pull.number)!;
180 const way = door.way;
181 if ("http" in way) {
182 if (way.http === "mcp-merge") {
183 const res = await post("/mcp", {
184 jsonrpc: "2.0",
185 id: n,
186 method: "tools/call",
187 params: { name: "merge", arguments: { repo: "alice/game", number: pull.number, dryRun: false } },
188 });
189 const body = (await res.json()) as { result: { isError?: boolean; content: { text: string }[] } };
190 return body.result.isError === true ? { landed: false, error: body.result.content[0]!.text } : { landed: true };
191 }
192 const path = `/api/v1/repos/alice/game/pulls/${pull.number}/${way.http === "api-merge" ? "merge" : "slices"}`;
193 const res = await post(path, way.http === "api-merge" ? {} : { upTo: first });
194 if (res.ok) return { landed: true };
195 return { landed: false, error: ((await res.json()) as { error: string }).error };
196 }
197 if (way.action === "landSlice") {
198 const landed = await landSlice(app, alice, repo, live, first);
199 return landed.ok ? { landed: true } : { landed: false, error: JSON.stringify(landed.error) };
200 }
201 const landed = await mergePull(app, alice, repo, live, "merge", undefined, way.opts ?? {});
202 return landed.ok ? { landed: true } : { landed: false, error: JSON.stringify(landed.error) };
203 };
204
205 beforeAll(async () => {
206 dataDir = await mkdtemp(join(tmpdir(), "forge-landing-doors-"));
207 app = createApp({ dataDir, port: 0 });
208 servers = await serve(app);
209 const made = await createUser(app.db, {
210 origin: "seed",
211 trusted: true,
212 username: "alice",
213 email: "[email protected]",
214 password: "a-good-passphrase",
215 adminIfFirst: true,
216 });
217 if (!made.ok) throw new Error(made.error);
218 alice = made.value;
219 const agent = await createUser(app.db, {
220 origin: "agent",
221 trusted: true,
222 username: "wright",
223 email: "[email protected]",
224 password: "",
225 agent: true,
226 });
227 if (!agent.ok) throw new Error(agent.error);
228 wright = agent.value;
229 createAgentRow(app.db, { userId: wright.id, brief: "Builds and reviews.", roles: ["builder", "reviewer"] });
230 promote(app.db, alice);
231 aliceRw = createToken(app.db, alice.id, "rw", "rw").token;
232 const created = await createRepo(app, alice, { name: "game", visibility: "public" });
233 if (!created.ok) throw new Error(String(created.error));
234 await commitToBranch(app, alice, created.value, { message: "seed", files: [{ path: "README.md", content: "# game\n" }] });
235 // Seated as the reviewer: its verdict carries a reviewer's weight on others' pulls,
236 // never on its own.
237 addCrew(app.db, created.value.id, wright.id, alice.id, "reviewer");
238 askForReviews(app.db, created.value.id);
239 repo = getRepo(app.db, "alice", "game")!;
240 const forked = await forkRepo(app, alice, repo, "game-fork");
241 if (!forked.ok) throw new Error(String(forked.error));
242 fork = forked.value;
243 }, 30_000);
244
245 afterAll(async () => {
246 servers.stop();
247 app.close();
248 await rm(dataDir, { recursive: true, force: true });
249 });
250
251 for (const door of DOORS) {
252 test(door.name, async () => {
253 const slice = "http" in door.way ? door.way.http === "api-slices" : door.way.action === "landSlice";
254 const { pull, first } = await agentPull(slice);
255
256 const none = await tryLand(door, pull, first);
257 expect(none.landed).toBe(false);
258 expect(none.error).toContain("review_required");
259
260 // The agent's own approval: the action refuses it, and a row that got in anyway
261 // does not count.
262 const head = getPull(app.db, repo.id, pull.number)!;
263 const self = await submitReview(app, wright, repo, head, { state: "approved" });
264 expect(self.ok ? "approved" : self.error).toBe("self_review");
265 expect(recordReview(app.db, { issueId: pull.id, authorId: wright.id, state: "approved", commitOid: head.headOid }).ok).toBe(true);
266 const own = await tryLand(door, pull, first);
267 expect(own.landed).toBe(false);
268 expect(own.error).toContain("review_required");
269
270 const other = await submitReview(app, alice, repo, head, { state: "approved" });
271 expect(other.ok).toBe(true);
272 const landed = await tryLand(door, pull, first);
273 expect(landed).toEqual({ landed: true });
274 });
275 }
276
277 test("the owner's exception: reviews off and no reviewer seated, an agent's pull lands with no review", async () => {
278 askForReviews(app.db, repo.id, false);
279 expect(setCrewRole(app.db, repo.id, wright.id, "builder")).toBe(true);
280 try {
281 const { pull } = await agentPull();
282 const landed = await mergePull(app, alice, getRepo(app.db, "alice", "game")!, getPull(app.db, repo.id, pull.number)!, "merge");
283 expect(landed.ok).toBe(true);
284 } finally {
285 askForReviews(app.db, repo.id);
286 setCrewRole(app.db, repo.id, wright.id, "reviewer");
287 }
288 });
289 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs