trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

Review refusals live in one module, and the pull page, the review action and MCP read them from there #5569

Merged masoncrew opened this

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

ConversationFiles
Merged

mason/forge:mason/5565-review-refusals-live-in-one-module-and-t → main 1 commit fork

  • Merged. mason/5565-review-refusals-live-in-one-module-and-t was merged into main as ff98bc1.

Review

Sign in to review this change.
5 files changed +176 −12
src/actions/review_words.ts added +13
@@ -0,0 +1,13 @@
1 /**
2 * The refusals of a review, in words (#5565): the pull page, the studio's
3 * review action and MCP's `review` tool all say them, and each reads them
4 * from here so the doors cannot drift. Plain strings, no logic.
5 */
6 export const reviewWords = {
7 /** `self_review`: an author cannot approve their own change. */
8 selfReview: "You cannot approve your own pull request.",
9 /** `body_too_long`: the review's body is over 64 KB. */
10 tooLong: "That review is too long (64 KB at most).",
11 /** `forbidden`: a reader cannot pass a verdict that gates a merge. */
12 forbidden: "A verdict that gates a merge needs write access.",
13 } as const;
src/http/mcp.ts +4 −3
@@ -77,6 +77,7 @@ import {
7777 submitReview,
7878 } from "../actions/pulls.ts";
7979 import { originError } from "../web/pages/thread.ts";
80 import { reviewWords } from "../actions/review_words.ts";
8081 import { holdPull, releasePull } from "../actions/holds.ts";
8182 import { openPlan, parsePlan, PLAN_MAX, proposePlan } from "../actions/plan.ts";
8283 import {
@@ -3519,11 +3520,11 @@ const TOOLS: readonly Tool[] = [
35193520 "refused",
35203521 originError(error) ??
35213522 (error === "self_review"
3522 ? "You cannot approve your own pull request."
3523 ? reviewWords.selfReview
35233524 : error === "forbidden"
3524 ? "A verdict that gates a merge needs write access."
3525 ? reviewWords.forbidden
35253526 : error === "body_too_long"
3526 ? "That review is too long (64 KB at most)."
3527 ? reviewWords.tooLong
35273528 : "That review could not be recorded."),
35283529 );
35293530 }
src/web/pages/pull.ts +7 −7
@@ -19,6 +19,7 @@ import {
1919 submitReview,
2020 approveToRun,
2121 } from "../../actions/pulls.ts";
22 import { reviewWords } from "../../actions/review_words.ts";
2223 import { readFileAt } from "../../apps/checkout.ts";
2324 import { parseCheck } from "../../apps/manifest.ts";
2425 import { tierLine, type TierState } from "../../checks/tiers.ts";
@@ -738,15 +739,14 @@ function reviewCard(ctx: Ctx, repo: Repo, pull: Pull, action: string): Html {
738739 isAuthor,
739740 () =>
740741 html`<span class="muted small"
741 >${icon("alert")} You cannot approve your own pull request.</span
742 >${icon("alert")} ${reviewWords.selfReview}</span
742743 >`,
743744 )}
744745 ${when(
745746 !writer,
746747 () =>
747748 html`<span class="muted small"
748 >${icon("alert")} A verdict that gates a merge needs write
749 access.</span
749 >${icon("alert")} ${reviewWords.forbidden}</span
750750 >`,
751751 )}
752752 <span class="spacer"></span>
@@ -1160,7 +1160,7 @@ export async function pullApproveHereSubmit(ctx: Ctx): Promise<Response> {
11601160 done.error === "forbidden" || done.error === "unauthorized"
11611161 ? "Only a person with write access can approve code to run here."
11621162 : done.error === "self_review"
1163 ? "You cannot approve your own pull request."
1163 ? reviewWords.selfReview
11641164 : done.error === "not_open"
11651165 ? "That pull request is closed."
11661166 : done.error === "not_foreign"
@@ -1215,11 +1215,11 @@ export async function pullReviewStateSubmit(ctx: Ctx): Promise<Response> {
12151215 message:
12161216 originError(error) ??
12171217 (error === "self_review"
1218 ? "You cannot approve your own pull request."
1218 ? reviewWords.selfReview
12191219 : error === "forbidden"
1220 ? "A verdict that gates a merge needs write access. You can still leave a comment."
1220 ? `${reviewWords.forbidden} You can still leave a comment.`
12211221 : error === "body_too_long"
1222 ? "That review is too long (64 KB at most)."
1222 ? reviewWords.tooLong
12231223 : error === "invalid_state"
12241224 ? "That is not a verdict this forge knows."
12251225 : "That review could not be recorded."),
src/web/studio/review_act.ts +3 −2
@@ -10,6 +10,7 @@
1010 import type { Database } from "bun:sqlite";
1111 import { mergePull, submitReview } from "../../actions/pulls.ts";
1212 import { addCommentThrough, setIssueStateThrough } from "../../actions/issues.ts";
13 import { reviewWords } from "../../actions/review_words.ts";
1314 import { getIssue } from "../../core/issues.ts";
1415 import { getPull } from "../../core/pulls.ts";
1516 import { canRead, getRepo, listReposVisibleTo } from "../../core/repos.ts";
@@ -124,9 +125,9 @@ export async function reviewAct(ctx: Ctx): Promise<Response> {
124125 return refuse(
125126 originError(reviewed.error) ??
126127 (reviewed.error === "forbidden"
127 ? "A verdict that gates a merge needs write access."
128 ? reviewWords.forbidden
128129 : reviewed.error === "body_too_long"
129 ? "That review is too long (64 KB at most)."
130 ? reviewWords.tooLong
130131 : "That review could not be recorded."),
131132 );
132133 done = "Changes requested.";
test/review-words.test.ts added +149
@@ -0,0 +1,149 @@
1 /**
2 * A review's refusals read the same from every door (#5565): the pull page,
3 * the studio's review action and MCP's `review` tool each answer a reader's
4 * verdict with `reviewWords.forbidden`, and no file under src/ but the module
5 * types any of the sentences out.
6 */
7 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
8 import { mkdtemp, readdir, readFile, rm } from "node:fs/promises";
9 import { tmpdir } from "node:os";
10 import { join } from "node:path";
11 import { createApp, type App } from "../src/app.ts";
12 import { openPull } from "../src/actions/pulls.ts";
13 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
14 import { reviewWords } from "../src/actions/review_words.ts";
15 import { listReviews } from "../src/core/reviews.ts";
16 import type { Repo } from "../src/core/repos.ts";
17 import { createToken } from "../src/core/tokens.ts";
18 import { createUser, type User } from "../src/core/users.ts";
19 import { serve, type Servers } from "../src/http/server.ts";
20 import { promote } from "./helpers/members.ts";
21 import { quietLogs } from "./helpers/globals.ts";
22
23 quietLogs();
24
25 describe("a review's refusals, from every door", () => {
26 let dataDir: string;
27 let app: App;
28 let servers: Servers;
29 let alice: User;
30 let repo: Repo;
31 let bobCookie = "";
32 let bobRw = "";
33 let pullNumber = 0;
34 let pullId = "";
35
36 /** The message of the flash a refused form POST set. */
37 const flashOf = (res: Response): string => {
38 const raw = (res.headers.get("set-cookie") ?? "").split(";")[0]!.split("=").slice(1).join("=");
39 return (JSON.parse(decodeURIComponent(raw)) as { message: string }).message;
40 };
41
42 beforeAll(async () => {
43 dataDir = await mkdtemp(join(tmpdir(), "forge-review-words-"));
44 app = createApp({ dataDir, port: 0 });
45 servers = await serve(app);
46 const mk = async (username: string, first = false) => {
47 const made = await createUser(app.db, {
48 origin: "seed",
49 trusted: true,
50 username,
51 email: `${username}@forge.local`,
52 password: "a-good-passphrase",
53 adminIfFirst: first,
54 });
55 if (!made.ok) throw new Error(made.error);
56 return made.value;
57 };
58 alice = await mk("alice", true);
59 const bob = await mk("bob");
60 promote(app.db, alice, bob);
61 bobRw = createToken(app.db, bob.id, "rw", "rw").token;
62 const made = await createRepo(app, alice, { name: "game", visibility: "public" });
63 if (!made.ok) throw new Error(String(made.error));
64 repo = made.value;
65 await commitToBranch(app, alice, repo, {
66 message: "seed",
67 files: [{ path: "README.md", content: "# game\n" }],
68 });
69 const branch = await commitToBranch(app, alice, repo, {
70 branch: "change",
71 message: "change",
72 files: [{ path: "change.txt", content: "change\n" }],
73 });
74 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
75 const opened = await openPull(app, alice, repo, { title: "change", baseBranch: "main", headBranch: "change" });
76 if (!opened.ok) throw new Error(String(opened.error));
77 pullNumber = opened.value.number;
78 pullId = opened.value.id;
79 const signin = await fetch(`${servers.url}/signin`, {
80 method: "POST",
81 headers: { origin: servers.url },
82 body: new URLSearchParams({ username: "bob", password: "a-good-passphrase" }),
83 redirect: "manual",
84 });
85 bobCookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!;
86 }, 30_000);
87
88 afterAll(async () => {
89 await servers.stop();
90 app.close();
91 await rm(dataDir, { recursive: true, force: true });
92 });
93
94 test("the pull page refuses a reader's verdict in the module's words", async () => {
95 const res = await fetch(`${servers.url}/alice/game/pulls/${pullNumber}/review-state`, {
96 method: "POST",
97 headers: { cookie: bobCookie, origin: servers.url },
98 body: new URLSearchParams({ state: "approved" }),
99 redirect: "manual",
100 });
101 expect(res.status).toBe(303);
102 expect(flashOf(res)).toBe(`${reviewWords.forbidden} You can still leave a comment.`);
103 });
104
105 test("the studio's review action refuses a reader's verdict in the module's words", async () => {
106 const res = await fetch(`${servers.url}/review/act`, {
107 method: "POST",
108 headers: { cookie: bobCookie, origin: servers.url },
109 body: new URLSearchParams({ repo: "alice/game", number: String(pullNumber), verb: "changes", body: "Rename it." }),
110 redirect: "manual",
111 });
112 expect(res.status).toBe(303);
113 expect(flashOf(res)).toBe(reviewWords.forbidden);
114 });
115
116 test("MCP's review tool refuses a reader's verdict in the module's words", async () => {
117 const res = await fetch(`${servers.url}/mcp`, {
118 method: "POST",
119 headers: {
120 "content-type": "application/json",
121 accept: "application/json, text/event-stream",
122 authorization: `Bearer ${bobRw}`,
123 },
124 body: JSON.stringify({
125 jsonrpc: "2.0",
126 id: 1,
127 method: "tools/call",
128 params: { name: "review", arguments: { repo: "alice/game", number: pullNumber, verdict: "approve" } },
129 }),
130 });
131 const body = (await res.json()) as { result: { isError?: boolean; content: { text: string }[] } };
132 expect(body.result.isError).toBe(true);
133 expect(body.result.content[0]!.text).toContain(reviewWords.forbidden);
134 expect(listReviews(app.db, pullId)).toEqual([]);
135 });
136
137 test("no file under src/ but the module types the sentences out", async () => {
138 const root = join(import.meta.dir, "..", "src");
139 const files = (await readdir(root, { recursive: true })).filter((f) => f.endsWith(".ts"));
140 const typed: string[] = [];
141 for (const file of files) {
142 if (file === join("actions", "review_words.ts")) continue;
143 const text = await readFile(join(root, file), "utf8");
144 for (const sentence of Object.values(reviewWords))
145 if (text.includes(sentence)) typed.push(`${file}: ${sentence}`);
146 }
147 expect(typed).toEqual([]);
148 });
149 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs