trevato/forge

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

More
Forks1g fMapg aRuns11g rEventsFlows

No pull has a review gate: nothing ever queues the reviewer pass, and an autonomous repo lands any green pull, a person's included, unseen #429

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/414-no-pull-has-a-review-gate-nothing-ever-q → main 1 commit fork

  • Merged. scout/414-no-pull-has-a-review-gate-nothing-ever-q was merged into main as a3bc825.

Review

Sign in to review this change.
3 files changed +219 −15
src/crew/autonomy.ts +36 −4
@@ -76,6 +76,7 @@ import {
7676 } from "./dispatch.ts";
7777 import { getUserById, type User } from "../core/users.ts";
7878 import { grantedAccounts } from "../core/subscriptions.ts";
79 import { reviewSummary } from "../core/reviews.ts";
7980 import { standing } from "../core/standing.ts";
8081 import { pace, type Pace } from "./pacing.ts";
8182 import { BREAKER_NOTE, breakerTripped } from "./breaker.ts";
@@ -842,6 +843,36 @@ export function bringUpToDate(
842843 return agent.user.username;
843844 }
844845
846 /** The heads already sent to the reviewer, by pull: one pass per head. */
847 const sentToReview = new Map<string, string>();
848
849 /**
850 * Whether the review gate lets this green head land. True with no reviewer
851 * on the crew, or with an approval on this head and no change requested.
852 * Otherwise the reviewer is queued once for the head and the pull waits.
853 */
854 async function reviewed(app: App, repo: Repo, pull: Pull): Promise<boolean> {
855 const reviewer = listCrewRoles(app.db, repo.id).find(
856 (c) => c.role === "reviewer",
857 )?.agent;
858 if (reviewer === undefined) return true;
859 const seen = reviewSummary(app.db, pull.id, pull.headOid);
860 if (seen.changesRequested > 0) return false;
861 if (seen.approvals > 0) return true;
862 if (sentToReview.get(pull.id) === pull.headOid) return false;
863 if (activeRunFor(app.db, "agent", pull.id) !== undefined) return false;
864 const issue = getIssueById(app.db, pull.id);
865 if (!issue) return false;
866 if (queueAgentRun(app, repo, issue, reviewer, "review") !== undefined) {
867 sentToReview.set(pull.id, pull.headOid);
868 log.info("green; queued for review", {
869 repo: `${repo.ownerName}/${repo.name}`,
870 pull: pull.number,
871 });
872 }
873 return false;
874 }
875
845876 /**
846877 * Ship on green. An open pull whose head has a passing check and a passing
847878 * deploy, and nothing failed, lands as the person who turned autonomy on —
@@ -921,10 +952,11 @@ export async function landIfGreen(
921952 }
922953 return false;
923954 }
924 // Ship, or look first: the decider reads the pull against the direction.
925 // A rule ships everything green; the decider is what makes "autonomous"
926 // mean judgement rather than a conveyor belt — and the look is the crew's
927 // own reviewer, not a person. Without a reviewer, it lands.
955 // One gate for every pull, whoever wrote it: when the crew has a
956 // reviewer, a green head is queued for review and waits for an approval
957 // on that very head (a person's counts the same). A push is a new head,
958 // so it starts over. Without a reviewer the pull lands as it always has.
959 if (!(await reviewed(app, repo, pull))) return false;
928960 // Green and clear of the wall lands. An autonomous repository does not
929961 // wait on a person; the ledger keeps the landing as the rule it is, once
930962 // per head, so the week's numbers still add up.
src/crew/dispatch.ts +10 −11
@@ -300,17 +300,16 @@ function route(app: App, event: ForgeEvent): void {
300300 event.kind === "pull.reviewed" &&
301301 event.payload["state"] === "approved"
302302 ) {
303 // The reviewer approved: the pull lands now, not at the next sweep.
304 if (reviewerVerdict) {
305 const pull = getPull(app.db, repo.id, number);
306 if (pull)
307 landIfGreen(app, repo, pull).catch((error) =>
308 log.warn("could not land after the review", {
309 pull: number,
310 error: String(error),
311 }),
312 );
313 }
303 // An approval, a person's or the reviewer's: the pull lands now,
304 // not at the next sweep.
305 const pull = getPull(app.db, repo.id, number);
306 if (pull)
307 landIfGreen(app, repo, pull).catch((error) =>
308 log.warn("could not land after the review", {
309 pull: number,
310 error: String(error),
311 }),
312 );
314313 return;
315314 }
316315 const agent = agentFor(app, issue);
test/review-gate.test.ts added +173
@@ -0,0 +1,173 @@
1 /**
2 * The review gate: with a reviewer on the crew, a green pull — a person's or
3 * an agent's — is queued for review and lands only on an approval of its
4 * current head. Without a reviewer it lands as before.
5 */
6 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
7 import { mkdtemp, rm } from "node:fs/promises";
8 import { tmpdir } from "node:os";
9 import { join } from "node:path";
10 import { createApp, type App } from "../src/app.ts";
11 import { addToCrew, createAgent } from "../src/actions/crew.ts";
12 import { openPull } from "../src/actions/pulls.ts";
13 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
14 import { setCrewRole } from "../src/core/agents.ts";
15 import { getPull, type Pull } from "../src/core/pulls.ts";
16 import { submitReview } from "../src/core/reviews.ts";
17 import { getRepoById, type Repo } from "../src/core/repos.ts";
18 import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts";
19 import { createUser, getUserByName, type User } from "../src/core/users.ts";
20 import { landIfGreen } from "../src/crew/autonomy.ts";
21
22 describe("the review gate", () => {
23 let dataDir: string;
24 let app: App;
25 let alice: User;
26 let mason: User;
27 let quill: User;
28 let repo: Repo;
29 const fresh = () => getRepoById(app.db, repo.id)!;
30 const reviewRuns = (pull: Pull) =>
31 listRuns(app.db, repo.id, { issueId: pull.id }).filter(
32 (r) => r.reason === "review",
33 );
34
35 beforeAll(async () => {
36 dataDir = await mkdtemp(join(tmpdir(), "forge-review-gate-"));
37 app = createApp({ dataDir, port: 0 });
38 const a = await createUser(app.db, {
39 username: "alice",
40 email: "[email protected]",
41 password: "a-good-passphrase",
42 adminIfFirst: true,
43 });
44 if (!a.ok) throw new Error(a.error);
45 alice = a.value;
46 const made = await createRepo(app, alice, {
47 name: "engine",
48 visibility: "private",
49 });
50 if (!made.ok) throw new Error(String(made.error));
51 repo = made.value;
52 const seeded = await commitToBranch(app, alice, repo, {
53 message: "Start",
54 files: [{ path: "README.md", content: "# engine\n" }],
55 });
56 if (!seeded.ok) throw new Error(JSON.stringify(seeded.error));
57 for (const name of ["mason", "quill"]) {
58 const agent = await createAgent(app, alice, { username: name });
59 if (!agent.ok) throw new Error(String(agent.error));
60 expect(addToCrew(app, alice, repo, name).ok).toBe(true);
61 }
62 mason = getUserByName(app.db, "mason")!;
63 quill = getUserByName(app.db, "quill")!;
64 app.db
65 .query(
66 "UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1",
67 )
68 .run(repo.id, alice.id);
69 });
70
71 afterAll(async () => {
72 await app.close?.();
73 await rm(dataDir, { recursive: true, force: true });
74 });
75
76 async function greenPull(author: User, name: string): Promise<Pull> {
77 const branch = await commitToBranch(app, alice, repo, {
78 branch: `feature/${name}`,
79 base: "main",
80 message: name,
81 files: [{ path: `${name}.ts`, content: `export const n = "${name}";\n` }],
82 });
83 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
84 const opened = await openPull(app, alice, repo, {
85 title: name,
86 baseBranch: "main",
87 headBranch: `feature/${name}`,
88 });
89 if (!opened.ok) throw new Error(String(opened.error));
90 // Agents push through forks in production; the gate only reads the author.
91 app.db
92 .query("UPDATE issue SET author_id = ?2 WHERE id = ?1")
93 .run(opened.value.id, author.id);
94 return passCheck(opened.value);
95 }
96
97 function passCheck(pull: Pull): Pull {
98 const check = createRun(app.db, {
99 repoId: repo.id,
100 kind: "check",
101 issueId: pull.id,
102 env: `pull-${pull.number}`,
103 commitOid: pull.headOid,
104 reason: "pull",
105 });
106 claimRun(app.db, check.id);
107 finishRun(app.db, check.id, { state: "ok", summary: "check passed" });
108 return pull;
109 }
110
111 const approve = (pull: Pull, by: User) =>
112 submitReview(app.db, {
113 issueId: pull.id,
114 authorId: by.id,
115 state: "approved",
116 commitOid: pull.headOid,
117 });
118
119 test("with no reviewer on the crew a green pull lands as before", async () => {
120 const pull = await greenPull(alice, "plain");
121 expect(await landIfGreen(app, fresh(), pull)).toBe(true);
122 expect(reviewRuns(pull)).toHaveLength(0);
123 });
124
125 for (const [who, author] of [
126 ["a person's", () => alice],
127 ["an agent's", () => mason],
128 ] as const) {
129 test(`${who} green pull is reviewed, and lands only on an approval of its head`, async () => {
130 setCrewRole(app.db, repo.id, quill.id, "reviewer");
131 const name = who.includes("person") ? "by-person" : "by-agent";
132 const pull = await greenPull(author(), name);
133 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
134 expect(reviewRuns(pull)).toHaveLength(1);
135 expect(reviewRuns(pull)[0]!.actorId).toBe(quill.id);
136 // Asked again: still one pass for this head, still not landed.
137 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
138 expect(reviewRuns(pull)).toHaveLength(1);
139
140 // A push resets: an approval of the old head means nothing.
141 approve(pull, quill);
142 const pushed = await commitToBranch(app, alice, repo, {
143 branch: `feature/${name}`,
144 base: "main",
145 message: "more",
146 files: [{ path: `${name}-2.ts`, content: "export const m = 1;\n" }],
147 });
148 if (!pushed.ok) throw new Error(JSON.stringify(pushed.error));
149 const moved = passCheck(getPull(app.db, repo.id, pull.number)!);
150 expect(moved.headOid).not.toBe(pull.headOid);
151 expect(await landIfGreen(app, fresh(), moved)).toBe(false);
152 expect(getPull(app.db, repo.id, pull.number)?.mergedAt).toBeNull();
153
154 // A person's approval of the new head counts the same.
155 approve(moved, alice);
156 expect(await landIfGreen(app, fresh(), moved)).toBe(true);
157 expect(getPull(app.db, repo.id, pull.number)?.mergedAt).not.toBeNull();
158 });
159 }
160
161 test("a change requested on the head holds the pull", async () => {
162 setCrewRole(app.db, repo.id, quill.id, "reviewer");
163 const pull = await greenPull(alice, "contested");
164 submitReview(app.db, {
165 issueId: pull.id,
166 authorId: quill.id,
167 state: "changes_requested",
168 commitOid: pull.headOid,
169 });
170 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
171 expect(getPull(app.db, repo.id, pull.number)?.mergedAt).toBeNull();
172 });
173 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs