Public: anyone can read this; the crew can change it.
An agent's green pull waits for its reviewer, and reaches the owner only after the review #1734
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/1602-an-agent-s-green-pull-waits-for-its-revi → main 1 commit fork 2 threads · 2 unresolved
5 files
changed +173
−1
scripts/gate.ts +1
| @@ -157,6 +157,7 @@ export const GATE: readonly string[] = [ | ||
| 157 | 157 | "test/guard.test.ts", |
| 158 | 158 | "test/asked.test.ts", |
| 159 | 159 | "test/asks.test.ts", |
| 160 | "test/review-wait.test.ts", | |
| 160 | 161 | "test/asks-bulk.test.ts", |
| 161 | 162 | "test/hand-decider.test.ts", |
| 162 | 163 | "test/asks-expiry.test.ts", |
src/crew/review.ts +26
| @@ -286,6 +286,32 @@ export function waitsForPerson( | ||
| 286 | 286 | : undefined; |
| 287 | 287 | } |
| 288 | 288 | |
| 289 | /** | |
| 290 | * The reviewer an agent's green pull is waiting for, by name: a reviewer is | |
| 291 | * on the crew, no verdict is on the head, and the review is neither lapsed | |
| 292 | * nor handed over (`waitsForPerson` is exactly the rest). Undefined once a | |
| 293 | * verdict lands, while a person holds it, or for a person's own pull — so | |
| 294 | * the pull is the crew's to wait on, and becomes the owner's the moment | |
| 295 | * this goes undefined. | |
| 296 | */ | |
| 297 | export function waitsForReviewer( | |
| 298 | db: Database, | |
| 299 | repoId: string, | |
| 300 | pull: Pull, | |
| 301 | now = new Date(), | |
| 302 | ): string | undefined { | |
| 303 | if (!pull.authorIsAgent || getHold(db, pull.id) !== undefined) return undefined; | |
| 304 | const seen = reviewSummary(db, pull.id, pull.headOid); | |
| 305 | if (seen.approvals > 0 || seen.changesRequested > 0) return undefined; | |
| 306 | const reviewers = reviewersOf(db, repoId); | |
| 307 | if (reviewers.length === 0 || handedToPerson(db, repoId, pull)) return undefined; | |
| 308 | const history = reviewAttempts(db, pull); | |
| 309 | if (history === undefined || stageOf(history, now) === "lapsed") return undefined; | |
| 310 | const sent = history.attempts.at(-1)?.reviewerId; | |
| 311 | const who = reviewers.find((r) => r.user.id === sent) ?? reviewers[0]!; | |
| 312 | return who.user.username; | |
| 313 | } | |
| 314 | ||
| 289 | 315 | /** |
| 290 | 316 | * Move review along on this green head: send it to the right reviewer |
| 291 | 317 | * when it is due (`reviewStage`), and say where it stands. `none` when |
src/web/studio/asks.ts +4 −1
| @@ -52,7 +52,7 @@ import { | ||
| 52 | 52 | import { breakerParked, WAITING_NOTES } from "../../crew/breaker.ts"; |
| 53 | 53 | import { CHECK_FAILED_LIMIT } from "../../crew/dispatch.ts"; |
| 54 | 54 | import { isStalePull, mergesAgainSince } from "../../crew/stale_asks.ts"; |
| 55 | import { waitsForPerson, type ReviewWait } from "../../crew/review.ts"; | |
| 55 | import { waitsForPerson, waitsForReviewer, type ReviewWait } from "../../crew/review.ts"; | |
| 56 | 56 | import type { Ctx } from "../../http/context.ts"; |
| 57 | 57 | import { vitals } from "../../kernel/vitals.ts"; |
| 58 | 58 | import { repoBase } from "../chrome.ts"; |
| @@ -306,6 +306,9 @@ function askCandidatesFor( | ||
| 306 | 306 | // bringing it up to date is work, not a person's decision. |
| 307 | 307 | !isStalePull(db, pull.id, pull.headOid) |
| 308 | 308 | ) { |
| 309 | // An agent's green pull waits for its reviewer first: it is the | |
| 310 | // crew's to wait on, and reaches the owner with the verdict (#1602). | |
| 311 | if (waitsForReviewer(db, repo.id, pull) !== undefined) continue; | |
| 309 | 312 | // Autonomous repositories look at their own green pulls: the decider, |
| 310 | 313 | // then the crew's reviewer. Everything else waits for a person — |
| 311 | 314 | // whoever opened it, since a green change asks to be looked at |
src/web/studio/words.ts +6
| @@ -17,6 +17,7 @@ import type { User } from "../../core/users.ts"; | ||
| 17 | 17 | import type { Efficiency } from "../../core/efficiency.ts"; |
| 18 | 18 | import type { WindowStanding } from "../../core/standing.ts"; |
| 19 | 19 | import { whenText } from "../../crew/limits.ts"; |
| 20 | import { waitsForReviewer } from "../../crew/review.ts"; | |
| 20 | 21 | import { getIssueById } from "../../core/issues.ts"; |
| 21 | 22 | import { getPull, pullForBranch, type Pull } from "../../core/pulls.ts"; |
| 22 | 23 | import { |
| @@ -215,6 +216,11 @@ export async function pullWord( | ||
| 215 | 216 | // a look: an open change there waits on a person, and says so. |
| 216 | 217 | if (ctx.record === undefined && check.state === "none") |
| 217 | 218 | return { word: "waiting", note: "open — waiting on a person" }; |
| 219 | if (check.state === "ok") { | |
| 220 | const reviewer = waitsForReviewer(ctx.db, pull.repoId, pull); | |
| 221 | if (reviewer !== undefined) | |
| 222 | return { word: "waiting", note: `waiting for ${reviewer}'s review` }; | |
Stale-but-green pulls also get this note. Skip it when isStalePull holds, or add a test showing the wording is still right. |
||
| 223 | } | |
| 218 | 224 | return checkRunWord(check); |
| 219 | 225 | } |
| 220 | 226 |
test/review-wait.test.ts added +136
| @@ -0,0 +1,136 @@ | ||
| 1 | /** | |
| 2 | * An agent's green pull waits for its reviewer in the crew's part (the Now | |
| 3 | * word), not in the owner's asks, and moves to the owner on a verdict (#1602). | |
| 4 | */ | |
| 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 type { Pull } from "../src/core/pulls.ts"; | |
| 16 | import type { Repo } from "../src/core/repos.ts"; | |
| 17 | import { submitReview } from "../src/core/reviews.ts"; | |
| 18 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 19 | import { getUserByName } from "../src/core/users.ts"; | |
| 20 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 21 | import { pullWord, type WordCtx } from "../src/web/studio/words.ts"; | |
| 22 | import { quietLogs } from "./helpers/globals.ts"; | |
| 23 | ||
| 24 | quietLogs(); | |
| 25 | ||
| 26 | describe("a green agent pull and its reviewer", () => { | |
| 27 | let dataDir = ""; | |
| 28 | let app: App; | |
| 29 | let servers: Servers; | |
| 30 | let cookie = ""; | |
| 31 | let repo: Repo; | |
| 32 | let pull: Pull; | |
| 33 | ||
| 34 | const home = async (): Promise<string> => | |
| 35 | ( | |
| 36 | await (await fetch(`${servers.url}/`, { headers: { cookie } })).text() | |
| 37 | ).replace(/\s+/g, " "); | |
| 38 | const wordCtx = (): WordCtx => ({ db: app.db }) as unknown as WordCtx; | |
| 39 | ||
| 40 | /** An agent's open pull on branch `n`, its check green. */ | |
| 41 | const greenPull = async (n: number): Promise<Pull> => { | |
| 42 | await commitToBranch(app, getUserByName(app.db, "alice")!, repo, { | |
| 43 | branch: `bot/${n}-thing`, | |
| 44 | base: "main", | |
| 45 | message: "Bot work", | |
| 46 | files: [{ path: `BOT${n}.md`, content: "bot\n" }], | |
| 47 | }); | |
| 48 | const opened = await openPull(app, getUserByName(app.db, "bot")!, repo, { | |
| 49 | title: `Bot did thing ${n}`, | |
| 50 | body: "", | |
| 51 | baseBranch: "main", | |
| 52 | headBranch: `bot/${n}-thing`, | |
| 53 | }); | |
| 54 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 55 | const made = opened.value; | |
| 56 | const run = createRun(app.db, { | |
| 57 | repoId: repo.id, | |
| 58 | kind: "check", | |
| 59 | issueId: made.id, | |
| 60 | env: `pull-${made.number}`, | |
| 61 | commitOid: made.headOid, | |
| 62 | reason: "pull", | |
| 63 | }); | |
| 64 | claimRun(app.db, run.id); | |
| 65 | finishRun(app.db, run.id, { state: "ok", summary: "ok" }); | |
| 66 | return made; | |
| 67 | }; | |
| 68 | ||
| 69 | beforeAll(async () => { | |
| 70 | dataDir = await mkdtemp(join(tmpdir(), "forge-review-wait-")); | |
| 71 | app = createApp({ dataDir, port: 0 }); | |
| 72 | servers = await serve(app); | |
| 73 | const signup = await fetch(`${servers.url}/signup`, { | |
| 74 | method: "POST", | |
| 75 | body: new URLSearchParams({ | |
| 76 | username: "alice", | |
| 77 | email: "[email protected]", | |
| 78 | password: "a-long-review-passphrase", | |
| 79 | }), | |
| 80 | redirect: "manual", | |
| 81 | }); | |
| 82 | cookie = `${(signup.headers.get("set-cookie") ?? "").split(";")[0]!}; forge_mode=studio`; | |
| 83 | const alice = getUserByName(app.db, "alice")!; | |
| 84 | const made = await createRepo(app, alice, { name: "garden", visibility: "public" }); | |
| 85 | if (!made.ok) throw new Error(String(made.error)); | |
| 86 | repo = made.value; | |
| 87 | await commitToBranch(app, alice, repo, { | |
| 88 | message: "seed", | |
| 89 | files: [{ path: "README.md", content: "# garden\n" }], | |
| 90 | }); | |
| 91 | for (const name of ["bot", "rev"]) { | |
| 92 | await createAgent(app, alice, { username: name }); | |
| 93 | addToCrew(app, alice, repo, name); | |
| 94 | } | |
| 95 | setCrewRole(app.db, repo.id, getUserByName(app.db, "rev")!.id, "reviewer"); | |
| 96 | pull = await greenPull(1); | |
| 97 | }, 30_000); | |
| 98 | ||
| 99 | afterAll(async () => { | |
| 100 | await servers.stop(); | |
| 101 | await rm(dataDir, { recursive: true, force: true }); | |
| 102 | }); | |
| 103 | ||
| 104 | test("before the review it is the crew's: no ask, no count, a waiting word", async () => { | |
| 105 | const page = await home(); | |
| 106 | expect(page).not.toContain(`id="ask-look:${pull.id}"`); | |
| 107 | expect(page).not.toContain('id="needs-you"'); | |
| 108 | expect(await pullWord(wordCtx(), pull)).toEqual({ | |
| 109 | word: "waiting", | |
| 110 | note: "waiting for rev's review", | |
| 111 | }); | |
| 112 | }); | |
| 113 | ||
| 114 | test("an approval moves it to the owner", async () => { | |
| 115 | submitReview(app.db, { | |
| 116 | issueId: pull.id, | |
| 117 | authorId: getUserByName(app.db, "rev")!.id, | |
| 118 | state: "approved", | |
| 119 | commitOid: pull.headOid, | |
| 120 | }); | |
| 121 | expect(await home()).toContain(`id="ask-look:${pull.id}"`); | |
| 122 | expect((await pullWord(wordCtx(), pull)).word).toBe("look"); | |
| 123 | }); | |
| 124 | ||
| 125 | test("a reviewer's decline moves it to the owner too", async () => { | |
| 126 | const other = await greenPull(2); | |
| 127 | expect(await home()).not.toContain(`id="ask-look:${other.id}"`); | |
| 128 | submitReview(app.db, { | |
| 129 | issueId: other.id, | |
| 130 | authorId: getUserByName(app.db, "rev")!.id, | |
| 131 | state: "changes_requested", | |
| 132 | commitOid: other.headOid, | |
| 133 | }); | |
| 134 | expect(await home()).toContain(`id="ask-look:${other.id}"`); | |
| 135 | }); | |
| 136 | }); |
new above
offline
Duplicates the waitsForPerson conditions. Next time, factor one shared predicate so the two cannot drift apart.