trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

A reviewed-pull post says the verdict, approved or changes asked, and the reviewer's first sentence #6459

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/5823-a-reviewed-pull-post-says-the-verdict-ap → main 7 commits fork

  • Merged. scout/5823-a-reviewed-pull-post-says-the-verdict-ap was merged into main as 0e2eaaf.

Review

Sign in to review this change.
3 files changed +205 −4
src/actions/pulls.ts +14 −3
@@ -558,15 +558,20 @@ export async function submitReview(
558558 pull.id,
559559 [sent.value],
560560 );
561 const stored = getReviewByRemoteId(app.db, pull.id, sent.value.remoteId);
561562 app.bus.append({
562563 kind: "pull.reviewed",
563564 repoId: repo.id,
564565 actorId: user.id,
565 payload: { number: pull.number, state: input.state, via: "origin" },
566 payload: {
567 number: pull.number,
568 state: input.state,
569 via: "origin",
570 ...(stored ? { reviewId: stored.id } : {}),
571 },
566572 });
567573 // Line comments stay here, as `addReview`'s always have on a mirror.
568574 for (const line of anchored.value) postLine(app, pull, user, line);
569 const stored = getReviewByRemoteId(app.db, pull.id, sent.value.remoteId);
570575 return stored ? ok(stored) : err("origin_refused");
571576 }
572577
@@ -611,6 +616,7 @@ export async function submitReview(
611616 number: pull.number,
612617 state: input.state,
613618 body: (input.body ?? "").trim(),
619 reviewId: recorded.value.id,
614620 },
615621 });
616622 return recorded;
@@ -752,7 +758,12 @@ export async function approveHere(
752758 kind: "pull.reviewed",
753759 repoId: repo.id,
754760 actorId: user.id,
755 payload: { number: pull.number, state: "approved", via: "here" },
761 payload: {
762 number: pull.number,
763 state: "approved",
764 via: "here",
765 reviewId: recorded.value.id,
766 },
756767 });
757768 return recorded;
758769 }
src/feed/items.ts +41 −1
@@ -12,6 +12,7 @@ import type { Chips } from "../core/feed_chips.ts";
1212 import { listDecidedIn } from "../core/decisions.ts";
1313 import { audienceFilter } from "../core/event_audience.ts";
1414 import { listEventsVisibleTo, type ForgeEvent } from "../core/events.ts";
15 import { getReview } from "../core/reviews.ts";
1516 import { commentAnchor, getIssuesByNumbers, numberKey, type Issue } from "../core/issues.ts";
1617 import {
1718 listDetachedMirrors,
@@ -516,6 +517,45 @@ interface Subjects {
516517 pull(repoId: string, number: number): Pull | undefined;
517518 }
518519
520 /** Asked-for-changes sits above plain news; an approval is news. */
521 const CHANGES_URGENCY = 1;
522
523 /**
524 * The verdict and the reviewer's first sentence, from the review the event
525 * names. A review that is gone, or one with no verdict, keeps the old words.
526 */
527 function reviewedPost(
528 db: Database,
529 p: Record<string, unknown>,
530 common: Omit<Partial, "what">,
531 title: string,
532 ): Partial {
533 const review =
534 typeof p["reviewId"] === "string" ? getReview(db, p["reviewId"]) : undefined;
535 const on = title === "" ? "a pull request" : clip(title);
536 if (review?.state === "approved")
537 return { ...common, what: `approved ${on}` };
538 if (review?.state === "changes_requested") {
539 const first = firstSentence(review.body);
540 return {
541 ...common,
542 urgency: CHANGES_URGENCY,
543 what: `asked for changes on ${on}${first === "" ? "" : `: ${first}`}`,
544 };
545 }
546 return {
547 ...common,
548 what: `reviewed a pull request${title ? `: ${clip(title)}` : ""}`,
549 };
550 }
551
552 /** Up to the first sentence's end, at most 100 characters. */
553 function firstSentence(body: string): string {
554 const text = body.trim().replace(/\s+/g, " ");
555 const end = text.search(/[.!?](\s|$)/);
556 return clip(end === -1 ? text : text.slice(0, end + 1), 100);
557 }
558
519559 function fromEvent(
520560 db: Database,
521561 e: ForgeEvent,
@@ -581,7 +621,7 @@ function fromEvent(
581621 case "pull.merged":
582622 return { ...common, kind: "pull", link: change, what: `merged a pull request${title ? `: ${clip(title)}` : ""}${toldWords}` };
583623 case "pull.reviewed":
584 return { ...common, kind: "pull", link: change, what: `reviewed a pull request${title ? `: ${clip(title)}` : ""}` };
624 return reviewedPost(db, p, { ...common, kind: "pull", link: change }, title);
585625 case "look.taken": {
586626 const env = p["env"];
587627 const oid = p["commitOid"];
test/feed-reviewed.test.ts added +150
@@ -0,0 +1,150 @@
1 /** A reviewed-pull post says the verdict and the reviewer's first sentence (#5823). */
2
3 import { afterAll, beforeAll, expect, test } from "bun:test";
4 import { mkdtemp, rm } from "node:fs/promises";
5 import { tmpdir } from "node:os";
6 import { join } from "node:path";
7 import { createApp, type App } from "../src/app.ts";
8 import { addToCrew, createAgent } from "../src/actions/crew.ts";
9 import { openPull, submitReview } from "../src/actions/pulls.ts";
10 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
11 import type { Pull } from "../src/core/pulls.ts";
12 import type { Repo } from "../src/core/repos.ts";
13 import { createToken } from "../src/core/tokens.ts";
14 import { createUser, getUserByName, type User } from "../src/core/users.ts";
15 import { serve, type Servers } from "../src/http/server.ts";
16 import { candidatesFor } from "../src/feed/items.ts";
17 import { quietLogs } from "./helpers/globals.ts";
18
19 quietLogs();
20
21 const PASSWORD = "a-long-reviewed-passphrase";
22 const APPROVED = "approved Tidy the footer";
23 const CHANGES = "asked for changes on Widen the board: The migration drops a column people use.";
24
25 let dataDir = "";
26 let app: App;
27 let servers: Servers;
28 let alice: User;
29 let cookie = "";
30 let token = "";
31 let repo: Repo;
32 let n = 0;
33
34 const pull = async (title: string): Promise<Pull> => {
35 n++;
36 await commitToBranch(app, alice, repo, {
37 branch: `bot/${n}`,
38 base: "main",
39 message: title,
40 files: [{ path: `F${n}.md`, content: `${n}\n` }],
41 });
42 const opened = await openPull(app, getUserByName(app.db, "bot")!, repo, {
43 title,
44 body: "",
45 baseBranch: "main",
46 headBranch: `bot/${n}`,
47 });
48 if (!opened.ok) throw new Error(String(opened.error));
49 return opened.value;
50 };
51
52 /** The fold appends " · opened …, 1 review" (#5808); the verdict is what comes before it. */
53 const said = (what: string): string => what.split(" · ")[0]!;
54
55 const mcpFeed = async (): Promise<string[]> => {
56 const res = await fetch(`${servers.url}/mcp`, {
57 method: "POST",
58 headers: {
59 "content-type": "application/json",
60 accept: "application/json, text/event-stream",
61 authorization: `Bearer ${token}`,
62 },
63 body: JSON.stringify({
64 jsonrpc: "2.0",
65 id: 1,
66 method: "tools/call",
67 params: { name: "feed", arguments: {} },
68 }),
69 });
70 const body = (await res.json()) as { result: { content: { text: string }[] } };
71 const text = body.result.content[0]!.text;
72 return (JSON.parse(text) as { items: { what: string }[] }).items.map((i) => said(i.what));
73 };
74
75 beforeAll(async () => {
76 dataDir = await mkdtemp(join(tmpdir(), "forge-feed-reviewed-"));
77 app = createApp({ dataDir, port: 0 });
78 servers = await serve(app);
79 const a = await createUser(app.db, {
80 origin: "seed",
81 trusted: true,
82 username: "alice",
83 email: "[email protected]",
84 password: PASSWORD,
85 adminIfFirst: true,
86 });
87 if (!a.ok) throw new Error(a.error);
88 alice = a.value;
89 const res = await fetch(`${servers.url}/signin`, {
90 method: "POST",
91 body: new URLSearchParams({ username: "alice", password: PASSWORD }),
92 redirect: "manual",
93 });
94 cookie = (res.headers.get("set-cookie") ?? "").split(";")[0]!;
95 token = createToken(app.db, alice.id, "ro", "ro").token;
96 await createAgent(app, alice, { username: "bot" });
97 const made = await createRepo(app, alice, { name: "site", visibility: "public" });
98 if (!made.ok) throw new Error(String(made.error));
99 repo = made.value;
100 await commitToBranch(app, alice, repo, {
101 message: "seed",
102 files: [{ path: "README.md", content: "# site\n" }],
103 });
104 addToCrew(app, alice, repo, "bot");
105 }, 30_000);
106
107 afterAll(async () => {
108 await servers.stop();
109 await rm(dataDir, { recursive: true, force: true });
110 });
111
112 test("each verdict is said in the post, asked-for-changes first, on / and in the MCP feed", async () => {
113 const ok = await pull("Tidy the footer");
114 const bad = await pull("Widen the board");
115 // The approval is the later event, so recency alone would put it first.
116 const asked = await submitReview(app, alice, repo, bad, {
117 state: "changes_requested",
118 body: "The migration drops a column people use. Please keep it.",
119 });
120 expect(asked.ok).toBe(true);
121 const approved = await submitReview(app, alice, repo, ok, { state: "approved" });
122 expect(approved.ok).toBe(true);
123
124 const posts = candidatesFor(app.db, alice, Date.now())
125 .map((c) => said(c.what))
126 .filter((w) => w.startsWith("approved ") || w.startsWith("asked for changes"));
127 expect(posts.sort()).toEqual([APPROVED, CHANGES].sort());
128
129 const home = await fetch(`${servers.url}/`, { headers: { cookie } });
130 const html = (await home.text()).replace(/\s+/g, " ");
131 const at = (s: string): number => html.indexOf(s);
132 expect(at(APPROVED)).toBeGreaterThan(-1);
133 expect(at(CHANGES)).toBeGreaterThan(-1);
134 expect(at(CHANGES)).toBeLessThan(at(APPROVED));
135
136 const rows = (await mcpFeed()).filter((w) => w === APPROVED || w === CHANGES);
137 expect(rows).toEqual([CHANGES, APPROVED]);
138 });
139
140 test("a review that is gone, or has no verdict, keeps the old words", async () => {
141 const gone = await pull("Remove the banner");
142 const given = await submitReview(app, alice, repo, gone, { state: "approved" });
143 if (!given.ok) throw new Error(given.error);
144 app.db.run("DELETE FROM review WHERE id = ?1", [given.value.id]);
145 const note = await pull("Explain the banner");
146 expect((await submitReview(app, alice, repo, note, { state: "commented", body: "Hm." })).ok).toBe(true);
147 const words = candidatesFor(app.db, alice, Date.now()).map((c) => said(c.what));
148 expect(words).toContain("reviewed a pull request: Remove the banner");
149 expect(words).toContain("reviewed a pull request: Explain the banner");
150 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs