Public: anyone can read this; the crew can change it.
The hand-over note after three review rounds quotes each review's asks in full, up to 600 characters #6604
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6586-the-hand-over-note-after-three-review-ro → main 2 commits fork 1 thread · 1 unresolved
2 files
changed +149
−9
src/crew/guard.ts +102 −7
| @@ -261,7 +261,14 @@ export const HANDOVER_NOTE = "Handed to a person by the crew:"; | ||
| 261 | 261 | |
| 262 | 262 | export interface Handover { |
| 263 | 263 | /** The crew reviewer's verdicts that asked for changes, oldest first. */ |
| 264 | readonly reviews: readonly { id: string; author: string; body: string; at: string }[]; | |
| 264 | readonly reviews: readonly { | |
| 265 | id: string; | |
| 266 | author: string; | |
| 267 | body: string; | |
| 268 | at: string; | |
| 269 | /** The line comments posted with it, oldest first. */ | |
| 270 | lines?: readonly { path: string; line: number; body: string }[]; | |
| 271 | }[]; | |
| 265 | 272 | /** When the third one landed: the card's age. */ |
| 266 | 273 | readonly at: string; |
| 267 | 274 | } |
| @@ -390,11 +397,99 @@ function handoversSince( | ||
| 390 | 397 | if (list) list.push(round); |
| 391 | 398 | else rounds.set(issueId, [round]); |
| 392 | 399 | } |
| 393 | for (const [id, list] of rounds) | |
| 394 | if (list.length >= HANDOVER_ROUNDS) out.set(id, { reviews: list, at: list[HANDOVER_ROUNDS - 1]!.at }); | |
| 400 | const handed = [...rounds].filter(([, list]) => list.length >= HANDOVER_ROUNDS); | |
| 401 | const lines = linesOf(db, handed.map(([id]) => id)); | |
| 402 | for (const [id, list] of handed) | |
| 403 | out.set(id, { | |
| 404 | reviews: withLines(lines.get(id) ?? [], list.slice(0, HANDOVER_ROUNDS), since.get(id)!), | |
| 405 | at: list[HANDOVER_ROUNDS - 1]!.at, | |
| 406 | }); | |
| 395 | 407 | return out; |
| 396 | 408 | } |
| 397 | 409 | |
| 410 | /** | |
| 411 | * Each review with the line comments its author posted just before it: lines | |
| 412 | * go in ahead of the verdict, so a review owns its author's lines since that | |
| 413 | * author's previous review (or since the thread was last reset). | |
| 414 | */ | |
| 415 | function withLines( | |
| 416 | rows: readonly LineRow[], | |
| 417 | reviews: readonly Handover["reviews"][number][], | |
| 418 | since: string, | |
| 419 | ): Handover["reviews"] { | |
| 420 | const last = new Map<string, string>(); | |
| 421 | return reviews.map((r) => { | |
| 422 | const from = last.get(r.author) ?? since; | |
| 423 | last.set(r.author, r.at); | |
| 424 | const lines = rows | |
| 425 | .filter((c) => c.author === r.author && c.at > from && c.at <= r.at) | |
| 426 | .map(({ path, line, body }) => ({ path, line, body })); | |
| 427 | return { ...r, lines }; | |
| 428 | }); | |
| 429 | } | |
| 430 | ||
| 431 | interface LineRow { | |
| 432 | pull: string; | |
| 433 | author: string; | |
| 434 | path: string; | |
| 435 | line: number; | |
| 436 | body: string; | |
| 437 | at: string; | |
| 438 | } | |
| 439 | ||
| 440 | /** The line comments of the given pulls in one read, by pull, oldest first. */ | |
| 441 | function linesOf(db: Database, ids: readonly string[]): Map<string, LineRow[]> { | |
| 442 | const out = new Map<string, LineRow[]>(); | |
| 443 | if (ids.length === 0) return out; | |
| 444 | for (const row of db | |
| 445 | .query<LineRow, string[]>( | |
| 446 | `SELECT c.issue_id AS pull, u.username AS author, c.path, c.line, c.body, c.created_at AS at | |
| 447 | FROM comment c JOIN user u ON u.id = c.author_id | |
| 448 | WHERE c.issue_id IN (${ids.map((_, n) => `?${n + 1}`).join(",")}) | |
| 449 | AND c.kind = 'review' AND c.origin_author IS NULL | |
| 450 | ORDER BY c.created_at, c.id`, | |
| 451 | ) | |
| 452 | .all(...ids)) { | |
| 453 | const list = out.get(row.pull); | |
| 454 | if (list) list.push(row); | |
| 455 | else out.set(row.pull, [row]); | |
| 456 | } | |
| 457 | return out; | |
| 458 | } | |
| 459 | ||
| 460 | /** What a review's quote may run to, and the window before it where a sentence end is preferred. */ | |
| 461 | const QUOTE_MAX = 600; | |
| 462 | const QUOTE_WINDOW = 100; | |
| 463 | const LINE_MAX = 200; | |
| 464 | ||
| 465 | /** `text` cut to `QUOTE_MAX`, at a sentence end when one falls in the last `QUOTE_WINDOW`. */ | |
| 466 | export function quoteCut(text: string): string { | |
| 467 | const t = text.trim(); | |
| 468 | if (t.length <= QUOTE_MAX) return t; | |
| 469 | const head = t.slice(0, QUOTE_MAX); | |
| 470 | let end = -1; | |
| 471 | for (const m of head.matchAll(/[.!?](?=\s|$)/g)) | |
| 472 | if (m.index! >= QUOTE_MAX - QUOTE_WINDOW) end = m.index! + 1; | |
| 473 | return end > 0 ? head.slice(0, end) : `${head.trimEnd()}…`; | |
| 474 | } | |
| 475 | ||
| 476 | const quoted = (text: string): string[] => | |
| 477 | text.split("\n").map((l) => (l.trim() === "" ? ">" : `> ${l.replace(/\s+$/, "")}`)); | |
| 478 | ||
| 479 | /** The note's body: each review quoted as a markdown blockquote, its line comments listed under it. */ | |
| 480 | function handoverQuotes(h: Handover): string[] { | |
| 481 | return h.reviews.slice(0, HANDOVER_ROUNDS).flatMap((r, n) => [ | |
| 482 | `${n + 1}. ${r.author}:`, | |
| 483 | "", | |
| 484 | ...quoted(quoteCut(r.body) || "no words"), | |
| 485 | ...(r.lines ?? []).flatMap((l) => [ | |
| 486 | "", | |
| 487 | `- ${l.path.replace(/\s+/g, " ")}:${l.line}: ${l.body.replace(/\s+/g, " ").trim().slice(0, LINE_MAX)}`, | |
| 488 | ]), | |
| 489 | "", | |
| 490 | ]); | |
| 491 | } | |
| 492 | ||
| 398 | 493 | /** One line per review, for the card and the thread: who asked, and the first line of what. */ |
| 399 | 494 | export function handoverLines(h: Handover): string[] { |
| 400 | 495 | return h.reviews.slice(0, HANDOVER_ROUNDS).map((r, n) => { |
| @@ -450,11 +545,11 @@ export function handoverNote(h: Handover, toPlanner = false): string { | ||
| 450 | 545 | return [ |
| 451 | 546 | `${HANDOVER_NOTE} the reviewer asked for changes three times with no approval between, so the planner re-scopes it: split, replaced, or one more round with the reviewer's ask restated.`, |
| 452 | 547 | "", |
| 453 | ...handoverLines(h), | |
| 454 | ].join("\n"); | |
| 548 | ...handoverQuotes(h), | |
| 549 | ].join("\n").trimEnd(); | |
| 455 | 550 | return [ |
| 456 | 551 | `${HANDOVER_NOTE} the reviewer asked for changes three times with no approval between, so no more feedback runs start here until you say what to try.`, |
| 457 | 552 | "", |
| 458 | ...handoverLines(h), | |
| 459 | ].join("\n"); | |
| 553 | ...handoverQuotes(h), | |
| 554 | ].join("\n").trimEnd(); | |
| 460 | 555 | } |
test/crew-loop.test.ts +47 −2
| @@ -15,6 +15,7 @@ import { commitToBranch } from "../src/actions/repos.ts"; | ||
| 15 | 15 | import { openPull, submitReview } from "../src/actions/pulls.ts"; |
| 16 | 16 | import { getPull, type Pull } from "../src/core/pulls.ts"; |
| 17 | 17 | import { HANDOVER_NOTE, handedOverPulls, handoverOf } from "../src/crew/guard.ts"; |
| 18 | import { addReviewComment } from "../src/core/reviews.ts"; | |
| 18 | 19 | import { sendToReview, waitsForPerson } from "../src/crew/review.ts"; |
| 19 | 20 | import { createRepo, updateRepoSettings } from "../src/actions/repos.ts"; |
| 20 | 21 | import { setCrewRole } from "../src/core/agents.ts"; |
| @@ -413,6 +414,50 @@ describe("a crew reviewer's changes-requested wakes the builder", () => { | ||
| 413 | 414 | c.body.startsWith("Handed to a person by the crew:"), |
| 414 | 415 | ); |
| 415 | 416 | |
| 417 | test("the hand-over note quotes each review up to 600 characters, cut at a sentence end, with its line comments", async () => { | |
| 418 | const set = await updateRepoSettings(app, alice, fresh(), { | |
| 419 | autonomy: "autonomous", | |
| 420 | autonomyCeilingUsd: 100, | |
| 421 | autonomyBy: alice.id, | |
| 422 | }); | |
| 423 | if (!set.ok) throw new Error(JSON.stringify(set.error)); | |
| 424 | const pull = await masonPull("quotes"); | |
| 425 | const sentence = "The token is logged on every retry and must not be. "; | |
| 426 | const long = sentence.repeat(18).trim(); // ~900 characters | |
| 427 | expect(long.length).toBeGreaterThan(850); | |
| 428 | addReviewComment(app.db, { | |
| 429 | pullId: pull.id, | |
| 430 | authorId: sentinel.id, | |
| 431 | body: `<b>log</b> ${"x".repeat(300)}`, | |
| 432 | path: "src/auth.ts", | |
| 433 | line: 42, | |
| 434 | side: "new", | |
| 435 | commitOid: "", | |
| 436 | lineHash: "", | |
| 437 | }); | |
| 438 | await verdict(pull, long); | |
| 439 | settleFeedback(pull); | |
| 440 | await verdict(pull, "Round two."); | |
| 441 | settleFeedback(pull); | |
| 442 | await verdict(pull, "Round three."); | |
| 443 | const body = handoverNotes(pull)[0]!.body; | |
| 444 | const quote = body | |
| 445 | .split("\n") | |
| 446 | .filter((l) => l.startsWith("> The token")) | |
| 447 | .join(""); | |
| 448 | expect(quote.length).toBeLessThanOrEqual(602); | |
| 449 | expect(quote.length).toBeGreaterThan(500); | |
| 450 | expect(quote.endsWith("retry and must not be.")).toBe(true); | |
| 451 | const line = body.split("\n").find((l) => l.startsWith("- src/auth.ts:42: "))!; | |
| 452 | expect(line).toBeDefined(); | |
| 453 | expect(line.length).toBe("- src/auth.ts:42: ".length + 200); | |
| 454 | expect(line).toContain("<b>log</b>"); | |
| 455 | expect(body).toContain("> Round two."); | |
| 456 | expect(body).toContain("> Round three."); | |
| 457 | // Out of the way of the tests after it, which count open hand-overs. | |
| 458 | await setIssueState(app, alice, fresh(), getIssueById(app.db, pull.id)!, "closed"); | |
| 459 | }); | |
| 460 | ||
| 416 | 461 | test("the third changes-requested hands the pull to a person; a fourth queues nothing", async () => { |
| 417 | 462 | const set = await updateRepoSettings(app, alice, fresh(), { |
| 418 | 463 | autonomy: "autonomous", |
| @@ -432,8 +477,8 @@ describe("a crew reviewer's changes-requested wakes the builder", () => { | ||
| 432 | 477 | expect(feedback(pull).length).toBe(2); |
| 433 | 478 | const notes = handoverNotes(pull); |
| 434 | 479 | expect(notes.length).toBe(1); |
| 435 | expect(notes[0]!.body).toContain("1. sentinel: Round one"); | |
| 436 | expect(notes[0]!.body).toContain("3. sentinel: Round three"); | |
| 480 | expect(notes[0]!.body).toContain("1. sentinel:\n\n> Round one"); | |
| 481 | expect(notes[0]!.body).toContain("3. sentinel:\n\n> Round three"); | |
| 437 | 482 | expect(notes[0]!.body).toContain("planner"); |
| 438 | 483 | // Not parked, not closed: still open, and the planner's, not a person's (#1655). |
| 439 | 484 | expect(getIssueById(app.db, pull.id)!.state).toBe("open"); |
new above
offline
Slicing UTF-16 units can split an emoji or other astral character. Use Array.from(...).slice(0, n).join("") (or cut at a code-point boundary) for the 200 and 600 caps.