trevato/forge

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

More
Forks1g fMapg aRuns11g rEventsFlows

MCP `review`: approve a pull, or request changes with a reason, as the page does #3657

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/3646-mcp-review-approve-a-pull-or-request-cha → main 2 commits fork 2 threads · 2 unresolved

  • Merged. scout/3646-mcp-review-approve-a-pull-or-request-cha was merged into main as 2b0fef4.

Review

Sign in to review this change.
4 files changed +240 −0
docs/MCP.md +1
@@ -58,6 +58,7 @@ for a tunnel and a forge that sleeps.
5858 | `milestones` | `{ app, issue? }`: how far each thing asked for is (and, with `issue`, whether you watch it: `watching`), from the dependency tree: the plan's limbs with their weights, or one milestone opened (who is building, what is blocked and on what, what merged in 7 days, an estimate, and for an owner `spend`: dollars, changes landed ever, the costliest three issues) | |
5959 | `limb` | `{ app, issue, action, reason?, pillar?, operation?, dryRun? }`: defer, resume or close a whole limb as not doing, or undo an operation; plans unless `dryRun` is false, and applying takes an admin token | yes |
6060 | `watch` | `{ app, issue, watch? }`: watch a milestone, or stop with `watch: false`: one item in your feed that follows it, and one push to your phone when it lands | yes |
61 | `review` | `{ repo, number, verdict, reason? }`: approve a pull (`approve`) or request changes (`changes`, with a `reason`), as the page does; a writer's or the crew's, and never your own pull | yes |
6162 | `usage` | `{ repo, days? }`: what the crew's runs spent — tokens, cost, wall time — by subscription, agent, issue, and each account's 5-hour and weekly standing; a writer's | |
6263 | `labels` | `{ repo, number, labels }`: set an issue's or a pull's labels to exactly these (`[]` clears); only labels the repository has, by a writer | yes |
6364 | `link` | `{ app, issue, blockedBy }`: the issue waits for another; loops and cross-repository are refused | yes |
src/http/mcp.ts +57
@@ -53,6 +53,7 @@ import {
5353 linkIssue,
5454 unlinkIssue,
5555 } from "../actions/dependencies.ts";
56 import { submitReview } from "../actions/pulls.ts";
5657 import { originError } from "../web/pages/thread.ts";
5758 import { holdPull, releasePull } from "../actions/holds.ts";
5859 import { openPlan, parsePlan, PLAN_MAX, proposePlan } from "../actions/plan.ts";
@@ -2348,6 +2349,62 @@ const TOOLS: readonly Tool[] = [
23482349 : bad("not_found", "no such app of yours (owner/name)");
23492350 },
23502351 },
2352 {
2353 name: "review",
2354 description:
2355 "Approve a pull, or request changes with a reason (`reason` is required for changes); approving your own pull, or a verdict without write access, is refused as on the page.",
2356 inputSchema: {
2357 type: "object",
2358 properties: {
2359 repo: REPO_ARG,
2360 number: { type: "integer", minimum: 1 },
2361 verdict: { type: "string", enum: ["approve", "changes"] },
2362 reason: { type: "string", maxLength: 65536 },
2363 },
2364 required: ["repo", "number", "verdict"],
2365 },
2366 writes: true,
2367 async run(ctx, user, args) {
2368 const repo = repoArg(ctx, user, args);
2369 if (isAnswer(repo)) return repo;
2370 const verdict = args["verdict"];
2371 if (verdict !== "approve" && verdict !== "changes")
2372 return bad("invalid", "verdict is approve or changes");
2373 const reason = str(args["reason"]);
2374 if (verdict === "changes" && reason === "")
2375 return bad("invalid", "say what to change (reason)");
2376 const n = args["number"];
2377 const pull =
2378 typeof n === "number" && Number.isInteger(n) && n > 0
2379 ? getPull(ctx.app.db, repo.id, n)
2380 : undefined;
2381 if (pull === undefined || !mayView(ctx.app.db, repo, pull, user))
2382 return bad("no_such_pull", `no pull #${String(n)}`);
2383 const state = verdict === "approve" ? "approved" : "changes_requested";
2384 const sent = await submitReview(ctx.app, user, repo, pull, {
2385 state,
2386 ...(reason === "" ? {} : { body: reason }),
2387 });
2388 if (!sent.ok) {
2389 const error = sent.error;
2390 return bad(
2391 "refused",
2392 originError(error) ??
2393 (error === "self_review"
2394 ? "You cannot approve your own pull request."
2395 : error === "forbidden"
2396 ? "A verdict that gates a merge needs write access."
2397 : error === "body_too_long"
2398 ? "That review is too long (64 KB at most)."
2399 : "That review could not be recorded."),
2400 );
2401 }
2402 return good({
2403 says: verdict === "approve" ? "Approved." : "Changes requested.",
2404 url: `/${repo.ownerName}/${repo.name}/pulls/${pull.number}`,
2405 });
2406 },
2407 },
23512408 {
23522409 name: "spend",
23532410 description:
test/mcp-review.test.ts added +181
@@ -0,0 +1,181 @@
1 /**
2 * The `review` tool over MCP: the page's verdict, with the page's gate —
3 * a writer's approval lands, a reader's is refused in the page's words, an
4 * author cannot approve their own pull, and bad input stops at the boundary.
5 */
6 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
7 import { mkdtemp, rm } from "node:fs/promises";
8 import { tmpdir } from "node:os";
9 import { join } from "node:path";
10 import { createApp, type App } from "../src/app.ts";
11 import { openPull } from "../src/actions/pulls.ts";
12 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
13 import { listReviews } from "../src/core/reviews.ts";
14 import { createToken } from "../src/core/tokens.ts";
15 import { createUser, type User } from "../src/core/users.ts";
16 import { serve, type Servers } from "../src/http/server.ts";
17 import { promote } from "./helpers/members.ts";
18
19 describe("the review tool over MCP", () => {
20 let dataDir: string;
21 let app: App;
22 let servers: Servers;
23 let alice: User;
24 let bob: User;
25 let aliceRw: string;
26 let bobRw: string;
27 let nextId = 1;
28
29 const tool = async (token: string, args: unknown) => {
30 const res = await fetch(`${servers.url}/mcp`, {
31 method: "POST",
32 headers: {
33 "content-type": "application/json",
34 accept: "application/json, text/event-stream",
35 authorization: `Bearer ${token}`,
36 },
37 body: JSON.stringify({
38 jsonrpc: "2.0",
39 id: nextId++,
40 method: "tools/call",
41 params: { name: "review", arguments: args },
42 }),
43 });
44 const body = (await res.json()) as any;
45 const text: string = body.result.content[0].text;
46 return body.result.isError === true
47 ? { error: text }
48 : { value: JSON.parse(text) };
49 };
50
51 /** A pull on alice/game, its author set to `author`. */
52 const pullBy = async (branch: string, author: User) => {
53 const repo = (await import("../src/core/repos.ts")).getRepo(
54 app.db,
55 "alice",
56 "game",
57 )!;
58 const made = await commitToBranch(app, alice, repo, {
59 branch,
60 message: branch,
61 files: [{ path: `${branch}.txt`, content: `${branch}\n` }],
62 });
63 if (!made.ok) throw new Error(JSON.stringify(made.error));
64 const opened = await openPull(app, alice, repo, {
65 title: branch,
66 baseBranch: "main",
67 headBranch: branch,
68 });
69 if (!opened.ok) throw new Error(String(opened.error));
70 app.db
71 .query("UPDATE issue SET author_id = ?1 WHERE id = ?2")
72 .run(author.id, opened.value.id);
73 return opened.value;
74 };
75
76 beforeAll(async () => {
77 dataDir = await mkdtemp(join(tmpdir(), "forge-mcp-review-"));
78 app = createApp({ dataDir, port: 0 });
79 servers = await serve(app);
80 const mk = async (username: string, first = false) => {
81 const made = await createUser(app.db, {
82 origin: "seed",
83 trusted: true,
84 username,
85 email: `${username}@forge.local`,
86 password: "a-good-passphrase",
87 adminIfFirst: first,
88 });
89 if (!made.ok) throw new Error(made.error);
90 return made.value;
91 };
92 alice = await mk("alice", true);
93 bob = await mk("bob");
94 promote(app.db, alice, bob);
95 aliceRw = createToken(app.db, alice.id, "rw", "rw").token;
96 bobRw = createToken(app.db, bob.id, "rw", "rw").token;
97 const made = await createRepo(app, alice, {
98 name: "game",
99 visibility: "public",
100 });
101 if (!made.ok) throw new Error(String(made.error));
102 await commitToBranch(app, alice, made.value, {
103 message: "seed",
104 files: [{ path: "README.md", content: "# game\n" }],
105 });
106 }, 30_000);
107
108 afterAll(async () => {
109 servers.stop();
110 app.close();
111 await rm(dataDir, { recursive: true, force: true });
112 });
113
114 test("a writer approves, and requests changes with a reason", async () => {
115 const pull = await pullBy("one", bob);
116 const approved = await tool(aliceRw, {
117 repo: "alice/game",
118 number: pull.number,
119 verdict: "approve",
120 });
121 expect(approved.value).toEqual({
122 says: "Approved.",
123 url: `/alice/game/pulls/${pull.number}`,
124 });
125 const asked = await tool(aliceRw, {
126 repo: "alice/game",
127 number: pull.number,
128 verdict: "changes",
129 reason: "Add a test.",
130 });
131 expect(asked.value.says).toBe("Changes requested.");
132 expect(listReviews(app.db, pull.id).map((r) => [r.state, r.body])).toEqual([
133 ["approved", ""],
134 ["changes_requested", "Add a test."],
135 ]);
136 });
137
138 test("a reader's verdict is refused with the page's words", async () => {
139 const pull = await pullBy("two", alice);
140 const refused = await tool(bobRw, {
141 repo: "alice/game",
142 number: pull.number,
143 verdict: "approve",
144 });
145 expect(refused.error).toContain(
146 "A verdict that gates a merge needs write access.",
147 );
148 expect(listReviews(app.db, pull.id)).toEqual([]);
149 });
150
151 test("an author cannot approve their own pull", async () => {
152 const pull = await pullBy("three", alice);
153 const refused = await tool(aliceRw, {
154 repo: "alice/game",
155 number: pull.number,
156 verdict: "approve",
157 });
158 expect(refused.error).toContain("You cannot approve your own pull request.");
159 });
160
161 test("bad input is refused at the boundary", async () => {
162 const pull = await pullBy("four", bob);
163 const base = { repo: "alice/game", number: pull.number };
164 expect((await tool(aliceRw, { ...base, verdict: "lgtm" })).error).toContain(
165 "verdict is approve or changes",
166 );
167 expect(
168 (await tool(aliceRw, { ...base, verdict: "changes", reason: " " })).error,
169 ).toContain("say what to change");
170 expect(
171 (await tool(aliceRw, { ...base, number: 9999, verdict: "approve" })).error,
172 ).toContain("no pull #9999");
173 expect(
174 (await tool(aliceRw, { ...base, number: "1", verdict: "approve" })).error,
175 ).toContain("no pull");
176 expect(
177 (await tool(aliceRw, { repo: "nope", number: 1, verdict: "approve" })).error,
178 ).toContain("not_found");
179 expect(listReviews(app.db, pull.id)).toEqual([]);
180 });
181 });
test/mcp.test.ts +1
@@ -221,6 +221,7 @@ describe("the forge over MCP", () => {
221221 "watch",
222222 "limb",
223223 "returns",
224 "review",
224225 "spend",
225226 "usage",
226227 "labels",

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs