trevato/forge

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

More
Forks1g fMapg aRuns15g rEventsFlows

A box pass on an already merged pull reviews the merge's own diff, never “(no diff available)” #8766

Merged torquecrew opened this

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

ConversationFiles
Merged

torque/forge:torque/8739-a-box-pass-on-an-already-merged-pull-rev → main 2 commits fork

  • Merged. torque/8739-a-box-pass-on-an-already-merged-pull-rev was merged into main as 7d00478.

Review

Sign in to review this change.
7 files changed +195 −33
docs/features/box-pass.md +2
@@ -23,6 +23,8 @@ A crew reviewer's approval, `POST /api/v1/repos/:owner/:name/pulls/:n/review-sta
2323
2424 An open issue whose pull already landed with boxes unticked is sent to the crew reviewer **once**. The mark is a pass note by a crew reviewer on the issue, or a review run on the pull since it merged. That pull has landed, so its pass applies at once and closes the issue if every box is ticked.
2525
26 The reviewer reads what landed: the merge commit on the base (kept by `refs/pull/N/merged`) against its first parent, so the diff is real when the head's refs and objects are gone. A squash has one parent and is read the same way. A pull that closed as carried, whose recorded commit is not its own, is read from its head. When neither can be read, no reviewer is sent: the issue stays held, and one note from the crew reviewer says why (`No box pass for pull #N: …`).
27
2628 ## Who can do this where
2729
2830 The API is the reviewer's door, and its brief gives the curl. The MCP `review` tool and the terminal's review card do not take `ticks` and `left`: the pass is the crew reviewer's report, and a person who reads a change ticks the boxes on the issue itself, where they see the box they mean.
src/crew/autonomy.ts +1 −1
@@ -441,7 +441,7 @@ export async function driveRepo(
441441 let held = sorted();
442442 if (freeParked(app, repo, by, held) > 0) held = sorted();
443443 // An issue a landing left open with boxes is given the reviewer's pass once (#8682).
444 passLanded(app, repo, held.by.landed);
444 await passLanded(app, repo, held.by.landed);
445445 holdHot(
446446 repo.id,
447447 held.by.hot_file,
src/crew/box_pass.ts +32 −10
@@ -6,14 +6,17 @@
66 */
77
88 import type { App } from "../app.ts";
9 import { addComment } from "../actions/issues.ts";
910 import { crewRole } from "../core/agents.ts";
1011 import { PASS_LEAD, unticked } from "../core/done_on_main.ts";
1112 import { getIssue, getIssueById, type Issue } from "../core/issues.ts";
1213 import { closesFromBody, getPull, mergedPullClosing, type Pull } from "../core/pulls.ts";
1314 import { PILLAR_MAX, type Repo } from "../core/repos.ts";
15 import { repoDir } from "../git/repos.ts";
1416 import type { BoxList } from "./brief.ts";
1517 import { queueAgentRun } from "./dispatch.ts";
1618 import { pickReviewer, reviewersOf, reviewLoad, reviewsInFlight } from "./review.ts";
19 import { reviewRange } from "./review_range.ts";
1720
1821 /** Landed issues one tick sends to review: the rest wait for the next, never unseen. */
1922 export const PASS_SENDS = 3;
@@ -33,15 +36,27 @@ export function landedBoxes(issues: readonly Issue[]): BoxList[] {
3336 return issues.map((i) => ({ number: i.number, title: i.title, boxes: unticked(i.body) }));
3437 }
3538
36 /** Whether the pull that landed on `issue` already had its boxes read: a pass note, or a review run since it merged. */
37 export function passed(app: App, repo: Repo, issue: Issue, pull: Pull): boolean {
38 // The note counts only from a crew reviewer: anyone else's comment cannot suppress the pass.
39 const notes = app.db
39 /** What the note on an issue starts with when its pass cannot be given: no diff of the pull is left to read (#8739). */
40 export const NO_DIFF_LEAD = "No box pass for pull #";
41
42 /** The note on an issue whose landed pull has no diff left: the reviewer is not sent blind. */
43 export function noDiffNote(pull: number): string {
44 return `${NO_DIFF_LEAD}${pull}: neither its merge commit nor its head is in the repository, so there is no diff to read the boxes against and no reviewer was sent. The issue stays held; a person may tick what is done, or close it.`;
45 }
46
47 /** Whether a crew reviewer noted `lead` on `issue`: anyone else's comment cannot suppress the pass. */
48 function notedByReviewer(app: App, repo: Repo, issue: Issue, lead: string): boolean {
49 return app.db
4050 .query<{ author_id: string }, [string, string]>(
4151 "SELECT author_id FROM comment WHERE issue_id = ?1 AND body LIKE ?2",
4252 )
43 .all(issue.id, `${PASS_LEAD.replace(/[%_]/g, "")}%`);
44 if (notes.some((c) => crewRole(app.db, repo.id, c.author_id) === "reviewer")) return true;
53 .all(issue.id, `${lead.replace(/[%_]/g, "")}%`)
54 .some((c) => crewRole(app.db, repo.id, c.author_id) === "reviewer");
55 }
56
57 /** Whether the pull that landed on `issue` already had its boxes read: a pass note, or a review run since it merged. */
58 export function passed(app: App, repo: Repo, issue: Issue, pull: Pull): boolean {
59 if (notedByReviewer(app, repo, issue, PASS_LEAD)) return true;
4560 return (
4661 app.db
4762 .query<{ n: number }, [string, string]>(
@@ -55,13 +70,15 @@ export function passed(app: App, repo: Repo, issue: Issue, pull: Pull): boolean
5570 /**
5671 * Send each issue held under `landed` that was never given the pass to the
5772 * 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. 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.
73 * mark, so a tick later finds it sent. A pull with no diff left to read is
74 * not sent; its issue stays held and says why, once. Returns how many were
75 * sent. The reviewer's slots bound it as they bound `sendToReview` (#8734):
76 * a paused reviewer is sent nothing, and a full one waits for the next tick.
6177 */
62 export function passLanded(app: App, repo: Repo, held: readonly Issue[]): number {
78 export async function passLanded(app: App, repo: Repo, held: readonly Issue[]): Promise<number> {
6379 const reviewers = reviewersOf(app.db, repo.id);
6480 if (reviewers.length === 0) return 0;
81 const bare = repoDir(app.reposRoot, repo.ownerName, repo.name);
6582 const room =
6683 repo.reviewerSlots >= PILLAR_MAX
6784 ? PASS_SENDS
@@ -75,6 +92,11 @@ export function passLanded(app: App, repo: Repo, held: readonly Issue[]): number
7592 const asked = getIssueById(app.db, pull.id);
7693 const reviewer = pickReviewer(reviewers, [], undefined, reviewLoad(app.db));
7794 if (asked === undefined || reviewer === undefined) continue;
95 if ((await reviewRange(bare, pull)) === undefined) {
96 if (!notedByReviewer(app, repo, issue, `${NO_DIFF_LEAD}${pull.number}:`))
97 addComment(app, reviewer.user, repo, issue, noDiffNote(pull.number));
98 continue;
99 }
78100 if (queueAgentRun(app, repo, asked, reviewer, "review") !== undefined) sent++;
79101 }
80102 return sent;
src/crew/brief.ts +5 −1
@@ -247,6 +247,8 @@ export interface TaskInput extends BriefInput {
247247 | undefined;
248248 /** For `review`: the pull's diff against its base, capped. */
249249 readonly reviewDiff?: string | undefined;
250 /** For `review` of a pull that landed: the merge commit `reviewDiff` was read from (#8739). */
251 readonly landedAs?: string | undefined;
250252 /** For `review`: the change touches `src/web/`, so the review is held to docs/DESIGN.md. */
251253 readonly design?: boolean | undefined;
252254 /** For `review`: an earlier review run on this pull ended with no verdict. */
@@ -481,7 +483,9 @@ function taskBody(input: TaskInput): string {
481483 `"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.`,
482484 `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.`,
483485 "",
484 "The change:",
486 input.landedAs === undefined
487 ? "The change:"
488 : `The change, as it landed: commit ${input.landedAs} on the base against its first parent. This pull is merged already, so judge what landed; your verdict carries the box pass.`,
485489 "```diff",
486490 input.reviewDiff ?? "(no diff available)",
487491 "```",
src/crew/review_range.ts added +41
@@ -0,0 +1,41 @@
1 /**
2 * The two commits a reviewer's diff runs between (#8739). An open pull is
3 * read as the web shows it: its head against the merge base with the live
4 * base branch. A pull that landed is read as it landed: its merge commit
5 * against that commit's first parent, the base it landed on — the commit
6 * `refs/pull/N/merged` keeps reachable when the head's objects are gone.
7 * A squash has one parent and a merge commit's first is the base, so the
8 * same range serves both.
9 *
10 * A landing whose first parent is not the base the pull recorded is not
11 * this pull's own commit (a pull that closed as carried, #2036, records
12 * the commit it found); it is read from its head like an open one.
13 */
14
15 import { runGit } from "../git/env.ts";
16 import { isSafeOid } from "../git/guard.ts";
17 import { mergeBase, resolveOid } from "../git/merge.ts";
18 import type { Pull } from "../core/pulls.ts";
19
20 export interface ReviewRange {
21 readonly from: string;
22 readonly to: string;
23 /** The range is the pull's own merge commit on the base. */
24 readonly landed: boolean;
25 }
26
27 /** The range a reviewer reads for `pull` in the repository at `bare`, or undefined when none can be produced. */
28 export async function reviewRange(bare: string, pull: Pull): Promise<ReviewRange | undefined> {
29 const merged = pull.mergedAt === null ? null : pull.mergedOid;
30 if (merged !== null && isSafeOid(merged)) {
31 const parent = await runGit(["rev-parse", "--verify", "--quiet", `${merged}^1^{commit}`], { cwd: bare });
32 const from = parent.ok ? parent.value.trim() : "";
33 if (from !== "" && from === pull.baseOid) return { from, to: merged, landed: true };
34 }
35 if (!isSafeOid(pull.headOid)) return undefined;
36 const head = await resolveOid(bare, pull.headOid);
37 const base = await resolveOid(bare, `refs/heads/${pull.baseBranch}`);
38 if (head === undefined || base === undefined) return undefined;
39 const from = await mergeBase(bare, base, head);
40 return from === undefined ? undefined : { from, to: head, landed: false };
41 }
src/crew/runner.ts +22 −11
@@ -132,6 +132,7 @@ import { laterGate } from "./later.ts";
132132 import { handedLimbs } from "./handed_limbs.ts";
133133 import { parkedTwice } from "./parks.ts";
134134 import { boxesOf, landedBoxes } from "./box_pass.ts";
135 import { reviewRange } from "./review_range.ts";
135136 import { heldNow, stuckOf } from "./held.ts";
136137 import { reserveOf } from "./reserve.ts";
137138 import { owed } from "./team_share.ts";
@@ -235,32 +236,37 @@ export function steerRun(app: App, runId: string, text: string): boolean {
235236 return send(text);
236237 }
237238
238 /** What a reviewer reads: the pull's diff against its base, and its pictures. */
239 /**
240 * What a reviewer reads: the pull's diff (`review_range.ts` — a landed pull's
241 * is its merge commit's), and its pictures. `landedAs` names the merge commit
242 * the diff was read from, when it was.
243 */
239244 export async function reviewInputs(
240245 app: App,
241246 repo: Repo,
242247 pull: Pull,
243248 record: AppRecord | undefined,
244249 bare: string,
245 ): Promise<{ reviewDiff: string; looks: string[]; design: boolean }> {
246 const base = (await resolveOid(bare, `refs/heads/${pull.baseBranch}`)) ?? "";
250 ): Promise<{ reviewDiff: string; looks: string[]; design: boolean; landedAs?: string }> {
251 const range = await reviewRange(bare, pull);
247252 let reviewDiff = "(no diff available)";
248253 let design = false;
249 if (base !== "") {
254 if (range !== undefined) {
250255 const names = await runGitCapture(
251 ["diff", "--name-only", `${base}...${pull.headOid}`],
256 ["diff", "--name-only", range.from, range.to],
252257 { cwd: bare },
253258 );
254259 design = touchesDesign(names.stdout.toString().split("\n").filter((n) => n !== ""));
255260 const out = await runGitCapture(
256 ["diff", "--stat", "-p", `${base}...${pull.headOid}`],
261 ["diff", "--stat", "-p", range.from, range.to],
257262 { cwd: bare },
258263 );
259264 const text = out.stdout.toString();
260 reviewDiff =
261 text.length > REVIEW_DIFF_MAX
262 ? `${text.slice(0, REVIEW_DIFF_MAX)}\n… (cut at ${REVIEW_DIFF_MAX} bytes)`
263 : text;
265 if (out.code === 0)
266 reviewDiff =
267 text.length > REVIEW_DIFF_MAX
268 ? `${text.slice(0, REVIEW_DIFF_MAX)}\n… (cut at ${REVIEW_DIFF_MAX} bytes)`
269 : text;
264270 }
265271 const looks =
266272 record === undefined
@@ -279,7 +285,12 @@ export async function reviewInputs(
279285 .map((l) =>
280286 l.path.startsWith("/") ? l.path : join(app.config.dataDir, l.path),
281287 );
282 return { reviewDiff, looks, design };
288 return {
289 reviewDiff,
290 looks,
291 design,
292 ...(range?.landed === true && reviewDiff !== "(no diff available)" ? { landedAs: range.to } : {}),
293 };
283294 }
284295
285296 const REVIEW_DIFF_MAX = 60 * 1024;
test/box-pass.test.ts +92 −10
@@ -20,8 +20,10 @@ import { addComment, getIssueById, listComments, updateIssueText, type Issue } f
2020 import { getPull, type Pull } from "../src/core/pulls.ts";
2121 import { getRepoById, type Repo } from "../src/core/repos.ts";
2222 import { createUser, getUserByName, type User } from "../src/core/users.ts";
23 import { landedBoxes, passed, passLanded } from "../src/crew/box_pass.ts";
23 import { landedBoxes, NO_DIFF_LEAD, passed, passLanded } from "../src/crew/box_pass.ts";
2424 import { reviewsInFlight } from "../src/crew/review.ts";
25 import { reviewInputs } from "../src/crew/runner.ts";
26 import { repoDir } from "../src/git/repos.ts";
2527 import { taskPrompt, type TaskInput } from "../src/crew/brief.ts";
2628 import { serve, type Servers } from "../src/http/server.ts";
2729
@@ -77,8 +79,8 @@ describe("the reviewer's pass over the boxes", () => {
7779 if (!opened.ok) throw new Error(String(opened.error));
7880 return getPull(app.db, repo.id, opened.value.number)!;
7981 };
80 const land = async (pull: Pull) => {
81 const merged = await mergePull(app, alice, fresh(), pull, "squash");
82 const land = async (pull: Pull, strategy: "squash" | "merge" = "squash") => {
83 const merged = await mergePull(app, alice, fresh(), pull, strategy);
8284 if (!merged.ok) throw new Error(String(merged.error));
8385 };
8486 const approve = async (as: User, pull: Pull, pass: Parameters<typeof passOf>) => {
@@ -188,9 +190,9 @@ describe("the reviewer's pass over the boxes", () => {
188190 expect(held(old)).toBe(true);
189191 const asked = () =>
190192 app.db.query<{ n: number }, [string]>("SELECT COUNT(*) AS n FROM run WHERE issue_id = ?1 AND reason = 'review'").get(pull.id)!.n;
191 expect(passLanded(app, fresh(), heldNow(app, fresh()).by.landed.filter((i) => i.id === old.id))).toBe(1);
193 expect(await passLanded(app, fresh(), heldNow(app, fresh()).by.landed.filter((i) => i.id === old.id))).toBe(1);
192194 expect(asked()).toBe(1);
193 expect(passLanded(app, fresh(), heldNow(app, fresh()).by.landed.filter((i) => i.id === old.id))).toBe(0);
195 expect(await passLanded(app, fresh(), heldNow(app, fresh()).by.landed.filter((i) => i.id === old.id))).toBe(0);
194196 expect(asked()).toBe(1);
195197 await approve(vesper, pull, [[tick(old, 1, "x"), tick(old, 2, "y")], undefined]);
196198 expect(now(old).state).toBe("closed");
@@ -247,9 +249,89 @@ describe("the reviewer's pass over the boxes", () => {
247249 await land(pull);
248250 const landed = getPull(app.db, repo.id, pull.number)!;
249251 expect(passed(app, fresh(), asked, landed)).toBe(false);
250 expect(passLanded(app, fresh(), [now(asked)])).toBe(1);
252 expect(await passLanded(app, fresh(), [now(asked)])).toBe(1);
251253 expect(passed(app, fresh(), asked, landed)).toBe(true);
252254 });
255 // #8739: the head's refs are deleted and its objects pruned, as a forge's gc would leave it.
256 const prune = async (pull: Pull) => {
257 const bare = repoDir(app.reposRoot, repo.ownerName, repo.name);
258 for (const args of [
259 ["update-ref", "-d", `refs/heads/${pull.headBranch}`],
260 ["update-ref", "-d", `refs/pull/${pull.number}/head`],
261 ["reflog", "expire", "--expire=now", "--all"],
262 ["gc", "--prune=now", "--quiet"],
263 ]) {
264 const ran = Bun.spawnSync(["git", ...args], { cwd: bare });
265 if (ran.exitCode !== 0) throw new Error(ran.stderr.toString());
266 }
267 const gone = Bun.spawnSync(["git", "cat-file", "-e", `${pull.headOid}^{commit}`], { cwd: bare });
268 expect(gone.exitCode).not.toBe(0);
269 return bare;
270 };
271
272 test("a pull merged with its head gone is reviewed from its merge commit, a real diff in the brief", async () => {
273 const gone = issue("Gone", "- [ ] g");
274 const pull = await propose("gone", gone);
275 await land(pull);
276 const landed = getPull(app.db, repo.id, pull.number)!;
277 const bare = await prune(landed);
278 expect(await passLanded(app, fresh(), [now(gone)])).toBe(1);
279 const inputs = await reviewInputs(app, fresh(), landed, undefined, bare);
280 expect(inputs.landedAs).toBe(landed.mergedOid!);
281 expect(inputs.reviewDiff).toContain("gone.txt");
282 expect(inputs.reviewDiff).toContain("+x");
283 expect(inputs.reviewDiff).not.toContain("README.md");
284 const brief = taskPrompt({
285 agent: { user: { username: "vesper", displayName: "" }, brief: "" },
286 repo: { id: repo.id, ownerName: "alice", name: "engine", defaultBranch: "main", autonomyDirection: "Keep it small." },
287 issue: { number: pull.number, title: "gone", body: `Closes #${gone.number}`, authorName: "alice" },
288 comments: [],
289 reviews: [],
290 reason: "review",
291 branch: "gone",
292 forgeUrl: "http://forge.test",
293 role: "reviewer",
294 ...inputs,
295 } as unknown as TaskInput);
296 expect(brief).toContain(`The change, as it landed: commit ${landed.mergedOid}`);
297 expect(brief).toContain("+++ b/gone.txt");
298 expect(brief).not.toContain("(no diff available)");
299 });
300
301 test("a merge commit is read against its first parent: the pull's change, not the base's", async () => {
302 const both = issue("Both", "- [ ] b");
303 const pull = await propose("both", both);
304 const moved = await commitToBranch(app, alice, fresh(), {
305 message: "base moves",
306 files: [{ path: "base-moved.txt", content: "z\n" }],
307 });
308 if (!moved.ok) throw new Error(JSON.stringify(moved.error));
309 await land(pull, "merge");
310 const landed = getPull(app.db, repo.id, pull.number)!;
311 // Its head is its second parent, so it never goes; the base moved under it.
312 const bare = repoDir(app.reposRoot, repo.ownerName, repo.name);
313 const inputs = await reviewInputs(app, fresh(), landed, undefined, bare);
314 expect(inputs.landedAs).toBe(landed.mergedOid!);
315 expect(inputs.reviewDiff).toContain("both.txt");
316 expect(inputs.reviewDiff).not.toContain("base-moved.txt");
317 });
318
319 test("a pull with no diff left is not sent: its issue stays held and says why, once", async () => {
320 const lost = issue("Lost", "- [ ] l");
321 const pull = await propose("lost", lost);
322 await land(pull);
323 await prune(getPull(app.db, repo.id, pull.number)!);
324 // A landing whose commit is not there to read either.
325 app.db.query("UPDATE pull SET merged_oid = ?1 WHERE issue_id = ?2").run("0".repeat(40), pull.id);
326 const asked = () =>
327 app.db.query<{ n: number }, [string]>("SELECT COUNT(*) AS n FROM run WHERE issue_id = ?1 AND reason = 'review'").get(pull.id)!.n;
328 expect(await passLanded(app, fresh(), [now(lost)])).toBe(0);
329 expect(await passLanded(app, fresh(), [now(lost)])).toBe(0);
330 expect(asked()).toBe(0);
331 const notes = listComments(app.db, lost.id).filter((c) => c.body.startsWith(NO_DIFF_LEAD));
332 expect(notes.map((c) => [c.body.slice(0, 40), c.authorId])).toEqual([[`${NO_DIFF_LEAD}${pull.number}: neither its merge commit`.slice(0, 40), vesper.id]]);
333 expect(held(lost)).toBe(true);
334 });
253335
254336 test("the reviewer's slots bound the pass: paused sends none, a full seat waits (#8734)", async () => {
255337 const first = issue("Capped one", "- [ ] a");
@@ -260,13 +342,13 @@ describe("the reviewer's pass over the boxes", () => {
260342 const inFlight = () => reviewsInFlight(app.db, repo.id);
261343 app.db.query("UPDATE run SET state = 'cancelled' WHERE repo_id = ?1 AND state IN ('queued','running')").run(repo.id);
262344 slots(0);
263 expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(0);
345 expect(await passLanded(app, fresh(), [now(first), now(second)])).toBe(0);
264346 slots(1);
265 expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(1);
347 expect(await passLanded(app, fresh(), [now(first), now(second)])).toBe(1);
266348 expect(inFlight()).toBe(1);
267 expect(passLanded(app, fresh(), [now(first), now(second)])).toBe(0);
349 expect(await passLanded(app, fresh(), [now(first), now(second)])).toBe(0);
268350 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);
351 expect(await passLanded(app, fresh(), [now(first), now(second)])).toBe(1);
270352 slots(8);
271353 });
272354 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs