trevato/forge

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

More
Forks1g fMapg aRuns12g rEventsFlows

Reviews are the bottleneck: reviewer runs fail on review anchors, count as failed for pushing nothing, and hold slots #552

Merged masoncrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

mason/forge:mason/487-reviews-are-the-bottleneck-reviewer-runs → main 11 commits fork

  • Merged. mason/487-reviews-are-the-bottleneck-reviewer-runs was merged into main as 70cca8f.

Review

Sign in to review this change.
18 files changed +1318 −65
docs/ORIGINS.md +7 −1
@@ -466,7 +466,7 @@ Review states and threads, on any repo:
466466
467467 ```
468468 GET /api/v1/repos/:o/:n/pulls/:n/review-states the verdicts
469 POST /api/v1/repos/:o/:n/pulls/:n/review-states {state, body?} → 201
469 POST /api/v1/repos/:o/:n/pulls/:n/review-states {state, body?, comments?} → 201
470470 GET /api/v1/repos/:o/:n/pulls/:n/threads anchors + resolution
471471 POST /api/v1/repos/:o/:n/pulls/:n/threads/:anchor/resolve
472472 DELETE /api/v1/repos/:o/:n/pulls/:n/threads/:anchor/resolve
@@ -478,6 +478,12 @@ rather than a mode of the same one, because a comment on a line and a verdict
478478 on a change are different things with different rules, and one path meaning
479479 two of them behind a query parameter is the shape that gets called wrong.
480480 Each line comment now carries `anchor`, the thread it belongs to.
481
482 A review that comes with line comments is still one call: `comments` on
483 `…/review-states` is a list of `{path, line, side, body}`, at most 50, posted
484 before the verdict and all or none — one anchor outside the diff refuses the
485 whole review with `bad_anchor`, and the message says how an anchor is written.
486 This is the one way the crew's reviewer brief posts a review.
481487 `GET …/pulls/:number` gains `status.reviews:
482488 { approved, changesRequested, required, mergeable }`.
483489
docs/PLANNING.md +19 −5
@@ -134,11 +134,25 @@ asked, under **Settings → Autonomy**:
134134 about a green pull reaches a person: first, a pull that no longer merges
135135 because `main` moved under it sends its agent one run (`conflict`, naming
136136 the files) to merge `main` into the branch, one waiting at a time, up to
137 the same limit a failing check has; then the decider reads the pull
138 against the direction and either lands it or hands it to the crew's
139 _reviewer_ (`review` run: the diff, the preview, the pictures the forge
140 took), whose approval lands it and whose change request goes back to the
141 builder as feedback; a crew with no reviewer lands it. Nothing waits for a
137 the same limit a failing check has; then the green head goes to the
138 crew's _reviewer_ (`review` run: the diff, the preview, the pictures the
139 forge took), whose approval lands it and whose change request goes back
140 to the builder as feedback; a crew with no reviewer lands it. With two
141 reviewers, a change to sign-in, tokens, secrets, the sandbox or the HTTP
142 edge goes to the one whose brief says it reads for security, and every
143 other change to the other (`crew/review.ts`). Every green pull an agent
144 wrote is sent this way in any repository with a reviewer, autonomous or
145 not; a review run is `ok` when it posted a verdict, since a reviewer
146 pushes nothing. A review that does not come back — the run ends without
147 a verdict, or is still out after 20 minutes — is sent once more; after
148 the second, or an hour green with no verdict however the runs went, the
149 review has lapsed and the pull lands on green with a note saying so. A
150 missing reviewer never holds a pull silently. An agent's verdict stands
151 only while the agent is a reviewer on the crew, and the reviewer gets two
152 rounds of changes on a pull — a round is a head it asked changes on;
153 past them its request is advice. A builder's run that answers a change
154 request without pushing parks the pull. A review run never resumes an
155 earlier session, and the same verdict sent twice on one head is one. Nothing waits for a
142156 person: a pull whose check stays red past its tries, or whose branch
143157 could not be brought up to date, is _parked_ — closed with the reason,
144158 its issue put back on the map unassigned with the same note — and the
forge.json +1 −1
@@ -6,7 +6,7 @@
66 "ready": 90
77 },
88 "check": {
9 "run": "bun x tsc --noEmit && bun scripts/test-shards.ts --timeout 60000 -- crew crew-wrapup crew-limits crew-parks crew-sandbox steer session-settle landing decide ledger checks dependencies map-closing autonomy studio studio-words forks fork-flow cross-repo-pull pulls apps apps-adversarial apps-busy apps-token surfaces-adversarial orgs web git-trust helpers shards migrate vitals guard asks feed feed-page offline share cloud run-secrets forge-cloudflare usage credentials standing board-windows spawn-plan spawn web-apps fleet-ops meter told-by",
9 "run": "bun x tsc --noEmit && bun scripts/test-shards.ts --timeout 60000 -- crew review-route review-lines review-gate crew-wrapup crew-limits crew-parks crew-sandbox steer session-settle landing decide ledger checks dependencies map-closing autonomy studio studio-words forks fork-flow cross-repo-pull pulls apps apps-adversarial apps-busy apps-token surfaces-adversarial orgs web git-trust helpers shards migrate vitals guard asks feed feed-page offline share cloud run-secrets forge-cloudflare usage credentials standing board board-windows spawn-plan spawn web-apps fleet-ops meter told-by",
1010 "timeout": 1800
1111 }
1212 }
src/actions/pulls.ts +94 −14
@@ -13,6 +13,7 @@ import {
1313 type BodyError,
1414 type Issue,
1515 getIssue,
16 MAX_BODY,
1617 setIssueState as setIssueStateCore,
1718 } from "../core/issues.ts";
1819 import {
@@ -38,6 +39,7 @@ import {
3839 hashLine,
3940 isReviewState,
4041 listReviewComments,
42 listReviews,
4143 resolveThread,
4244 submitReview as recordReview,
4345 threadComments,
@@ -246,29 +248,70 @@ export async function addReview(
246248 const user = requireParticipant(app, actor, repo);
247249 if (!user.ok) return user;
248250 if (pull.repoId !== repo.id) return err("not_found");
249 if (!Number.isInteger(input.line) || input.line < 1) return err("bad_anchor");
251 const anchored = await anchorLines(app, repo, pull, [input]);
252 if (!anchored.ok) return anchored;
253 return postLine(app, pull, user.value, anchored.value[0]!);
254 }
255
256 /** A line comment, with the head and line content it was anchored to. */
257 interface AnchoredLine extends ReviewInput {
258 readonly commitOid: string;
259 readonly lineHash: string;
260 }
261
262 /**
263 * Each line comment anchored to the pull's current diff, or the first
264 * reason one cannot be: a body that is empty or too long, or an anchor
265 * that is not a row of the diff. All or none, so a review never lands
266 * with half its comments.
267 */
268 async function anchorLines(
269 app: App,
270 repo: Repo,
271 pull: Pull,
272 lines: readonly ReviewInput[],
273 ): Promise<Result<AnchoredLine[], BodyError | "bad_anchor">> {
250274 const { status, diff } = await pullDiff(
251275 repoDir(app.reposRoot, repo.ownerName, repo.name),
252276 pull,
253277 );
254 const content = anchorContent(diff, input.path, input.side, input.line);
255 if (content === undefined) return err("bad_anchor");
278 const out: AnchoredLine[] = [];
279 for (const line of lines) {
280 if (!Number.isInteger(line.line) || line.line < 1)
281 return err("bad_anchor");
282 const content = anchorContent(diff, line.path, line.side, line.line);
283 if (content === undefined) return err("bad_anchor");
284 const body = line.body.trim();
285 if (body === "") return err("empty_body");
286 if (body.length > MAX_BODY) return err("body_too_long");
287 out.push({ ...line, commitOid: status.headOid, lineHash: hashLine(content) });
288 }
289 return ok(out);
290 }
291
292 /** Record one anchored line comment and say so on the outbox. */
293 function postLine(
294 app: App,
295 pull: Pull,
296 user: User,
297 line: AnchoredLine,
298 ): Result<ReviewComment, BodyError> {
256299 const added = addReviewComment(app.db, {
257300 pullId: pull.id,
258 authorId: user.value.id,
259 body: input.body,
260 path: input.path,
261 line: input.line,
262 side: input.side,
263 commitOid: status.headOid,
264 lineHash: hashLine(content),
301 authorId: user.id,
302 body: line.body,
303 path: line.path,
304 line: line.line,
305 side: line.side,
306 commitOid: line.commitOid,
307 lineHash: line.lineHash,
265308 });
266309 if (!added.ok) return added;
267310 app.bus.append({
268311 kind: "pull.reviewed",
269 repoId: repo.id,
270 actorId: user.value.id,
271 payload: { number: pull.number, path: input.path, line: input.line },
312 repoId: pull.repoId,
313 actorId: user.id,
314 payload: { number: pull.number, path: line.path, line: line.line },
272315 });
273316 return added;
274317 }
@@ -391,11 +434,20 @@ export type SubmitReviewActionError =
391434 | "body_too_long"
392435 | "invalid_state"
393436 /** You cannot approve your own change. */
394 | "self_review";
437 | "self_review"
438 | "empty_body"
439 /** A line comment's anchor is not a row of the pull's current diff. */
440 | "bad_anchor"
441 | "too_many_lines";
442
443 /** The most line comments one review may carry. */
444 export const MAX_REVIEW_LINES = 50;
395445
396446 export interface SubmitReviewInput {
397447 readonly state: ReviewState;
398448 readonly body?: string;
449 /** Line comments posted with the verdict, all or none. */
450 readonly comments?: readonly ReviewInput[];
399451 }
400452
401453 /**
@@ -438,6 +490,13 @@ export async function submitReview(
438490 )
439491 return err("forbidden");
440492
493 const lines = input.comments ?? [];
494 if (lines.length > MAX_REVIEW_LINES) return err("too_many_lines");
495 // Every anchor is checked before anything is written: a review with one
496 // wrong line posts nothing, and the reviewer can fix it and send it again.
497 const anchored = await anchorLines(app, repo, pull, lines);
498 if (!anchored.ok) return anchored;
499
441500 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
442501 const status = await pullStatus(dir, pull);
443502
@@ -461,6 +520,8 @@ export async function submitReview(
461520 actorId: user.id,
462521 payload: { number: pull.number, state: input.state, via: "origin" },
463522 });
523 // Line comments stay here, as `addReview`'s always have on a mirror.
524 for (const line of anchored.value) postLine(app, pull, user, line);
464525 const stored = getReviewByRemoteId(app.db, pull.id, sent.value.remoteId);
465526 return stored ? ok(stored) : err("origin_refused");
466527 }
@@ -468,6 +529,25 @@ export async function submitReview(
468529 if (input.state === "approved" && pull.authorId === user.id)
469530 return err("self_review");
470531
532 // The same verdict again on the same head — a retried call, a run that
533 // sent it twice — is the one already there, not a second review.
534 const again = listReviews(app.db, pull.id).findLast(
535 (r) => r.authorId === user.id,
536 );
537 if (
538 again !== undefined &&
539 again.commitOid === status.headOid &&
540 again.state === input.state &&
541 again.body.trim() === (input.body ?? "").trim()
542 )
543 return ok(again);
544
545 // The lines first, so whoever the verdict wakes reads them with it.
546 for (const line of anchored.value) {
547 const posted = postLine(app, pull, user, line);
548 if (!posted.ok) return posted;
549 }
550
471551 const recorded = recordReview(app.db, {
472552 issueId: pull.id,
473553 authorId: user.id,
src/core/review-stats.ts added +75
@@ -0,0 +1,75 @@
1 /**
2 * How review is keeping up, per repository: the crew's pulls that are green
3 * and waiting for a verdict, how long a green head waits for one, and how
4 * many verdicts each reviewer on the crew passed. Read from the review and
5 * run tables; nothing here is stored.
6 */
7
8 import type { Database } from "bun:sqlite";
9
10 export interface ReviewStats {
11 /** Open pulls an agent wrote whose head is green and has no verdict on it. */
12 readonly awaiting: number;
13 /** The median wait from a head's first passing check to an agent's verdict on it, over the window; null with none. */
14 readonly latencyMs: number | null;
15 /** Verdicts each agent passed in the window, most first. */
16 readonly byReviewer: readonly { readonly name: string; readonly count: number }[];
17 }
18
19 /** The middle of some numbers, or null when there are none. */
20 function median(values: readonly number[]): number | null {
21 if (values.length === 0) return null;
22 const sorted = [...values].sort((a, b) => a - b);
23 const mid = Math.floor(sorted.length / 2);
24 return sorted.length % 2 === 1
25 ? sorted[mid]!
26 : (sorted[mid - 1]! + sorted[mid]!) / 2;
27 }
28
29 /** Review on one repository, its verdicts counted from `since` (an ISO time). */
30 export function reviewStats(
31 db: Database,
32 repoId: string,
33 since: string,
34 ): ReviewStats {
35 const awaiting =
36 db
37 .query<{ n: number }, [string]>(
38 `SELECT COUNT(*) AS n FROM issue
39 JOIN pull ON pull.issue_id = issue.id
40 JOIN user AS author ON author.id = issue.author_id AND author.is_agent = 1
41 WHERE issue.repo_id = ?1 AND issue.state = 'open' AND pull.merged_at IS NULL
42 AND EXISTS (SELECT 1 FROM run WHERE run.issue_id = issue.id
43 AND run.commit_oid = pull.head_oid AND run.kind IN ('check','deploy'))
44 AND NOT EXISTS (SELECT 1 FROM run WHERE run.issue_id = issue.id
45 AND run.commit_oid = pull.head_oid AND run.kind IN ('check','deploy')
46 AND run.state <> 'ok')
47 AND NOT EXISTS (SELECT 1 FROM review WHERE review.issue_id = issue.id
48 AND review.commit_oid = pull.head_oid AND review.state <> 'commented')`,
49 )
50 .get(repoId)?.n ?? 0;
51 const waits = db
52 .query<{ ms: number | null }, [string, string]>(
53 `SELECT (julianday(review.created_at) - julianday(
54 (SELECT MIN(run.finished_at) FROM run WHERE run.issue_id = review.issue_id
55 AND run.commit_oid = review.commit_oid AND run.kind IN ('check','deploy')
56 AND run.state = 'ok'))) * 86400000 AS ms
57 FROM review
58 JOIN issue ON issue.id = review.issue_id
59 JOIN user ON user.id = review.author_id AND user.is_agent = 1
60 WHERE issue.repo_id = ?1 AND review.created_at >= ?2`,
61 )
62 .all(repoId, since)
63 .map((r) => r.ms)
64 .filter((ms): ms is number => ms !== null && ms >= 0);
65 const byReviewer = db
66 .query<{ name: string; count: number }, [string, string]>(
67 `SELECT user.username AS name, COUNT(*) AS count FROM review
68 JOIN issue ON issue.id = review.issue_id
69 JOIN user ON user.id = review.author_id AND user.is_agent = 1
70 WHERE issue.repo_id = ?1 AND review.created_at >= ?2
71 GROUP BY user.username ORDER BY count DESC, name`,
72 )
73 .all(repoId, since);
74 return { awaiting, latencyMs: median(waits), byReviewer };
75 }
src/core/reviews.ts +60
@@ -358,6 +358,7 @@ export function reviewSummary(
358358 issueId: string,
359359 headOid: string,
360360 ): ReviewSummary {
361 const agents = agentVerdicts(db, issueId);
361362 let approvals = 0;
362363 let changesRequested = 0;
363364 let commented = 0;
@@ -367,6 +368,24 @@ export function reviewSummary(
367368 if (review.state !== "commented") stale++;
368369 continue;
369370 }
371 const agent = agents.authors.get(review.authorId);
372 // An agent's verdict is its role's, not its own: it stands only while
373 // the agent is a reviewer on this crew, so one taken off the crew stops
374 // gating the pulls it judged.
375 if (agent !== undefined && !agent.reviewer) {
376 commented++;
377 continue;
378 }
379 // The crew's reviewer gets REVIEW_ROUNDS rounds of changes on a pull;
380 // past them, what it still asks is advice, and the pull lands as it is.
381 if (
382 agent !== undefined &&
383 review.state === "changes_requested" &&
384 agents.earlierRounds(headOid) >= REVIEW_ROUNDS
385 ) {
386 commented++;
387 continue;
388 }
370389 if (review.state === "approved") approvals++;
371390 else if (review.state === "changes_requested") changesRequested++;
372391 else commented++;
@@ -374,6 +393,47 @@ export function reviewSummary(
374393 return { approvals, changesRequested, commented, stale };
375394 }
376395
396 /** Rounds of changes the crew's reviewer may ask for on one pull: a round is a head it asked changes on. */
397 export const REVIEW_ROUNDS = 2;
398
399 /**
400 * What the gate needs to know about the agents who passed verdicts on a
401 * pull: whether each is a reviewer on the pull's crew now, and on how many
402 * heads other than a given one an agent asked for changes.
403 */
404 function agentVerdicts(
405 db: Database,
406 issueId: string,
407 ): {
408 authors: Map<string, { reviewer: boolean }>;
409 earlierRounds: (headOid: string) => number;
410 } {
411 const rows = db
412 .query<
413 { author_id: string; role: string | null; commit_oid: string; state: string },
414 [string]
415 >(
416 `SELECT review.author_id, repo_crew.role, review.commit_oid, review.state
417 FROM review
418 JOIN issue ON issue.id = review.issue_id
419 JOIN user ON user.id = review.author_id AND user.is_agent = 1
420 LEFT JOIN repo_crew ON repo_crew.repo_id = issue.repo_id
421 AND repo_crew.user_id = review.author_id
422 WHERE review.issue_id = ?1`,
423 )
424 .all(issueId);
425 const authors = new Map(
426 rows.map((r) => [r.author_id, { reviewer: r.role === "reviewer" }]),
427 );
428 const asked = new Set(
429 rows.filter((r) => r.state === "changes_requested").map((r) => r.commit_oid),
430 );
431 return {
432 authors,
433 earlierRounds: (headOid) => [...asked].filter((oid) => oid !== headOid).length,
434 };
435 }
436
377437 // ---- threads ----
378438
379439 /**
src/crew/autonomy.ts +72 −28
@@ -85,6 +85,7 @@ import { interruptionOf } from "./interrupted.ts";
8585 import { parkedCount as countParks, PARK_STRIKES } from "./parks.ts";
8686 import { createLog } from "../kernel/log.ts";
8787 import { crewOffInPreview } from "./preview.ts";
88 import { reviewersOf, sendToReview, sweepReviews } from "./review.ts";
8889
8990 const log = createLog("autonomy");
9091
@@ -597,6 +598,7 @@ export function settleAgentRun(
597598 if (run.reason === "review") return false;
598599 if (/opened pull #|updated pull #/.test(run.summary)) return false;
599600 if (interruptionOf(run.summary) !== undefined) return false;
601 if (answeredWithoutChange(app, repo, run)) return true;
600602 const issue = getIssueById(app.db, run.issueId);
601603 if (!issue || issue.kind !== "issue" || issue.state !== "open") return false;
602604 if (issue.id === repo.directionIssueId) return false;
@@ -613,6 +615,33 @@ export function settleAgentRun(
613615 );
614616 }
615617
618 /**
619 * A builder's run on its pull that ended without a push while the crew's
620 * reviewer still asks for changes on the head: the request was not
621 * answered, and nothing will answer it. The pull is parked rather than
622 * left waiting on a verdict that will not move.
623 */
624 function answeredWithoutChange(
625 app: App,
626 repo: Repo,
627 run: { readonly issueId: string; readonly reason: string; readonly actorId: string | null },
628 ): boolean {
629 if (run.reason !== "feedback" || run.actorId === null) return false;
630 if (crewRole(app.db, repo.id, run.actorId) !== "builder") return false;
631 const issue = getIssueById(app.db, run.issueId);
632 if (!issue || issue.kind !== "pull" || issue.state !== "open") return false;
633 const pull = getPull(app.db, repo.id, issue.number);
634 if (!pull || pull.mergedAt !== null) return false;
635 if (reviewSummary(app.db, pull.id, pull.headOid).changesRequested === 0)
636 return false;
637 return parkPull(
638 app,
639 repo,
640 pull,
641 "the reviewer asked for changes and the builder's run ended without pushing any",
642 );
643 }
644
616645 /**
617646 * Park a pull the crew cannot finish: close it with the reason, put its
618647 * issues back on the map unassigned with the same reason, and move on. A
@@ -740,6 +769,13 @@ export async function autonomyTick(app: App, now = new Date()): Promise<void> {
740769 });
741770 }
742771 }
772 // Review is not autonomy's alone: an agent's green pull is read by the
773 // crew's reviewer in every repository that has one.
774 try {
775 await sweepReviews(app);
776 } catch (error) {
777 log.error("review sweep failed", { error: String(error) });
778 }
743779 }
744780
745781 /**
@@ -847,34 +883,24 @@ export function bringUpToDate(
847883 return agent.user.username;
848884 }
849885
850 /** The heads already sent to the reviewer, by pull: one pass per head. */
851 const sentToReview = new Map<string, string>();
852
853886 /**
854 * Whether the review gate lets this green head land. True with no reviewer
855 * on the crew, or with an approval on this head and no change requested.
856 * Otherwise the reviewer is queued once for the head and the pull waits.
887 * Whether the review gate lets this green head land: `pass` with no
888 * reviewer on the crew, or with an approval on this head and no change
889 * requested; `hold` while the review is out or changes are asked for; and
890 * `lapsed` when the reviewer was asked twice and never answered — the pull
891 * lands without it, and says so (`crew/review.ts`).
857892 */
858 async function reviewed(app: App, repo: Repo, pull: Pull): Promise<boolean> {
859 const reviewer = listCrewRoles(app.db, repo.id).find(
860 (c) => c.role === "reviewer",
861 )?.agent;
862 if (reviewer === undefined) return true;
893 async function reviewed(
894 app: App,
895 repo: Repo,
896 pull: Pull,
897 ): Promise<"pass" | "hold" | "lapsed"> {
898 if (reviewersOf(app.db, repo.id).length === 0) return "pass";
863899 const seen = reviewSummary(app.db, pull.id, pull.headOid);
864 if (seen.changesRequested > 0) return false;
865 if (seen.approvals > 0) return true;
866 if (sentToReview.get(pull.id) === pull.headOid) return false;
867 if (activeRunFor(app.db, "agent", pull.id) !== undefined) return false;
868 const issue = getIssueById(app.db, pull.id);
869 if (!issue) return false;
870 if (queueAgentRun(app, repo, issue, reviewer, "review") !== undefined) {
871 sentToReview.set(pull.id, pull.headOid);
872 log.info("green; queued for review", {
873 repo: `${repo.ownerName}/${repo.name}`,
874 pull: pull.number,
875 });
876 }
877 return false;
900 if (seen.changesRequested > 0) return "hold";
901 if (seen.approvals > 0) return "pass";
902 const stage = await sendToReview(app, repo, pull);
903 return stage === "lapsed" ? "lapsed" : stage === "none" ? "pass" : "hold";
878904 }
879905
880906 /**
@@ -968,8 +994,10 @@ export async function landIfGreen(
968994 // One gate for every pull, whoever wrote it: when the crew has a
969995 // reviewer, a green head is queued for review and waits for an approval
970996 // on that very head (a person's counts the same). A push is a new head,
971 // so it starts over. Without a reviewer the pull lands as it always has.
972 if (!(await reviewed(app, repo, pull))) return false;
997 // so it starts over. Without a reviewer the pull lands as it always has,
998 // and so does one whose review lapsed.
999 const review = await reviewed(app, repo, pull);
1000 if (review === "hold") return false;
9731001 // Green and clear of the wall lands. An autonomous repository does not
9741002 // wait on a person; the ledger keeps the landing as the rule it is, once
9751003 // per head, so the week's numbers still add up.
@@ -986,7 +1014,10 @@ export async function landIfGreen(
9861014 kind: "land",
9871015 repoId: repo.id,
9881016 issueId: pull.id,
989 state: { head: pull.headOid },
1017 state:
1018 review === "lapsed"
1019 ? { head: pull.headOid, review: "lapsed" }
1020 : { head: pull.headOid },
9901021 answer: "ship",
9911022 p: 1,
9921023 by: "rule",
@@ -1010,6 +1041,19 @@ export async function landIfGreen(
10101041 }
10111042 conflicted.delete(pull.id);
10121043 resolveOpen(app.db, pull.id, "land", "shipped");
1044 // Said once it has landed, so the note wakes no builder: the review was
1045 // asked for and never came.
1046 if (review === "lapsed") {
1047 const landed = getIssueById(app.db, pull.id);
1048 if (landed)
1049 addComment(
1050 app,
1051 by,
1052 repo,
1053 landed,
1054 "Landed on green without a review: the crew's reviewer was asked twice and posted no verdict.",
1055 );
1056 }
10131057 for (const number of closesFromBody(pull.body)) {
10141058 const closed = getIssue(app.db, repo.id, number);
10151059 if (closed) resolveOpen(app.db, closed.id, "next", "shipped");
src/crew/brief.ts +11 −6
@@ -11,7 +11,11 @@ import type { Agent, CrewRole } from "../core/agents.ts";
1111 import type { Comment, Issue } from "../core/issues.ts";
1212 import type { Pull } from "../core/pulls.ts";
1313 import type { Repo } from "../core/repos.ts";
14 import type { Review, ReviewComment } from "../core/reviews.ts";
14 import {
15 REVIEW_ROUNDS,
16 type Review,
17 type ReviewComment,
18 } from "../core/reviews.ts";
1519
1620 export type RunReason =
1721 | "assigned"
@@ -197,7 +201,7 @@ function taskBody(input: TaskInput): string {
197201 if (input.reason === "review") {
198202 const pull = input.pull;
199203 out.push(
200 `You are the reviewer on this crew. Pull #${issue.number} — "${issue.title}" — is green and would land on its own; the crew's decider wants a second pair of eyes first, because it may be large, off the direction, or touch money, sign-in, data, or what people see first.`,
204 `You are the reviewer on this crew. Pull #${issue.number} — "${issue.title}" — is green; every pull the crew writes is read by a reviewer before it lands, and this one is yours.`,
201205 "",
202206 `The direction of this repository: ${input.repo.autonomyDirection.trim() || "(none written)"}`,
203207 "",
@@ -206,7 +210,7 @@ function taskBody(input: TaskInput): string {
206210 ...(input.previewUrl === undefined
207211 ? []
208212 : [
209 `- The preview is at ${input.previewUrl}. Use it as a person would: curl its pages, try the doors it offers.`,
213 `- This pull's preview, at this head, is at ${input.previewUrl}. Use it as a person would: curl its pages, try the doors it offers. Judge this preview only — another pull's preview is not this change.`,
210214 ]),
211215 ...(input.looks !== undefined && input.looks.length > 0
212216 ? [
@@ -227,9 +231,10 @@ function taskBody(input: TaskInput): string {
227231 "- The contract: every action asks `permit` and none compares roles by hand; a new door has its row in `src/contract.test.ts`; and no cell in `contract.json` is loosened unless the issue asked for exactly that — a loosened cell is a wall a person crosses, so a pull that loosens one without being asked is a change request, whatever else it does well.",
228232 ]),
229233 "",
230 "Then submit exactly one review through the forge's API and stop:",
231 ` curl -sS -X POST -H "authorization: Bearer $FORGE_TOKEN" -H "content-type: application/json" "$FORGE_URL/api/v1/repos/${input.repo.ownerName}/${input.repo.name}/pulls/${issue.number}/reviews" -d '{"state":"approved","body":"..."}'`,
232 `Approve when it does what the direction asks, works in the preview, and looks right. Otherwise use "changes_requested" with a body a builder can act on: what does not work, and the one thing to fix first, concrete. You get two rounds of changes on a pull; after that it lands as it is, so if something is left, open an issue for it with the API instead. One review, then stop; your last message is not read by anyone.`,
234 "Then submit exactly one review through the forge's API — the verdict and its line comments in one call — and stop:",
235 ` curl -sS -X POST -H "authorization: Bearer $FORGE_TOKEN" -H "content-type: application/json" "$FORGE_URL/api/v1/repos/${input.repo.ownerName}/${input.repo.name}/pulls/${issue.number}/review-states" -d '{"state":"approved","body":"...","comments":[{"path":"src/x.ts","line":12,"side":"new","body":"..."}]}'`,
236 `"comments" is optional. Each anchors to a line of the diff below: "path" is the file, "side" is "new" for an added or kept line (numbered as in the head) or "old" for a removed one (numbered as in the base), and "line" counts from the hunk header — in \`@@ -a,b +c,d @@\` the hunk's first line is c on the new side. If the API answers \`bad_anchor\`, nothing was posted: read its message, fix the anchors, and send the review once more; if it is refused again, send it without "comments" and put those points in the body.`,
237 `Approve when it does what the direction asks, works in the preview, and looks right. Otherwise use "changes_requested" with a body a builder can act on: what does not work, and the one thing to fix first, concrete. You get ${REVIEW_ROUNDS} rounds of changes on a pull; after that it lands as it is, so if something is left, open an issue for it with the API instead. One review, then stop; your last message is not read by anyone.`,
233238 "",
234239 "The change:",
235240 "```diff",
src/crew/dispatch.ts +16 −2
@@ -25,6 +25,7 @@ import { resolveOpen } from "../core/decisions.ts";
2525 import { decideAndRecord } from "./ledger.ts";
2626 import { getRepoById, type Repo } from "../core/repos.ts";
2727 import { listReviews } from "../core/reviews.ts";
28 import { greenHead, sendToReview } from "./review.ts";
2829 import {
2930 activeRunFor,
3031 hasQueuedRun,
@@ -336,9 +337,12 @@ function route(app: App, event: ForgeEvent): void {
336337 if (!actor) return;
337338 // Agents talking is not feedback — except the crew's reviewer passing
338339 // a verdict on a pull, which is exactly what a person's review is.
340 // Its line comments ride along with the verdict; the verdict alone
341 // wakes the builder, once.
339342 const reviewerVerdict =
340343 actor.kind === "agent" &&
341344 event.kind === "pull.reviewed" &&
345 typeof event.payload["state"] === "string" &&
342346 crewRole(app.db, repo.id, actor.id) === "reviewer";
343347 if (actor.kind === "agent" && !reviewerVerdict) return;
344348 const issue = getIssue(app.db, repo.id, number);
@@ -413,11 +417,21 @@ function route(app: App, event: ForgeEvent): void {
413417 "retry",
414418 event.payload["state"] === "ok" ? "green" : "red",
415419 );
416 // Green, in a repository that ships on green: land it.
420 // Green: an agent's pull goes to the crew's reviewer, and in a
421 // repository that ships on green it lands (behind that review).
417422 if (event.payload["state"] === "ok") {
418 if (repo.autonomy !== "autonomous") return;
419423 const pull = getPull(app.db, repo.id, issue.number);
420424 if (!pull) return;
425 if (repo.autonomy !== "autonomous") {
426 if (pull.authorIsAgent && greenHead(app.db, pull))
427 sendToReview(app, repo, pull).catch((error) =>
428 log.error("sending to review failed", {
429 pull: issue.number,
430 error: String(error),
431 }),
432 );
433 return;
434 }
421435 landIfGreen(app, repo, pull).catch((error) =>
422436 log.error("landing on green failed", {
423437 pull: issue.number,
src/crew/review.ts added +203
@@ -0,0 +1,203 @@
1 /**
2 * Who reviews a pull, and sending it to them.
3 *
4 * A crew may carry more than one reviewer: one who reads for the
5 * experience, one who reads for security. A pull that touches the
6 * security-bearing paths goes to the second; every other pull goes to the
7 * first. Which reviewer watches security is read from what the agent says
8 * it is — the first paragraph of its brief, before the rules every brief
9 * shares — so naming a reviewer is all it takes to give it the job.
10 *
11 * Every green head of a pull an agent wrote is sent, whatever the
12 * repository's autonomy: the event sends it the moment its checks pass,
13 * and the tick sweeps up any head the event missed. A review that does
14 * not come back is sent once more; after that it has lapsed, and the pull
15 * is not held for it (`reviewStage`).
16 */
17
18 import type { Database } from "bun:sqlite";
19 import type { App } from "../app.ts";
20 import { listCrewRoles, type Agent } from "../core/agents.ts";
21 import { getIssueById } from "../core/issues.ts";
22 import { listPulls, pullDiff, type Pull } from "../core/pulls.ts";
23 import { getRepoById, type Repo } from "../core/repos.ts";
24 import { reviewSummary } from "../core/reviews.ts";
25 import { checksForHead } from "../core/runs.ts";
26 import { repoDir } from "../git/repos.ts";
27 import { createLog } from "../kernel/log.ts";
28 import { queueAgentRun } from "./dispatch.ts";
29
30 const log = createLog("crew");
31
32 /** Paths whose change is the security reviewer's to read: sign-in, tokens, secrets, the sandbox, the HTTP edge. */
33 const SECURITY_PATH = /auth|token|secret|sandbox|http/i;
34
35 export const touchesSecurity = (paths: readonly string[]): boolean =>
36 paths.some((p) => SECURITY_PATH.test(p));
37
38 /** What an agent says it is: its brief's first paragraph. */
39 const focusOf = (brief: string): string =>
40 brief.trim().split(/\n\s*\n/)[0] ?? "";
41
42 /** Whether this reviewer says it reads for security. */
43 export const watchesSecurity = (agent: Agent): boolean =>
44 /\bsecurity\b/i.test(focusOf(agent.brief));
45
46 /**
47 * The reviewer for a change to these paths: the security reviewer when
48 * the change touches what it watches, the experience reviewer otherwise,
49 * and whichever reviewer there is when the crew has only one kind.
50 */
51 export function pickReviewer(
52 reviewers: readonly Agent[],
53 paths: readonly string[],
54 ): Agent | undefined {
55 const security = reviewers.filter(watchesSecurity);
56 const experience = reviewers.filter((r) => !watchesSecurity(r));
57 const preferred = touchesSecurity(paths) ? security : experience;
58 return preferred[0] ?? reviewers[0];
59 }
60
61 /** The crew's reviewers on a repository. */
62 export const reviewersOf = (db: Database, repoId: string): Agent[] =>
63 listCrewRoles(db, repoId)
64 .filter((c) => c.role === "reviewer")
65 .map((c) => c.agent);
66
67 /** Whether every check on the pull's head ran and passed. */
68 export function greenHead(db: Database, pull: Pull): boolean {
69 const checks = checksForHead(db, pull.id, pull.headOid);
70 return checks.length > 0 && checks.every((c) => c.state === "ok");
71 }
72
73 /** How long a review may take from being queued before it is sent again. */
74 export const REVIEW_WAIT_MS = 20 * 60_000;
75 /** How long a green head waits for a verdict at most, however the attempts went. */
76 export const REVIEW_LAPSE_MS = 60 * 60_000;
77 /** Review runs one head gets: the first, and one more. */
78 const REVIEW_TRIES = 2;
79
80 /** One review run sent for a head: when it was queued, and whether it is still out. */
81 export interface ReviewAttempt {
82 readonly createdAt: string;
83 readonly active: boolean;
84 }
85
86 /**
87 * Where review stands on a green head that has no verdict: `send` a
88 * review run (the first, or the one retry), `wait` for the one that is
89 * out, or `lapsed` — two runs came back with nothing, or the head has
90 * waited past the lapse — and the pull goes on without one. A missing
91 * reviewer must never hold a pull silently.
92 */
93 export function reviewStage(
94 greenAt: string,
95 attempts: readonly ReviewAttempt[],
96 now: Date,
97 ): "send" | "wait" | "lapsed" {
98 const at = now.getTime();
99 if (at - Date.parse(greenAt) >= REVIEW_LAPSE_MS) return "lapsed";
100 const last = attempts.at(-1);
101 if (last === undefined) return "send";
102 const fresh = at - Date.parse(last.createdAt) < REVIEW_WAIT_MS;
103 if (last.active && fresh) return "wait";
104 return attempts.length < REVIEW_TRIES ? "send" : "lapsed";
105 }
106
107 /**
108 * When the pull's head first went green, and the review runs sent for it
109 * since — read from the run table, so a restart forgets nothing.
110 */
111 export function reviewAttempts(
112 db: Database,
113 pull: Pull,
114 ): { greenAt: string; attempts: ReviewAttempt[] } | undefined {
115 const green = db
116 .query<{ at: string | null }, [string, string]>(
117 `SELECT MIN(finished_at) AS at FROM run WHERE issue_id = ?1 AND commit_oid = ?2
118 AND kind IN ('check','deploy') AND state = 'ok'`,
119 )
120 .get(pull.id, pull.headOid)?.at;
121 if (green === null || green === undefined) return undefined;
122 const attempts = db
123 .query<{ created_at: string; state: string }, [string, string]>(
124 `SELECT created_at, state FROM run WHERE issue_id = ?1 AND kind = 'agent'
125 AND reason = 'review' AND created_at >= ?2 ORDER BY created_at, id`,
126 )
127 .all(pull.id, green)
128 .map((r) => ({
129 createdAt: r.created_at,
130 active: r.state === "queued" || r.state === "running",
131 }));
132 return { greenAt: green, attempts };
133 }
134
135 /**
136 * Move review along on this green head: send it to the right reviewer
137 * when it is due (`reviewStage`), and say where it stands. `none` when
138 * there is nothing to wait for — no reviewer on the crew — and `done`
139 * when a verdict is on the head. A run the pull's builder still has open
140 * does not stop a send — the review queues behind it.
141 */
142 export async function sendToReview(
143 app: App,
144 repo: Repo,
145 pull: Pull,
146 now = new Date(),
147 ): Promise<"none" | "done" | "sent" | "wait" | "lapsed"> {
148 const reviewers = reviewersOf(app.db, repo.id);
149 if (reviewers.length === 0) return "none";
150 const seen = reviewSummary(app.db, pull.id, pull.headOid);
151 if (seen.approvals > 0 || seen.changesRequested > 0) return "done";
152 const history = reviewAttempts(app.db, pull);
153 if (history === undefined) return "wait";
154 const stage = reviewStage(history.greenAt, history.attempts, now);
155 if (stage !== "send") return stage;
156 const issue = getIssueById(app.db, pull.id);
157 if (!issue) return "wait";
158 const { diff } = await pullDiff(
159 repoDir(app.reposRoot, repo.ownerName, repo.name),
160 pull,
161 );
162 const reviewer = pickReviewer(
163 reviewers,
164 diff.files.map((f) => f.path),
165 );
166 if (reviewer === undefined) return "wait";
167 if (queueAgentRun(app, repo, issue, reviewer, "review") === undefined)
168 return "wait";
169 log.info("green; queued for review", {
170 repo: `${repo.ownerName}/${repo.name}`,
171 pull: pull.number,
172 reviewer: reviewer.user.username,
173 attempt: history.attempts.length + 1,
174 });
175 return "sent";
176 }
177
178 /**
179 * Every open, green pull an agent wrote that has no verdict on its head,
180 * in every repository with a reviewer on its crew, sent to review. The
181 * tick's net under the check event; returns how many went.
182 */
183 export async function sweepReviews(app: App): Promise<number> {
184 const repoIds = app.db
185 .query<{ repo_id: string }, []>(
186 "SELECT DISTINCT repo_id FROM repo_crew WHERE role = 'reviewer'",
187 )
188 .all()
189 .map((r) => r.repo_id);
190 let queued = 0;
191 for (const id of repoIds) {
192 const repo = getRepoById(app.db, id);
193 if (!repo) continue;
194 for (const pull of listPulls(app.db, repo.id, {
195 state: "open",
196 limit: 50,
197 })) {
198 if (!pull.authorIsAgent || !greenHead(app.db, pull)) continue;
199 if ((await sendToReview(app, repo, pull)) === "sent") queued++;
200 }
201 }
202 return queued;
203 }
src/crew/runner.ts +54 −2
@@ -36,7 +36,11 @@ import {
3636 type Repo,
3737 } from "../core/repos.ts";
3838 import { getUserById } from "../core/users.ts";
39 import { listReviewComments, listReviews } from "../core/reviews.ts";
39 import {
40 listReviewComments,
41 listReviews,
42 type Review,
43 } from "../core/reviews.ts";
4044 import {
4145 cancelRun,
4246 checkSummary,
@@ -199,6 +203,29 @@ export function wrapUpReserve(budgetUsd: number): number {
199203 );
200204 }
201205
206 /**
207 * How a review run ends, read from the verdicts on the pull: `ok` when the
208 * reviewer passed one since the run began, or stands by one on the head it
209 * was given (the same verdict sent again is the one already there);
210 * `failed` when it did neither. A reviewer pushes nothing by design, so
211 * what it pushed never decides it.
212 */
213 export function settleReview(
214 reviews: readonly Review[],
215 reviewerId: string,
216 since: string,
217 headOid: string,
218 ): { state: "ok" | "failed"; summary: string } {
219 const verdict = reviews.findLast(
220 (r) =>
221 r.authorId === reviewerId &&
222 (r.createdAt >= since || r.commitOid === headOid),
223 );
224 return verdict === undefined
225 ? { state: "failed", summary: "reviewed: no verdict was posted" }
226 : { state: "ok", summary: `reviewed: ${verdict.state.replace("_", " ")}` };
227 }
228
202229 /**
203230 * Claim and execute a queued agent run. Never throws. A run the provider
204231 * turned away ends cancelled with a `paused:` summary and returns when its
@@ -489,6 +516,7 @@ async function work(
489516 signal: AbortSignal,
490517 meter: Meter,
491518 ): Promise<Outcome> {
519 const began = new Date().toISOString();
492520 const repo = getRepoById(app.db, run.repoId);
493521 const issue =
494522 run.issueId === null ? undefined : getIssueById(app.db, run.issueId);
@@ -657,7 +685,12 @@ async function work(
657685 isApp: record !== undefined,
658686 previewUrl,
659687 policy,
660 resuming: previous?.sessionId !== undefined && previous.sessionId !== "",
688 // A review reads one head afresh: a resumed session would carry an
689 // older head's diff and preview into the verdict on this one.
690 resuming:
691 !reviewing &&
692 previous?.sessionId !== undefined &&
693 previous.sessionId !== "",
661694 role: crewRole(app.db, repo.id, agent.user.id),
662695 };
663696 const failedCheck =
@@ -990,6 +1023,25 @@ async function work(
9901023 };
9911024 }
9921025
1026 // A reviewer's work is its verdict, posted through the API; it pushes
1027 // nothing and has nothing to land. The run is judged by whether the
1028 // verdict is there, not by what the session said about itself.
1029 if (reviewing && pull !== undefined) {
1030 const settled = settleReview(
1031 listReviews(app.db, pull.id),
1032 agent.user.id,
1033 began,
1034 pull.headOid,
1035 );
1036 write(settled.summary);
1037 return {
1038 ...settled,
1039 sessionId: result.sessionId,
1040 costUsd: result.costUsd,
1041 turns: result.turns,
1042 };
1043 }
1044
9931045 // Bookkeeping the agent does not have to get right.
9941046 const landed = await landWork(
9951047 app,
src/http/api.ts +59 −2
@@ -63,6 +63,8 @@ import {
6363 refreshPullHead,
6464 resolveReviewThread,
6565 submitReview,
66 MAX_REVIEW_LINES,
67 type ReviewInput,
6668 } from "../actions/pulls.ts";
6769 import { search } from "../actions/search.ts";
6870 import {
@@ -555,6 +557,31 @@ function editFailure(error: { kind: string } & Record<string, unknown>) {
555557 * The files of an edit, out of a JSON body. `content: null` deletes; a
556558 * `base64` encoding is how a client sends bytes that are not text.
557559 */
560 /**
561 * How a line comment is anchored, said the same way by every refusal of
562 * one — the sentence a reviewer reads to send it right the second time.
563 */
564 const ANCHOR_FORMAT =
565 'An anchor is {"path": a file in the pull\'s diff, "line": its line number, "side": "new" (the head\'s numbering, for added or kept lines) or "old" (the base\'s, for removed lines)}; the line must be inside one of the diff\'s hunks: in `@@ -a,b +c,d @@` the hunk\'s first line is c on the new side and a on the old. GET …/pulls/N returns the diff.';
566
567 /** A review's line comments as the API was sent them; undefined when they are not a list of objects. */
568 function reviewLines(raw: unknown): ReviewInput[] | undefined {
569 if (raw === undefined || raw === null) return [];
570 if (!Array.isArray(raw) || raw.length > MAX_REVIEW_LINES) return undefined;
571 const out: ReviewInput[] = [];
572 for (const row of raw) {
573 if (typeof row !== "object" || row === null) return undefined;
574 const r = row as Record<string, unknown>;
575 out.push({
576 path: str(r.path),
577 line: Number(r.line),
578 side: r.side === "old" ? "old" : "new",
579 body: str(r.body),
580 });
581 }
582 return out;
583 }
584
558585 function editFilesFrom(raw: unknown): EditFile[] {
559586 if (!Array.isArray(raw)) return [];
560587 const files: EditFile[] = [];
@@ -2099,7 +2126,18 @@ export function apiRoutes(router: Router<Handler>): void {
20992126 side,
21002127 },
21012128 );
2102 if (!added.ok) return respondAccess(added.error, 422);
2129 if (!added.ok) {
2130 // A verdict sent here is the call this route most often gets wrong.
2131 if (added.error === "bad_anchor" && input.state !== undefined)
2132 return apiError(
2133 "bad_anchor",
2134 422,
2135 `this route takes one line comment; a verdict goes to …/review-states as {"state","body","comments"}. ${ANCHOR_FORMAT}`,
2136 );
2137 if (added.error === "bad_anchor")
2138 return apiError("bad_anchor", 422, ANCHOR_FORMAT);
2139 return respondAccess(added.error, 422);
2140 }
21032141 return json(added.value, 201);
21042142 },
21052143 );
@@ -2134,12 +2172,19 @@ export function apiRoutes(router: Router<Handler>): void {
21342172 422,
21352173 "a review is approved, changes_requested, or commented",
21362174 );
2175 const comments = reviewLines(input.comments);
2176 if (comments === undefined)
2177 return apiError(
2178 "invalid_comments",
2179 422,
2180 `comments is a list of {"path","line","side","body"}. ${ANCHOR_FORMAT}`,
2181 );
21372182 const submitted = await submitReview(
21382183 ctx.app,
21392184 ctx.auth.user,
21402185 found.repo,
21412186 found.pull,
2142 { state, body: str(input.body) },
2187 { state, body: str(input.body), comments },
21432188 );
21442189 if (!submitted.ok) {
21452190 if (submitted.error === "self_review")
@@ -2148,6 +2193,18 @@ export function apiRoutes(router: Router<Handler>): void {
21482193 422,
21492194 "you cannot approve your own pull request",
21502195 );
2196 if (submitted.error === "bad_anchor")
2197 return apiError(
2198 "bad_anchor",
2199 422,
2200 `nothing was posted: a comment's anchor is not in the diff. ${ANCHOR_FORMAT}`,
2201 );
2202 if (submitted.error === "too_many_lines")
2203 return apiError(
2204 "too_many_lines",
2205 422,
2206 `a review carries at most ${MAX_REVIEW_LINES} line comments`,
2207 );
21512208 return originAware(submitted.error, 422);
21522209 }
21532210 return json(submitted.value, 201);
src/web/pages/board.ts +28
@@ -21,6 +21,7 @@ import {
2121 } from "../../core/decisions.ts";
2222 import { appsRuntime } from "../../apps/runtime.ts";
2323 import { efficiencyForRepos } from "../../core/efficiency.ts";
24 import { reviewStats, type ReviewStats } from "../../core/review-stats.ts";
2425 import { standings, WINDOW_MS } from "../../core/standing.ts";
2526 import {
2627 listCredentials,
@@ -96,6 +97,26 @@ const KIND_WORDS: Record<DecisionKind, string> = {
9697
9798 const money = (n: number): string => `$${n.toFixed(2)}`;
9899
100 /** A wait, in the unit a person reads it in. */
101 function waitWords(ms: number): string {
102 const minutes = Math.round(ms / 60_000);
103 if (minutes < 60) return `${minutes}m`;
104 return `${Math.round(minutes / 60)}h`;
105 }
106
107 /** How review is keeping up on one app today, or null when there is nothing to say. */
108 function reviewWords(stats: ReviewStats): string | null {
109 const parts: string[] = [];
110 if (stats.awaiting > 0) parts.push(`${stats.awaiting} awaiting review`);
111 if (stats.latencyMs !== null)
112 parts.push(`${waitWords(stats.latencyMs)} from green to a verdict`);
113 if (stats.byReviewer.length > 0)
114 parts.push(
115 stats.byReviewer.map((r) => `${r.name} ${r.count}`).join(", "),
116 );
117 return parts.length === 0 ? null : parts.join(" · ");
118 }
119
99120 /** A whole percentage, for the width rules in `boardCss`. */
100121 const pct = (n: number): number =>
101122 Math.max(0, Math.min(100, Math.round(n * 100)));
@@ -643,6 +664,7 @@ function today(ctx: Ctx, scope: BoardScope): Html {
643664 limit: 100,
644665 }).filter((p) => p.mergedAt !== null && p.mergedAt >= since).length;
645666 const share = s.ceilingUsd > 0 ? s.spentTodayUsd / s.ceilingUsd : 0;
667 const review = reviewWords(reviewStats(db, repo.id, since));
646668 const note =
647669 s.paused === "ceiling"
648670 ? "at its ceiling until 00:00 UTC"
@@ -672,6 +694,12 @@ function today(ctx: Ctx, scope: BoardScope): Html {
672694 <span class="board-app-week"
673695 >${perShippedWords(week.get(repo.id))} this week</span
674696 >
697 ${when(
698 review !== null,
699 () => html`<span class="board-app-week board-app-review"
700 >${review}</span
701 >`,
702 )}
675703 <span class="board-spend"
676704 ><span class="board-fill" data-w="${pct(share)}"></span
677705 ></span>
test/board.test.ts +43 −1
@@ -21,7 +21,8 @@ import { recordDecision } from "../src/core/decisions.ts";
2121 import { recordLook } from "../src/core/looks.ts";
2222 import { getPull } from "../src/core/pulls.ts";
2323 import { updateRepoSettings, type Repo } from "../src/core/repos.ts";
24 import { claimRun, createRun } from "../src/core/runs.ts";
24 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
25 import { submitReview } from "../src/core/reviews.ts";
2526 import { createUser, getUserByName, type User } from "../src/core/users.ts";
2627 import { serve, type Servers } from "../src/http/server.ts";
2728
@@ -277,6 +278,47 @@ describe("the board", () => {
277278 expect(byOwner).not.toContain("on the cart");
278279 });
279280
281 test("review on an app's card: pulls awaiting it, the wait for a verdict, verdicts per reviewer", async () => {
282 const scout = getUserByName(app.db, "scout")!;
283 const branch = await commitToBranch(app, alice, notes, {
284 branch: "feature/review-me",
285 base: "main",
286 message: "Review me",
287 files: [{ path: "REVIEW.md", content: "review me\n" }],
288 });
289 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
290 const opened = await openPull(app, scout, notes, {
291 title: "Review me",
292 body: "",
293 baseBranch: "main",
294 headBranch: "feature/review-me",
295 });
296 if (!opened.ok) throw new Error(String(opened.error));
297 const check = createRun(app.db, {
298 repoId: notes.id,
299 kind: "check",
300 issueId: opened.value.id,
301 env: `pull-${opened.value.number}`,
302 commitOid: opened.value.headOid,
303 reason: "pull",
304 });
305 claimRun(app.db, check.id);
306 finishRun(app.db, check.id, { state: "ok", summary: "check passed" });
307 expect(await (await get("/board")).text()).toContain("1 awaiting review");
308
309 const juniper = await createAgent(app, alice, { username: "juniper" });
310 if (!juniper.ok) throw new Error(String(juniper.error));
311 submitReview(app.db, {
312 issueId: opened.value.id,
313 authorId: juniper.value.user.id,
314 state: "changes_requested",
315 commitOid: opened.value.headOid,
316 });
317 const html = await (await get("/board")).text();
318 expect(html).not.toContain("awaiting review");
319 expect(html).toMatch(/\d+m from green to a verdict · juniper 1/);
320 });
321
280322 test("the home's shelf opens the board", async () => {
281323 const home = await (
282324 await get("/", { cookie: `${cookie}; forge_mode=studio` })
test/crew.test.ts +51 −1
@@ -30,7 +30,8 @@ import {
3030 } from "../src/crew/dispatch.ts";
3131 import { workdirFor } from "../src/crew/runner.ts";
3232 import { startChecks, type ChecksHandle } from "../src/checks/service.ts";
33 import { getAgentByName } from "../src/core/agents.ts";
33 import { getAgentByName, setCrewRole } from "../src/core/agents.ts";
34 import { listReviews } from "../src/core/reviews.ts";
3435 import { getIssue, listComments } from "../src/core/issues.ts";
3536 import { listPulls } from "../src/core/pulls.ts";
3637 import { resolveOid } from "../src/git/merge.ts";
@@ -120,6 +121,12 @@ if grep -q DECLINE-ME "${promptFile}"; then
120121 echo '{"type":"result","subtype":"success","is_error":false,"result":"DECLINED: this is out of scope for me.","total_cost_usd":0.005,"num_turns":1,"session_id":"fake-2","terminal_reason":"completed"}'
121122 exit 0
122123 fi
124 if grep -q "You are the reviewer on this crew" "${promptFile}"; then
125 path=$(grep -o 'api/v1/repos/[^"\\]*/review-states' "${promptFile}" | head -1)
126 curl -sS -X POST -H "authorization: Bearer $FORGE_TOKEN" -H "content-type: application/json" "\${FORGE_URL%/}/$path" -d '{"state":"changes_requested","body":"Name the fence."}' > "${dataDir}/review.out"
127 echo '{"type":"result","subtype":"success","is_error":false,"result":"Reviewed.","total_cost_usd":0.004,"num_turns":1,"session_id":"fake-3","terminal_reason":"completed"}'
128 exit 0
129 fi
123130 if grep -q "The check on your pull" "${promptFile}"; then echo "daffodils" >> CREW.md; fi
124131 echo "work by the crew $(date +%s%N)" >> CREW.md
125132 git add -A && git commit -qm "crew work" && git push -q -u origin HEAD
@@ -596,4 +603,47 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Added CREW
596603 );
597604 expect(passed.summary).toMatch(/^check passed/);
598605 }, 120_000);
606
607 test("a reviewer's run that posts its verdict is ok, though it pushes nothing", async () => {
608 const made = await createAgent(app, alice, { username: "rev" });
609 if (!made.ok) throw new Error(String(made.error));
610 expect(addToCrew(app, alice, repo, "rev").ok).toBe(true);
611 setCrewRole(app.db, repo.id, made.value.user.id, "reviewer");
612 const branch = await commitToBranch(app, alice, repo, {
613 branch: "fence",
614 base: "main",
615 message: "A fence",
616 files: [{ path: "FENCE.md", content: "a fence\n" }],
617 });
618 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
619 const opened = await openPull(app, alice, repo, {
620 title: "A fence",
621 baseBranch: "main",
622 headBranch: "fence",
623 });
624 if (!opened.ok) throw new Error(String(opened.error));
625 const issue = getIssue(app.db, repo.id, opened.value.number)!;
626 expect(queueAgentRun(app, repo, issue, made.value, "review")).toBeDefined();
627
628 const run = await settled(
629 (r) => r.reason === "review" && r.issueId === issue.id,
630 );
631 expect(run.state).toBe("ok");
632 expect(run.summary).toBe("reviewed: changes requested");
633 expect(
634 listReviews(app.db, issue.id).map((r) => [r.authorId, r.state]),
635 ).toEqual([[made.value.user.id, "changes_requested"]]);
636 // Nothing pushed and nothing said on the pull: the verdict is the word.
637 expect(listComments(app.db, issue.id)).toHaveLength(0);
638
639 // A second pass reads afresh: no earlier session carries an older
640 // head into it, and the same verdict again is not a second review.
641 expect(queueAgentRun(app, repo, issue, made.value, "review")).toBeDefined();
642 const again = await settled(
643 (r) => r.reason === "review" && r.issueId === issue.id && r.id !== run.id,
644 );
645 expect(again.state).toBe("ok");
646 expect(readFileSync(argsFile, "utf8")).not.toContain("--resume");
647 expect(listReviews(app.db, issue.id)).toHaveLength(1);
648 }, 60_000);
599649 });
test/review-gate.test.ts +111 −2
@@ -13,11 +13,16 @@ import { openPull } from "../src/actions/pulls.ts";
1313 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
1414 import { setCrewRole } from "../src/core/agents.ts";
1515 import { getPull, type Pull } from "../src/core/pulls.ts";
16 import { submitReview } from "../src/core/reviews.ts";
16 import { listComments } from "../src/core/issues.ts";
17 import {
18 REVIEW_ROUNDS,
19 reviewSummary,
20 submitReview,
21 } from "../src/core/reviews.ts";
1722 import { getRepoById, type Repo } from "../src/core/repos.ts";
1823 import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts";
1924 import { createUser, getUserByName, type User } from "../src/core/users.ts";
20 import { landIfGreen } from "../src/crew/autonomy.ts";
25 import { landIfGreen, settleAgentRun } from "../src/crew/autonomy.ts";
2126
2227 describe("the review gate", () => {
2328 let dataDir: string;
@@ -158,6 +163,31 @@ describe("the review gate", () => {
158163 });
159164 }
160165
166 test("a reviewer asked twice who never answers does not hold the pull: it lands, and says so", async () => {
167 setCrewRole(app.db, repo.id, quill.id, "reviewer");
168 const pull = await greenPull(mason, "unanswered");
169 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
170 const ended = () => {
171 for (const run of reviewRuns(pull).filter((r) => r.state === "queued")) {
172 claimRun(app.db, run.id);
173 finishRun(app.db, run.id, {
174 state: "failed",
175 summary: "reviewed: no verdict was posted",
176 });
177 }
178 };
179 ended();
180 // Back with nothing: sent once more.
181 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
182 expect(reviewRuns(pull)).toHaveLength(2);
183 ended();
184 expect(await landIfGreen(app, fresh(), pull)).toBe(true);
185 expect(reviewRuns(pull)).toHaveLength(2);
186 expect(
187 listComments(app.db, pull.id).map((c) => c.body).join("\n"),
188 ).toContain("without a review");
189 });
190
161191 test("a change requested on the head holds the pull", async () => {
162192 setCrewRole(app.db, repo.id, quill.id, "reviewer");
163193 const pull = await greenPull(alice, "contested");
@@ -170,4 +200,83 @@ describe("the review gate", () => {
170200 expect(await landIfGreen(app, fresh(), pull)).toBe(false);
171201 expect(getPull(app.db, repo.id, pull.number)?.mergedAt).toBeNull();
172202 });
203
204 const askChanges = (pull: Pull, by: User) =>
205 submitReview(app.db, {
206 issueId: pull.id,
207 authorId: by.id,
208 state: "changes_requested",
209 commitOid: pull.headOid,
210 });
211
212 /** Push one more commit to the pull's branch and pass its check. */
213 async function pushTo(pull: Pull, name: string): Promise<Pull> {
214 const pushed = await commitToBranch(app, alice, repo, {
215 branch: pull.headBranch,
216 base: "main",
217 message: name,
218 files: [{ path: `${name}.ts`, content: `export const x = "${name}";\n` }],
219 });
220 if (!pushed.ok) throw new Error(JSON.stringify(pushed.error));
221 return passCheck(getPull(app.db, repo.id, pull.number)!);
222 }
223
224 test("a reviewer taken off the crew no longer holds the pulls it judged", async () => {
225 setCrewRole(app.db, repo.id, quill.id, "reviewer");
226 const pull = await greenPull(mason, "judged");
227 askChanges(pull, quill);
228 expect(reviewSummary(app.db, pull.id, pull.headOid).changesRequested).toBe(1);
229 setCrewRole(app.db, repo.id, quill.id, "builder");
230 expect(reviewSummary(app.db, pull.id, pull.headOid).changesRequested).toBe(0);
231 // A person's request is the person's, whatever their crew.
232 askChanges(pull, alice);
233 expect(reviewSummary(app.db, pull.id, pull.headOid).changesRequested).toBe(1);
234 });
235
236 test(`the reviewer gets ${REVIEW_ROUNDS} rounds of changes; a third is advice and the pull lands`, async () => {
237 setCrewRole(app.db, repo.id, quill.id, "reviewer");
238 let pull = await greenPull(mason, "rounds");
239 askChanges(pull, quill);
240 // Asked twice on one head is still one round.
241 askChanges(pull, quill);
242 pull = await pushTo(pull, "rounds-2");
243 askChanges(pull, quill);
244 expect(reviewSummary(app.db, pull.id, pull.headOid).changesRequested).toBe(1);
245 pull = await pushTo(pull, "rounds-3");
246 askChanges(pull, quill);
247 expect(reviewSummary(app.db, pull.id, pull.headOid).changesRequested).toBe(0);
248 approve(pull, alice);
249 expect(await landIfGreen(app, fresh(), pull)).toBe(true);
250 });
251
252 test("a builder's run that answers a change request without pushing parks the pull", async () => {
253 setCrewRole(app.db, repo.id, quill.id, "reviewer");
254 setCrewRole(app.db, repo.id, mason.id, "builder");
255 const pull = await greenPull(mason, "unmoved");
256 askChanges(pull, quill);
257 expect(
258 settleAgentRun(app, fresh(), {
259 issueId: pull.id,
260 state: "ok",
261 summary: "no changes pushed: I think it is fine",
262 reason: "feedback",
263 actorId: mason.id,
264 }),
265 ).toBe(true);
266 expect(getPull(app.db, repo.id, pull.number)?.state).toBe("closed");
267
268 // One that pushed is answering, and is left alone.
269 const moved = await greenPull(mason, "moved");
270 askChanges(moved, quill);
271 expect(
272 settleAgentRun(app, fresh(), {
273 issueId: moved.id,
274 state: "ok",
275 summary: `updated pull #${moved.number} (abc1234)`,
276 reason: "feedback",
277 actorId: mason.id,
278 }),
279 ).toBe(false);
280 expect(getPull(app.db, repo.id, moved.number)?.state).toBe("open");
281 });
173282 });
test/review-lines.test.ts added +165
@@ -0,0 +1,165 @@
1 /**
2 * One way to post a review: the verdict and its line comments in one call
3 * to `…/review-states`. Every anchor is checked before anything is written,
4 * and a refused anchor says how an anchor is written, so a reviewer can
5 * send the review right the second time. A verdict sent to the line-comment
6 * route — the call the reviewer brief used to make — is told where it goes.
7 */
8 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
9 import { mkdtemp, rm } from "node:fs/promises";
10 import { tmpdir } from "node:os";
11 import { join } from "node:path";
12 import { createApp, type App } from "../src/app.ts";
13 import { addToCrew, createAgent } from "../src/actions/crew.ts";
14 import { openPull } from "../src/actions/pulls.ts";
15 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
16 import { listEvents } from "../src/core/events.ts";
17 import { listReviewComments, listReviews } from "../src/core/reviews.ts";
18 import type { Pull } from "../src/core/pulls.ts";
19 import type { Repo } from "../src/core/repos.ts";
20 import { createToken } from "../src/core/tokens.ts";
21 import { createUser, getUserByName, type User } from "../src/core/users.ts";
22 import { serve, type Servers } from "../src/http/server.ts";
23
24 describe("a review with line comments", () => {
25 let dataDir: string;
26 let app: App;
27 let servers: Servers;
28 let alice: User;
29 let repo: Repo;
30 let pull: Pull;
31 let token: string;
32
33 const post = (path: string, payload: unknown) =>
34 fetch(`${servers.url}/api/v1/repos/alice/engine/pulls/${pull.number}/${path}`, {
35 method: "POST",
36 headers: {
37 authorization: `Bearer ${token}`,
38 "content-type": "application/json",
39 },
40 body: JSON.stringify(payload),
41 });
42
43 beforeAll(async () => {
44 dataDir = await mkdtemp(join(tmpdir(), "forge-review-lines-"));
45 app = createApp({ dataDir, port: 0 });
46 servers = await serve(app);
47 const a = await createUser(app.db, {
48 username: "alice",
49 email: "[email protected]",
50 password: "a-good-passphrase",
51 adminIfFirst: true,
52 });
53 if (!a.ok) throw new Error(a.error);
54 alice = a.value;
55 const made = await createRepo(app, alice, {
56 name: "engine",
57 visibility: "private",
58 });
59 if (!made.ok) throw new Error(String(made.error));
60 repo = made.value;
61 await commitToBranch(app, alice, repo, {
62 message: "Start",
63 files: [{ path: "a.ts", content: "one\ntwo\n" }],
64 });
65 await commitToBranch(app, alice, repo, {
66 branch: "feature/three",
67 base: "main",
68 message: "three",
69 files: [{ path: "a.ts", content: "one\ntwo\nthree\n" }],
70 });
71 const opened = await openPull(app, alice, repo, {
72 title: "three",
73 baseBranch: "main",
74 headBranch: "feature/three",
75 });
76 if (!opened.ok) throw new Error(String(opened.error));
77 pull = opened.value;
78 const agent = await createAgent(app, alice, { username: "juniper" });
79 if (!agent.ok) throw new Error(String(agent.error));
80 expect(addToCrew(app, alice, repo, "juniper").ok).toBe(true);
81 token = createToken(
82 app.db,
83 getUserByName(app.db, "juniper")!.id,
84 "run",
85 "rw",
86 ).token;
87 });
88
89 afterAll(async () => {
90 servers.stop();
91 app.close();
92 await rm(dataDir, { recursive: true, force: true });
93 });
94
95 test("a bad anchor posts nothing and says how an anchor is written", async () => {
96 const refused = await post("review-states", {
97 state: "changes_requested",
98 body: "two things",
99 comments: [
100 { path: "a.ts", line: 3, side: "new", body: "fine anchor" },
101 { path: "a.ts", line: 99, side: "new", body: "no such line" },
102 ],
103 });
104 expect(refused.status).toBe(422);
105 const said = (await refused.json()) as { error: string; message: string };
106 expect(said.error).toBe("bad_anchor");
107 expect(said.message).toContain("nothing was posted");
108 expect(said.message).toContain("@@ -a,b +c,d @@");
109 expect(listReviews(app.db, pull.id)).toHaveLength(0);
110 expect(listReviewComments(app.db, pull.id)).toHaveLength(0);
111 });
112
113 test("the verdict and its line comments land in one call", async () => {
114 const sent = await post("review-states", {
115 state: "changes_requested",
116 body: "one thing",
117 comments: [{ path: "a.ts", line: 3, side: "new", body: "say why" }],
118 });
119 expect(sent.status).toBe(201);
120 expect(listReviews(app.db, pull.id).map((r) => r.state)).toEqual([
121 "changes_requested",
122 ]);
123 const lines = listReviewComments(app.db, pull.id);
124 expect(lines.map((c) => [c.path, c.line, c.body])).toEqual([
125 ["a.ts", 3, "say why"],
126 ]);
127 // One event for the line, one for the verdict: only the second carries
128 // a state, and only that one wakes the builder.
129 const kinds = listEvents(app.db, { repoId: repo.id })
130 .filter((e) => e.kind === "pull.reviewed")
131 .map((e) => (typeof e.payload["state"] === "string" ? "verdict" : "line"));
132 expect(kinds.sort()).toEqual(["line", "verdict"]);
133 });
134
135 test("the same verdict sent twice on one head is one review", async () => {
136 const again = await post("review-states", {
137 state: "changes_requested",
138 body: "one thing",
139 comments: [{ path: "a.ts", line: 3, side: "new", body: "say why" }],
140 });
141 expect(again.status).toBe(201);
142 expect(listReviews(app.db, pull.id)).toHaveLength(1);
143 expect(listReviewComments(app.db, pull.id)).toHaveLength(1);
144 });
145
146 test("comments that are not a list of objects are refused", async () => {
147 const refused = await post("review-states", {
148 state: "commented",
149 comments: "a.ts:3 looks off",
150 });
151 expect(refused.status).toBe(422);
152 expect(((await refused.json()) as { error: string }).error).toBe(
153 "invalid_comments",
154 );
155 });
156
157 test("a verdict sent to the line-comment route is told where it goes", async () => {
158 const wrong = await post("reviews", { state: "approved", body: "ok" });
159 expect(wrong.status).toBe(422);
160 const said = (await wrong.json()) as { error: string; message: string };
161 expect(said.error).toBe("bad_anchor");
162 expect(said.message).toContain("review-states");
163 expect(said.message).toContain('"side"');
164 });
165 });
test/review-route.test.ts added +249
@@ -0,0 +1,249 @@
1 /**
2 * Who reviews a pull, and how a review run ends. A change to the
3 * security-bearing paths goes to the reviewer whose brief says it reads for
4 * security; everything else to the one who reads for the experience. Every
5 * green head an agent wrote is sent, in any repository with a reviewer —
6 * not only one that ships on its own. A review run is judged by its verdict:
7 * it pushes nothing by design.
8 */
9 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10 import { mkdtemp, rm } from "node:fs/promises";
11 import { tmpdir } from "node:os";
12 import { join } from "node:path";
13 import { createApp, type App } from "../src/app.ts";
14 import { addToCrew, createAgent } from "../src/actions/crew.ts";
15 import { openPull } from "../src/actions/pulls.ts";
16 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
17 import { setCrewRole, type Agent } from "../src/core/agents.ts";
18 import type { Pull } from "../src/core/pulls.ts";
19 import type { Repo } from "../src/core/repos.ts";
20 import type { Review } from "../src/core/reviews.ts";
21 import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts";
22 import { createUser, getUserByName, type User } from "../src/core/users.ts";
23 import {
24 pickReviewer,
25 REVIEW_LAPSE_MS,
26 REVIEW_WAIT_MS,
27 reviewStage,
28 sweepReviews,
29 touchesSecurity,
30 watchesSecurity,
31 } from "../src/crew/review.ts";
32 import { settleReview } from "../src/crew/runner.ts";
33
34 const SHARED = "How I work, always:\n- Security is not optional.";
35 const agent = (name: string, brief: string): Agent =>
36 ({ user: { username: name }, brief }) as unknown as Agent;
37 const juniper = agent(
38 "juniper",
39 `I am Juniper. I review the experience.\n\n${SHARED}`,
40 );
41 const sentinel = agent(
42 "sentinel",
43 `I am Sentinel. I read every changed line for security.\n\n${SHARED}`,
44 );
45
46 describe("which reviewer", () => {
47 test("security is read from what the agent says it is, not the rules every brief shares", () => {
48 expect(watchesSecurity(sentinel)).toBe(true);
49 expect(watchesSecurity(juniper)).toBe(false);
50 });
51
52 test("the security-bearing paths", () => {
53 for (const p of [
54 "src/http/api.ts",
55 "src/core/tokens.ts",
56 "src/crew/sandbox.ts",
57 "src/web/auth.ts",
58 "docs/SECRETS.md",
59 ])
60 expect(touchesSecurity([p])).toBe(true);
61 expect(touchesSecurity(["src/web/pages/board.ts", "README.md"])).toBe(
62 false,
63 );
64 });
65
66 test("a security change to sentinel, any other to juniper, and one reviewer takes all", () => {
67 const both = [juniper, sentinel];
68 expect(pickReviewer(both, ["src/core/tokens.ts"])).toBe(sentinel);
69 expect(pickReviewer(both, ["src/web/pages/board.ts"])).toBe(juniper);
70 expect(pickReviewer([juniper], ["src/http/api.ts"])).toBe(juniper);
71 expect(pickReviewer([sentinel], ["README.md"])).toBe(sentinel);
72 expect(pickReviewer([], ["README.md"])).toBeUndefined();
73 });
74 });
75
76 describe("a review that does not come back", () => {
77 const green = "2026-10-02T10:00:00.000Z";
78 const at = (ms: number) => new Date(Date.parse(green) + ms);
79 const sent = (ms: number, active: boolean) => ({
80 createdAt: at(ms).toISOString(),
81 active,
82 });
83
84 test("sent first, waited for while it is out, sent once more, then lapsed", () => {
85 expect(reviewStage(green, [], at(1_000))).toBe("send");
86 expect(reviewStage(green, [sent(0, true)], at(60_000))).toBe("wait");
87 // Out past the wait, or back with nothing: one more.
88 expect(reviewStage(green, [sent(0, true)], at(REVIEW_WAIT_MS))).toBe(
89 "send",
90 );
91 expect(reviewStage(green, [sent(0, false)], at(60_000))).toBe("send");
92 expect(
93 reviewStage(green, [sent(0, false), sent(60_000, true)], at(120_000)),
94 ).toBe("wait");
95 expect(
96 reviewStage(green, [sent(0, false), sent(60_000, false)], at(120_000)),
97 ).toBe("lapsed");
98 });
99
100 test("a head never sent — at the ceiling, say — still lapses, so nothing is held for ever", () => {
101 expect(reviewStage(green, [], at(REVIEW_LAPSE_MS))).toBe("lapsed");
102 });
103 });
104
105 describe("how a review run ends", () => {
106 const verdict = (
107 authorId: string,
108 createdAt: string,
109 commitOid = "old",
110 ): Review =>
111 ({ authorId, state: "changes_requested", createdAt, commitOid }) as Review;
112 const since = "2026-10-02T10:00:00.000Z";
113
114 test("a verdict posted during the run is ok, though nothing was pushed", () => {
115 expect(
116 settleReview(
117 [verdict("j", "2026-10-02T10:05:00.000Z")],
118 "j",
119 since,
120 "head",
121 ),
122 ).toEqual({ state: "ok", summary: "reviewed: changes requested" });
123 });
124
125 test("standing by its verdict on the head it was given is ok too", () => {
126 expect(
127 settleReview(
128 [verdict("j", "2026-10-02T09:00:00.000Z", "head")],
129 "j",
130 since,
131 "head",
132 ).state,
133 ).toBe("ok");
134 });
135
136 test("no verdict, an earlier one on another head, or someone else's is a failed run", () => {
137 for (const reviews of [
138 [],
139 [verdict("j", "2026-10-02T09:00:00.000Z")],
140 [verdict("x", "2026-10-02T10:05:00.000Z")],
141 ])
142 expect(settleReview(reviews, "j", since, "head").state).toBe("failed");
143 });
144 });
145
146 describe("every green pull an agent wrote is sent to review", () => {
147 let dataDir: string;
148 let app: App;
149 let alice: User;
150 let repo: Repo;
151
152 beforeAll(async () => {
153 dataDir = await mkdtemp(join(tmpdir(), "forge-review-route-"));
154 app = createApp({ dataDir, port: 0 });
155 const a = await createUser(app.db, {
156 username: "alice",
157 email: "[email protected]",
158 password: "a-good-passphrase",
159 adminIfFirst: true,
160 });
161 if (!a.ok) throw new Error(a.error);
162 alice = a.value;
163 const made = await createRepo(app, alice, {
164 name: "engine",
165 visibility: "private",
166 });
167 if (!made.ok) throw new Error(String(made.error));
168 repo = made.value;
169 await commitToBranch(app, alice, repo, {
170 message: "Start",
171 files: [{ path: "README.md", content: "# engine\n" }],
172 });
173 for (const [name, brief] of [
174 ["scout", "I am Scout. I ship small things."],
175 ["juniper", juniper.brief],
176 ["sentinel", sentinel.brief],
177 ] as const) {
178 const made = await createAgent(app, alice, { username: name, brief });
179 if (!made.ok) throw new Error(String(made.error));
180 expect(addToCrew(app, alice, repo, name).ok).toBe(true);
181 }
182 for (const name of ["juniper", "sentinel"])
183 setCrewRole(app.db, repo.id, getUserByName(app.db, name)!.id, "reviewer");
184 // Not autonomous: review is not the shipping loop's alone.
185 expect(repo.autonomy).not.toBe("autonomous");
186 });
187
188 afterAll(async () => {
189 await app.close?.();
190 await rm(dataDir, { recursive: true, force: true });
191 });
192
193 async function pullBy(author: User, name: string, path: string) {
194 await commitToBranch(app, alice, repo, {
195 branch: `feature/${name}`,
196 base: "main",
197 message: name,
198 files: [{ path, content: `${name}\n` }],
199 });
200 const opened = await openPull(app, alice, repo, {
201 title: name,
202 baseBranch: "main",
203 headBranch: `feature/${name}`,
204 });
205 if (!opened.ok) throw new Error(String(opened.error));
206 app.db
207 .query("UPDATE issue SET author_id = ?2 WHERE id = ?1")
208 .run(opened.value.id, author.id);
209 return opened.value;
210 }
211
212 function pass(pull: Pull): void {
213 const check = createRun(app.db, {
214 repoId: repo.id,
215 kind: "check",
216 issueId: pull.id,
217 env: `pull-${pull.number}`,
218 commitOid: pull.headOid,
219 reason: "pull",
220 });
221 claimRun(app.db, check.id);
222 finishRun(app.db, check.id, { state: "ok", summary: "check passed" });
223 }
224
225 const reviewer = (pull: Pull) =>
226 listRuns(app.db, repo.id, { issueId: pull.id })
227 .filter((r) => r.reason === "review")
228 .map((r) => getUserByName(app.db, "juniper")!.id === r.actorId ? "juniper" : "sentinel");
229
230 test("green pulls go to the right reviewer, once; red and people's pulls wait", async () => {
231 const scout = getUserByName(app.db, "scout")!;
232 const page = await pullBy(scout, "page", "src/web/page.ts");
233 const auth = await pullBy(scout, "auth", "src/http/auth.ts");
234 const red = await pullBy(scout, "red", "src/web/red.ts");
235 const person = await pullBy(alice, "mine", "src/web/mine.ts");
236 pass(page);
237 pass(auth);
238 pass(person);
239
240 expect(await sweepReviews(app)).toBe(2);
241 expect(reviewer(page)).toEqual(["juniper"]);
242 expect(reviewer(auth)).toEqual(["sentinel"]);
243 expect(reviewer(red)).toEqual([]);
244 expect(reviewer(person)).toEqual([]);
245
246 // The next tick sends nothing twice.
247 expect(await sweepReviews(app)).toBe(0);
248 });
249 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs