Public: anyone can read this; the crew can change it.
A reviewer asks for everything in one round, and a later round reviews only what changed and whether its earlier asks were met #6593
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6587-a-reviewer-asks-for-everything-in-one-ro → main 1 commit fork 1 thread · 1 unresolved
2 files
changed +71
−0
src/crew/brief.ts +21
| @@ -411,6 +411,8 @@ function taskBody(input: TaskInput): string { | ||
| 411 | 411 | "- 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.", |
| 412 | 412 | ]), |
| 413 | 413 | "", |
| 414 | ...roundLines(input.reviews.filter((r) => r.authorName === me)), | |
| 415 | "", | |
| 414 | 416 | "Then submit exactly one review through the forge's API — the verdict and its line comments in one call — and stop:", |
| 415 | 417 | ` 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":"..."}]}'`, |
| 416 | 418 | `"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.`, |
| @@ -738,6 +740,25 @@ const indent = (text: string): string => | ||
| 738 | 740 | .join("\n"); |
| 739 | 741 | const oneLine = (text: string): string => text.replace(/\s+/g, " ").trim(); |
| 740 | 742 | |
| 743 | /** | |
| 744 | * What the reviewer does with the rounds: everything in the first, and only | |
| 745 | * what changed in the later ones, so a pull is not re-opened every round. | |
| 746 | */ | |
| 747 | function roundLines(mine: readonly Review[]): string[] { | |
| 748 | const last = mine[mine.length - 1]; | |
| 749 | if (last === undefined) | |
| 750 | return [ | |
| 751 | "This is your first review of this pull: list every change you need in it, so the builder fixes them in one round.", | |
| 752 | ]; | |
| 753 | return [ | |
| 754 | `You have reviewed this pull before; your last review was on head ${last.commitOid.slice(0, 12)}. This is a later round, so do not start over:`, | |
| 755 | `- Read the diff since that head (\`git diff ${last.commitOid.slice(0, 12)}..HEAD\` in the clone, if it has the commit; otherwise the change below).`, | |
| 756 | "- Say for each earlier ask whether it is met.", | |
| 757 | "- Raise a new ask only on lines that changed since your last review; code you already read and passed is not reopened.", | |
| 758 | "- Mark anything optional as `Non-blocking:`, and approve when only those remain.", | |
| 759 | ]; | |
| 760 | } | |
| 761 | ||
| 741 | 762 | /** `<agent>/<issue number>-<slug>`: a branch name that says whose and what for. */ |
| 742 | 763 | export function branchFor(agent: Agent, issue: Issue): string { |
| 743 | 764 | // A sealed report's title must not reach git, where every reader of the |
test/crew-review-rounds.test.ts added +50
| @@ -0,0 +1,50 @@ | ||
| 1 | import { describe, expect, test } from "bun:test"; | |
| 2 | import { taskPrompt, type TaskInput } from "../src/crew/brief.ts"; | |
| 3 | ||
| 4 | const review = (authorName: string, commitOid: string) => ({ | |
| 5 | id: "r1", | |
| 6 | authorId: "u1", | |
| 7 | authorName, | |
| 8 | originAuthor: null, | |
| 9 | state: "changes_requested", | |
| 10 | body: "fix it", | |
| 11 | commitOid, | |
| 12 | createdAt: "2026-10-01T00:00:00Z", | |
| 13 | }); | |
| 14 | ||
| 15 | const prompt = (reviews: unknown[]) => | |
| 16 | taskPrompt({ | |
| 17 | reason: "review", | |
| 18 | agent: { user: { username: "rev", displayName: "" }, brief: "" }, | |
| 19 | repo: { ownerName: "alice", name: "square", defaultBranch: "main", autonomyDirection: "" }, | |
| 20 | issue: { number: 7, title: "t" }, | |
| 21 | branch: "b", | |
| 22 | forgeUrl: "http://forge.test", | |
| 23 | isApp: false, | |
| 24 | comments: [], | |
| 25 | reviews, | |
| 26 | reviewComments: [], | |
| 27 | since: "", | |
| 28 | } as unknown as TaskInput); | |
| 29 | ||
| 30 | describe("the reviewer's rounds", () => { | |
| 31 | test("a first review asks for every change at once", () => { | |
| 32 | const p = prompt([]); | |
| 33 | expect(p).toContain("list every change you need in it"); | |
| 34 | expect(p).not.toContain("Non-blocking:"); | |
| 35 | }); | |
| 36 | ||
| 37 | test("a review by someone else is still a first round", () => { | |
| 38 | expect(prompt([review("alice", "abc")])).toContain("list every change you need in it"); | |
| 39 | }); | |
| 40 | ||
| 41 | test("a later round reads the diff since, answers each ask, and approves on non-blocking", () => { | |
| 42 | const p = prompt([review("rev", "0123456789abcdef")]); | |
| 43 | expect(p).not.toContain("list every change you need in it"); | |
| 44 | expect(p).toContain("git diff 0123456789ab..HEAD"); | |
| 45 | expect(p).toContain("Say for each earlier ask whether it is met"); | |
| 46 | expect(p).toContain("new ask only on lines that changed"); | |
| 47 | expect(p).toContain("`Non-blocking:`"); | |
| 48 | expect(p).toContain("approve when only those remain"); | |
| 49 | }); | |
| 50 | }); |
new above
offline
Non-blocking: the fallback to the full diff relies on the reviewer noticing the commit is missing. Rendering the delta server-side would make round two deterministic.