Public: anyone can read this; the crew can change it.
The reviewer seat's cap is proven end to end: at 1, a second agent pull waits until the first review ends #8764
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/8734-the-reviewer-seat-s-cap-is-proven-end-to → main 1 commit fork
3 files
changed +91
−6
src/crew/box_pass.ts +10 −4
| @@ -10,10 +10,10 @@ import { crewRole } from "../core/agents.ts"; | ||
| 10 | 10 | import { PASS_LEAD, unticked } from "../core/done_on_main.ts"; |
| 11 | 11 | import { getIssue, getIssueById, type Issue } from "../core/issues.ts"; |
| 12 | 12 | import { closesFromBody, getPull, mergedPullClosing, type Pull } from "../core/pulls.ts"; |
| 13 | import type { Repo } from "../core/repos.ts"; | |
| 13 | import { PILLAR_MAX, type Repo } from "../core/repos.ts"; | |
| 14 | 14 | import type { BoxList } from "./brief.ts"; |
| 15 | 15 | import { queueAgentRun } from "./dispatch.ts"; |
| 16 | import { pickReviewer, reviewersOf, reviewLoad } from "./review.ts"; | |
| 16 | import { pickReviewer, reviewersOf, reviewLoad, reviewsInFlight } from "./review.ts"; | |
| 17 | 17 | |
| 18 | 18 | /** Landed issues one tick sends to review: the rest wait for the next, never unseen. */ |
| 19 | 19 | export const PASS_SENDS = 3; |
| @@ -55,14 +55,20 @@ export function passed(app: App, repo: Repo, issue: Issue, pull: Pull): boolean | ||
| 55 | 55 | /** |
| 56 | 56 | * Send each issue held under `landed` that was never given the pass to the |
| 57 | 57 | * crew's reviewer, once: the review run on the pull that landed is the |
| 58 | * mark, so a tick later finds it sent. Returns how many were sent. | |
| 58 | * mark, so a tick later finds it sent. Returns how many were sent. The | |
| 59 | * reviewer's slots bound it as they bound `sendToReview` (#8734): a paused | |
| 60 | * reviewer is sent nothing, and a full one waits for the next tick. | |
| 59 | 61 | */ |
| 60 | 62 | export function passLanded(app: App, repo: Repo, held: readonly Issue[]): number { |
| 61 | 63 | const reviewers = reviewersOf(app.db, repo.id); |
| 62 | 64 | if (reviewers.length === 0) return 0; |
| 65 | const room = | |
| 66 | repo.reviewerSlots >= PILLAR_MAX | |
| 67 | ? PASS_SENDS | |
| 68 | : Math.min(PASS_SENDS, repo.reviewerSlots - reviewsInFlight(app.db, repo.id)); | |
| 63 | 69 | let sent = 0; |
| 64 | 70 | for (const issue of held) { |
| 65 | if (sent >= PASS_SENDS) break; | |
| 71 | if (sent >= room) break; | |
| 66 | 72 | const merged = mergedPullClosing(app.db, repo.id, issue.number); |
| 67 | 73 | const pull = merged === undefined ? undefined : getPull(app.db, repo.id, merged.number); |
| 68 | 74 | if (pull === undefined || pull.mergedAt === null || passed(app, repo, issue, pull)) continue; |
test/box-pass.test.ts +20
| @@ -21,6 +21,7 @@ import { getPull, type Pull } from "../src/core/pulls.ts"; | ||
| 21 | 21 | import { getRepoById, type Repo } from "../src/core/repos.ts"; |
| 22 | 22 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; |
| 23 | 23 | import { landedBoxes, passed, passLanded } from "../src/crew/box_pass.ts"; |
| 24 | import { reviewsInFlight } from "../src/crew/review.ts"; | |
| 24 | 25 | import { taskPrompt, type TaskInput } from "../src/crew/brief.ts"; |
| 25 | 26 | import { serve, type Servers } from "../src/http/server.ts"; |
| 26 | 27 | |
| @@ -249,6 +250,25 @@ describe("the reviewer's pass over the boxes", () => { | ||
| 249 | 250 | expect(passLanded(app, fresh(), [now(asked)])).toBe(1); |
| 250 | 251 | expect(passed(app, fresh(), asked, landed)).toBe(true); |
| 251 | 252 | }); |
| 253 | ||
| 254 | test("the reviewer's slots bound the pass: paused sends none, a full seat waits (#8734)", async () => { | |
| 255 | const first = issue("Capped one", "- [ ] a"); | |
| 256 | const second = issue("Capped two", "- [ ] b"); | |
| 257 | await land(await propose("capped-one", first)); | |
| 258 | await land(await propose("capped-two", second)); | |
| 259 | const slots = (n: number) => app.db.query("UPDATE repo SET reviewer_slots = ?2 WHERE id = ?1").run(repo.id, n); | |
| 260 | const inFlight = () => reviewsInFlight(app.db, repo.id); | |
| 261 | app.db.query("UPDATE run SET state = 'cancelled' WHERE repo_id = ?1 AND state IN ('queued','running')").run(repo.id); | |
| 262 | slots(0); | |
| 263 | expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(0); | |
| 264 | slots(1); | |
| 265 | expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(1); | |
| 266 | expect(inFlight()).toBe(1); | |
| 267 | expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(0); | |
| 268 | app.db.query("UPDATE run SET state = 'cancelled' WHERE repo_id = ?1 AND state IN ('queued','running')").run(repo.id); | |
| 269 | expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(1); | |
| 270 | slots(8); | |
| 271 | }); | |
| 252 | 272 | }); |
| 253 | 273 | |
| 254 | 274 | describe("the review-states door", () => { |
test/seat-slots.test.ts +61 −2
| @@ -5,7 +5,7 @@ | ||
| 5 | 5 | * a change is audited and takes effect on the next tick, and the MCP `crew` |
| 6 | 6 | * tool sets the same numbers. |
| 7 | 7 | */ |
| 8 | import { afterAll, beforeAll, expect, setDefaultTimeout, test } from "bun:test"; | |
| 8 | import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test"; | |
| 9 | 9 | import { mkdtemp, rm } from "node:fs/promises"; |
| 10 | 10 | import { tmpdir } from "node:os"; |
| 11 | 11 | import { join } from "node:path"; |
| @@ -15,7 +15,9 @@ import { commitToBranch, createRepo, updateRepoSettings } from "../src/actions/r | ||
| 15 | 15 | import { setCrewRole } from "../src/core/agents.ts"; |
| 16 | 16 | import { reviewWaits, waitLine } from "../src/actions/owner_waits.ts"; |
| 17 | 17 | import { checkAutonomy, getRepoById, type Repo } from "../src/core/repos.ts"; |
| 18 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 18 | import { openPull } from "../src/actions/pulls.ts"; | |
| 19 | import type { Pull } from "../src/core/pulls.ts"; | |
| 20 | import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts"; | |
| 19 | 21 | import { createToken } from "../src/core/tokens.ts"; |
| 20 | 22 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; |
| 21 | 23 | import { driveRepo } from "../src/crew/autonomy.ts"; |
| @@ -208,3 +210,60 @@ test("the reviewer's cap counts the reviews in flight on this repository only", | ||
| 208 | 210 | expect(reviewsInFlight(app.db, repo.id)).toBe(before + 1); |
| 209 | 211 | expect(reviewsInFlight(app.db, "other-repo")).toBe(0); |
| 210 | 212 | }); |
| 213 | ||
| 214 | describe("the reviewer's cap, end to end (#8734)", () => { | |
| 215 | let porch: Repo; | |
| 216 | let n = 0; | |
| 217 | const now = new Date(); | |
| 218 | const porchNow = () => getRepoById(app.db, porch.id)!; | |
| 219 | const reviews = (pull: Pull) => listRuns(app.db, porch.id, { issueId: pull.id }).filter((r) => r.reason === "review"); | |
| 220 | // A green agent pull: its check went green a minute before `now`, so review is due. | |
| 221 | const agentPull = async (): Promise<Pull> => { | |
| 222 | const branch = `cap${++n}`; | |
| 223 | ok(await commitToBranch(app, alice, porch, { branch, base: "main", message: branch, files: [{ path: `${branch}.md`, content: "x\n" }] })); | |
| 224 | const pull = ok(await openPull(app, alice, porch, { title: branch, body: "", baseBranch: "main", headBranch: branch })); | |
| 225 | const run = createRun(app.db, { repoId: porch.id, kind: "check", issueId: pull.id, env: `pull-${pull.number}`, commitOid: pull.headOid, reason: "pull" }); | |
| 226 | claimRun(app.db, run.id); | |
| 227 | finishRun(app.db, run.id, { state: "ok", summary: "ok" }); | |
| 228 | const green = new Date(now.getTime() - 60_000).toISOString(); | |
| 229 | app.db.query("UPDATE run SET created_at = ?2, finished_at = ?2 WHERE id = ?1").run(run.id, green); | |
| 230 | return { ...pull, authorIsAgent: true }; | |
| 231 | }; | |
| 232 | ||
| 233 | beforeAll(async () => { | |
| 234 | porch = ok(await createRepo(app, alice, { name: "porch", visibility: "private" })); | |
| 235 | ok(await commitToBranch(app, alice, porch, { message: "Start", files: [{ path: "README.md", content: "# porch\n" }] })); | |
| 236 | expect(addToCrew(app, alice, porch, "judge").ok).toBe(true); | |
| 237 | setCrewRole(app.db, porch.id, getUserByName(app.db, "judge")!.id, "reviewer"); | |
| 238 | }); | |
| 239 | ||
| 240 | test("at 1, the second agent pull is queued only after the first review ends", async () => { | |
| 241 | ok(await updateRepoSettings(app, alice, porchNow(), { reviewerSlots: 1 })); | |
| 242 | // A review queued on another repository takes none of this one's slot. | |
| 243 | createRun(app.db, { repoId: repo.id, kind: "agent", actorId: getUserByName(app.db, "judge")!.id, reason: "review" }); | |
| 244 | const first = await agentPull(); | |
| 245 | const second = await agentPull(); | |
| 246 | expect(await sendToReview(app, porchNow(), first, now)).toBe("sent"); | |
| 247 | expect(await sendToReview(app, porchNow(), second, now)).toBe("wait"); | |
| 248 | expect(reviews(first)).toHaveLength(1); | |
| 249 | expect(reviews(second)).toHaveLength(0); | |
| 250 | // Running still holds the slot. | |
| 251 | const run = reviews(first)[0]!; | |
| 252 | claimRun(app.db, run.id); | |
| 253 | expect(await sendToReview(app, porchNow(), second, now)).toBe("wait"); | |
| 254 | finishRun(app.db, run.id, { state: "ok", summary: "", costUsd: 0 }); | |
| 255 | expect(await sendToReview(app, porchNow(), second, now)).toBe("sent"); | |
| 256 | expect(reviews(second)).toHaveLength(1); | |
| 257 | app.db.query("UPDATE run SET state = 'cancelled' WHERE state = 'queued'").run(); | |
| 258 | }); | |
| 259 | ||
| 260 | test("at 0, neither agent pull is queued: both wait for a person", async () => { | |
| 261 | ok(await updateRepoSettings(app, alice, porchNow(), { reviewerSlots: 0 })); | |
| 262 | const a = await agentPull(); | |
| 263 | const b = await agentPull(); | |
| 264 | expect(await sendToReview(app, porchNow(), a, now)).toBe("none"); | |
| 265 | expect(await sendToReview(app, porchNow(), b, now)).toBe("none"); | |
| 266 | expect(reviews(a)).toHaveLength(0); | |
| 267 | expect(reviews(b)).toHaveLength(0); | |
| 268 | }); | |
| 269 | }); |
new above
offline