Public: anyone can read this; the crew can change it.
MCP `take_back` opens the pull that undoes a shipped one, with the same rules as the page #7221
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/7211-mcp-take-back-opens-the-pull-that-undoes → main 3 commits fork 3 threads · 3 unresolved
7 files
changed +251
−37
docs/MCP.md +1
| @@ -161,6 +161,7 @@ for a tunnel and a forge that sleeps. | ||
| 161 | 161 | | `subscriptions` | the Claude accounts the crew works on: id (the handle `subscription_wake`, `subscription_budget` and `subscription_delete` take), name, kind, the repositories and organizations each pays for, budget and spend, and whether it is resting; never a token. A forge admin's | | |
| 162 | 162 | | `sync` | `{ repo, originId? }`: sync a mirror now, or with `originId` (admin token) reconnect its origin; refused as `POST …/sync` and `…/origin/reconnect` are, a fork is pointed to `take_updates` | yes | |
| 163 | 163 | | `system` | the machine the forge runs on, now: CPU, load, memory, disk, network, processes; a forge admin, or an agent granted `telemetry:read` | | |
| 164 | | `take_back` | `{ repo, number }`: open the pull that undoes shipped pull `number` (branch `revert-N`, title `Revert "…"`), never touching the default branch; the same rules and words as the pull page's take it back: refused for a pull not shipped (`not_shipped`), already taken back (`already_reverted`), on a mirror (`mirrored`), a revert that does not apply (`conflict`), an agent (`forbidden`) and a pull the caller cannot see (`not_found`); answers the new pull's `number`, `title` and `url` | yes | | |
| 164 | 165 | | `take_updates` | a fork takes its parent's updates when git can do it alone; a conflict is refused. On a spawned forge's code repository it opens the one `forge/take-updates` pull and never moves the default branch | yes | |
| 165 | 166 | | `teams` | `{ slug?, charter?, dial?, members?, dryRun? }`: the crew teams (person admins): without `slug` lists them; with it, changes charter, dial (0–100) or members, planned unless `dryRun` is false | yes | |
| 166 | 167 | | `telemetry_grant` | `{ agent, on }`: grant an agent `telemetry:read`, or take it back with `false`, as the Telemetry page does; a person admin's, with an admin token; answers `says` | yes | |
src/actions/revert_words.ts added +42
| @@ -0,0 +1,42 @@ | ||
| 1 | /** | |
| 2 | * What a take-back refusal says (#7211): one place, so the studio's | |
| 3 | * "take it back" button and the MCP `take_back` tool read the same. | |
| 4 | */ | |
| 5 | ||
| 6 | import type { RevertPullError } from "./revert.ts"; | |
| 7 | ||
| 8 | /** A take-back refusal in the studio's words. */ | |
| 9 | export function takeBackWords(error: RevertPullError): string { | |
| 10 | if (typeof error === "string") { | |
| 11 | switch (error) { | |
| 12 | case "not_shipped": | |
| 13 | return "it has not shipped"; | |
| 14 | case "mirrored": | |
| 15 | return "the origin owns this history"; | |
| 16 | case "already_reverted": | |
| 17 | return "it has already been taken back"; | |
| 18 | case "unauthorized": | |
| 19 | return "sign in first"; | |
| 20 | case "forbidden": | |
| 21 | return "only a person can take a pull request back"; | |
| 22 | case "not_found": | |
| 23 | return "that pull request was not found"; | |
| 24 | } | |
| 25 | } | |
| 26 | switch (error.kind) { | |
| 27 | case "conflict": | |
| 28 | return `it conflicts with the current code in ${error.files.join(", ")}`; | |
| 29 | case "unsafe": | |
| 30 | return "git refused it"; | |
| 31 | case "base_moved": | |
| 32 | return "the base moved under it"; | |
| 33 | case "git": | |
| 34 | return `git said ${error.message}`; | |
| 35 | case "octopus": | |
| 36 | return "it has more than two parents"; | |
| 37 | case "branch_failed": | |
| 38 | return `it could not be made ready (${error.reason})`; | |
| 39 | case "pull_failed": | |
| 40 | return `the change could not be opened (${error.reason})`; | |
| 41 | } | |
| 42 | } |
src/http/mcp/tools/index.ts +1
| @@ -135,6 +135,7 @@ export { tool as subscription_wake } from "./subscription_wake.ts"; | ||
| 135 | 135 | export { tool as subscriptions } from "./subscriptions.ts"; |
| 136 | 136 | export { tool as sync } from "./sync.ts"; |
| 137 | 137 | export { tool as system } from "./system.ts"; |
| 138 | export { tool as take_back } from "./take_back.ts"; | |
| 138 | 139 | export { tool as take_updates } from "./take_updates.ts"; |
| 139 | 140 | export { tool as teams } from "./teams.ts"; |
| 140 | 141 | export { tool as telemetry_grant } from "./telemetry_grant.ts"; |
src/http/mcp/tools/take_back.ts added +48
| @@ -0,0 +1,48 @@ | ||
| 1 | import { revertPull } from "../../../actions/revert.ts"; | |
| 2 | import { takeBackWords } from "../../../actions/revert_words.ts"; | |
| 3 | import { getPull } from "../../../core/pulls.ts"; | |
| 4 | import { mayView } from "../../../core/sealed.ts"; | |
| 5 | import { good, bad, type Tool, REPO_ARG, repoArg, isAnswer } from "../shared.ts"; | |
| 6 | ||
| 7 | export const tool: Tool = { | |
| 8 | name: "take_back", | |
| 9 | doc: "`{ repo, number }`: open the pull that undoes shipped pull `number` (branch `revert-N`, title `Revert \"…\"`), never touching the default branch; the same rules and words as the pull page's take it back: refused for a pull not shipped (`not_shipped`), already taken back (`already_reverted`), on a mirror (`mirrored`), a revert that does not apply (`conflict`), an agent (`forbidden`) and a pull the caller cannot see (`not_found`); answers the new pull's `number`, `title` and `url`", | |
| 10 | description: "Open the pull that undoes a shipped pull.", | |
| 11 | inputSchema: { | |
| 12 | type: "object", | |
| 13 | properties: { | |
| 14 | repo: REPO_ARG, | |
| 15 | number: { type: "integer", minimum: 1 }, | |
| 16 | }, | |
| 17 | required: ["repo", "number"], | |
| 18 | }, | |
| 19 | writes: true, | |
| 20 | async run(ctx, user, args) { | |
| 21 | const repo = repoArg(ctx, user, args); | |
| 22 | if (isAnswer(repo)) return repo; | |
| 23 | const n = args["number"]; | |
| 24 | const pull = | |
| 25 | typeof n === "number" && Number.isInteger(n) && n > 0 | |
| 26 | ? getPull(ctx.app.db, repo.id, n) | |
| 27 | : undefined; | |
| 28 | // `getPull` reads pulls only; an issue's number is not found, as on the page. | |
| 29 | if (pull === undefined || !mayView(ctx.app.db, repo, pull, user)) | |
| 30 | return bad("not_found", `That could not be taken back: ${takeBackWords("not_found")}.`); | |
| 31 | const reverted = await revertPull(ctx.app, user, repo, pull); | |
| 32 | if (!reverted.ok) { | |
| 33 | const e = reverted.error; | |
| 34 | return bad( | |
| 35 | typeof e === "string" ? e : e.kind, | |
| 36 | `That could not be taken back: ${takeBackWords(e)}.`, | |
| 37 | ); | |
| 38 | } | |
| 39 | const made = reverted.value; | |
| 40 | const origin = ctx.app.config.url ?? ctx.url.origin; | |
| 41 | return good({ | |
| 42 | says: `Opened "${made.title}" — ship it to take "${pull.title}" back.`, | |
| 43 | number: made.number, | |
| 44 | title: made.title, | |
| 45 | url: `${origin}/${repo.ownerName}/${repo.name}/pulls/${made.number}`, | |
| 46 | }); | |
| 47 | }, | |
| 48 | }; |
src/web/studio/actions.ts +2 −37
| @@ -17,7 +17,8 @@ import { NOT_TOLD_FROM_WORDS, NOT_TOLD_WORDS, SAY_SOMETHING_WORDS } from "../../ | ||
| 17 | 17 | import { keepThought } from "../../actions/thought.ts"; |
| 18 | 18 | import { DIRECTION_NOTE } from "../../crew/direct.ts"; |
| 19 | 19 | import { mergePull } from "../../actions/pulls.ts"; |
| 20 | import { revertPull, type RevertPullError } from "../../actions/revert.ts"; | |
| 20 | import { revertPull } from "../../actions/revert.ts"; | |
| 21 | import { takeBackWords } from "../../actions/revert_words.ts"; | |
| 21 | 22 | import { getPull } from "../../core/pulls.ts"; |
| 22 | 23 | import { canRead, getRepo, type Repo } from "../../core/repos.ts"; |
| 23 | 24 | import { bringUpToDate } from "../../crew/autonomy.ts"; |
| @@ -241,42 +242,6 @@ export async function shipSubmit(ctx: Ctx): Promise<Response> { | ||
| 241 | 242 | }); |
| 242 | 243 | } |
| 243 | 244 | |
| 244 | /** A take-back refusal in the studio's words. */ | |
| 245 | function takeBackWords(error: RevertPullError): string { | |
| 246 | if (typeof error === "string") { | |
| 247 | switch (error) { | |
| 248 | case "not_shipped": | |
| 249 | return "it has not shipped"; | |
| 250 | case "mirrored": | |
| 251 | return "the origin owns this history"; | |
| 252 | case "already_reverted": | |
| 253 | return "it has already been taken back"; | |
| 254 | case "unauthorized": | |
| 255 | return "sign in first"; | |
| 256 | case "forbidden": | |
| 257 | return "only a person can take a pull request back"; | |
| 258 | case "not_found": | |
| 259 | return "that pull request was not found"; | |
| 260 | } | |
| 261 | } | |
| 262 | switch (error.kind) { | |
| 263 | case "conflict": | |
| 264 | return `it conflicts with the current code in ${error.files.join(", ")}`; | |
| 265 | case "unsafe": | |
| 266 | return "git refused it"; | |
| 267 | case "base_moved": | |
| 268 | return "the base moved under it"; | |
| 269 | case "git": | |
| 270 | return `git said ${error.message}`; | |
| 271 | case "octopus": | |
| 272 | return "it has more than two parents"; | |
| 273 | case "branch_failed": | |
| 274 | return `it could not be made ready (${error.reason})`; | |
| 275 | case "pull_failed": | |
| 276 | return `the change could not be opened (${error.reason})`; | |
| 277 | } | |
| 278 | } | |
| 279 | ||
| 280 | 245 | /** |
| 281 | 246 | * POST /:owner/:name/pulls/:number/take-back — open a new change that |
| 282 | 247 | * undoes exactly what a shipped one merged. Never touches the default |
test/mcp-take-back.test.ts added +156
| @@ -0,0 +1,156 @@ | ||
| 1 | /** MCP `take_back` (#7211): the pull page's take it back, through `/mcp` with a real token, in the page's words. */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 8 | import { mergePull, openPull } from "../src/actions/pulls.ts"; | |
| 9 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 10 | import { takeBackWords } from "../src/actions/revert_words.ts"; | |
| 11 | import { getPull, revertOf, 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, type User } from "../src/core/users.ts"; | |
| 15 | import { resolveOid } from "../src/git/merge.ts"; | |
| 16 | import { repoDir } from "../src/git/repos.ts"; | |
| 17 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 18 | import { quietLogs } from "./helpers/globals.ts"; | |
| 19 | ||
| 20 | quietLogs(); | |
| 21 | ||
| 22 | let dataDir: string; | |
| 23 | let app: App; | |
| 24 | let servers: Servers; | |
| 25 | let ada: User; | |
| 26 | let repo: Repo; | |
| 27 | let shipped: Pull; | |
| 28 | let open: Pull; | |
| 29 | const tok: Record<string, string> = {}; | |
| 30 | ||
| 31 | const ok = <T>(r: { ok: true; value: T } | { ok: false; error: unknown }): T => { | |
| 32 | if (!r.ok) throw new Error(JSON.stringify(r.error)); | |
| 33 | return r.value; | |
| 34 | }; | |
| 35 | ||
| 36 | async function takeBack(token: string, args: unknown): Promise<{ error?: string; value?: any }> { | |
| 37 | const res = await fetch(`${servers.url}/mcp`, { | |
| 38 | method: "POST", | |
| 39 | headers: { | |
| 40 | "content-type": "application/json", | |
| 41 | accept: "application/json, text/event-stream", | |
| 42 | authorization: `Bearer ${token}`, | |
| 43 | }, | |
| 44 | body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/call", params: { name: "take_back", arguments: args } }), | |
| 45 | }); | |
| 46 | const body: any = await res.json(); | |
| 47 | const out: string = body.result.content[0].text; | |
| 48 | return body.result.isError === true ? { error: out } : { value: JSON.parse(out) }; | |
| 49 | } | |
| 50 | ||
| 51 | async function change(branch: string, content: string): Promise<Pull> { | |
| 52 | ok(await commitToBranch(app, ada, repo, { | |
| 53 | branch, | |
| 54 | base: "main", | |
| 55 | message: branch, | |
| 56 | files: [{ path: "code.txt", content }], | |
| 57 | })); | |
| 58 | return ok(await openPull(app, ada, repo, { title: `Make it ${branch}`, baseBranch: "main", headBranch: branch })); | |
| 59 | } | |
| 60 | ||
| 61 | beforeAll(async () => { | |
| 62 | dataDir = await mkdtemp(join(tmpdir(), "forge-mcp-take-back-")); | |
| 63 | app = createApp({ dataDir, port: 0 }); | |
| 64 | servers = await serve(app); | |
| 65 | const make = (username: string, adminIfFirst = false) => | |
| 66 | createUser(app.db, { | |
| 67 | origin: "seed", | |
| 68 | trusted: true, | |
| 69 | username, | |
| 70 | email: `${username}@forge.local`, | |
| 71 | password: "a-good-passphrase", | |
| 72 | adminIfFirst, | |
| 73 | }); | |
| 74 | ada = ok(await make("ada", true)); | |
| 75 | const sam = ok(await make("sam")); | |
| 76 | repo = ok(await createRepo(app, ada, { name: "svc", visibility: "private" })); | |
| 77 | ok(await commitToBranch(app, ada, repo, { branch: "main", message: "base", files: [{ path: "code.txt", content: "alpha\n" }] })); | |
| 78 | const loud = await change("loud", "alpha\nBETA\n"); | |
| 79 | ok(await mergePull(app, ada, repo, loud, "squash")); | |
| 80 | shipped = getPull(app.db, repo.id, loud.number)!; | |
| 81 | open = await change("quiet", "alpha\ngamma\n"); | |
| 82 | const bot = ok(await createAgent(app, ada, { username: "bot" })); | |
| 83 | ok(addToCrew(app, ada, repo, "bot")); | |
| 84 | tok["ada"] = createToken(app.db, ada.id, "rw", "rw").token; | |
| 85 | tok["ada-ro"] = createToken(app.db, ada.id, "ro", "ro").token; | |
| 86 | tok["bot"] = createToken(app.db, bot.user.id, "rw", "rw").token; | |
| 87 | tok["sam"] = createToken(app.db, sam.id, "rw", "rw").token; | |
| 88 | }, 30_000); | |
| 89 | ||
| 90 | afterAll(async () => { | |
| 91 | servers?.stop(); | |
| 92 | app?.close(); | |
| 93 | await rm(dataDir, { recursive: true, force: true }); | |
| 94 | }); | |
| 95 | ||
| 96 | const dir = () => repoDir(app.reposRoot, repo.ownerName, repo.name); | |
| 97 | ||
| 98 | test("an agent token is refused in the page's words, and nothing is made", async () => { | |
| 99 | const refused = await takeBack(tok["bot"]!, { repo: "ada/svc", number: shipped.number }); | |
| 100 | expect(refused.error).toBe(`forbidden: That could not be taken back: ${takeBackWords("forbidden")}.`); | |
| 101 | expect(await resolveOid(dir(), `refs/heads/revert-${shipped.number}`)).toBeUndefined(); | |
| 102 | expect(revertOf(app.db, repo.id, shipped.id)).toBeUndefined(); | |
| 103 | }); | |
| 104 | ||
| 105 | test("a read-only token cannot take a pull back", async () => { | |
| 106 | const refused = await takeBack(tok["ada-ro"]!, { repo: "ada/svc", number: shipped.number }); | |
| 107 | expect(refused.error).toBe("forbidden: this token may read, not act"); | |
outdated
|
||
| 108 | expect(revertOf(app.db, repo.id, shipped.id)).toBeUndefined(); | |
| 109 | }); | |
| 110 | ||
| 111 | test("an open pull has not shipped", async () => { | |
| 112 | const refused = await takeBack(tok["ada"]!, { repo: "ada/svc", number: open.number }); | |
| 113 | expect(refused.error).toBe(`not_shipped: That could not be taken back: ${takeBackWords("not_shipped")}.`); | |
| 114 | }); | |
| 115 | ||
| 116 | test("a pull the caller cannot see, or no pull at all, is not found", async () => { | |
| 117 | expect((await takeBack(tok["sam"]!, { repo: "ada/svc", number: shipped.number })).error).toStartWith("not_found:"); | |
| 118 | const none = await takeBack(tok["ada"]!, { repo: "ada/svc", number: 999 }); | |
| 119 | expect(none.error).toBe(`not_found: That could not be taken back: ${takeBackWords("not_found")}.`); | |
| 120 | expect((await takeBack(tok["ada"]!, { repo: "ada/svc", number: "1" })).error).toBe( | |
outdated
Same here: pin |
||
| 121 | `not_found: That could not be taken back: ${takeBackWords("not_found")}.`, | |
| 122 | ); | |
| 123 | }); | |
| 124 | ||
| 125 | test("a shipped pull opens the pull that undoes it; a second call is already taken back", async () => { | |
| 126 | const made = await takeBack(tok["ada"]!, { repo: "ada/svc", number: shipped.number }); | |
| 127 | expect(made.error).toBeUndefined(); | |
| 128 | const revert = revertOf(app.db, repo.id, shipped.id)!; | |
| 129 | expect(revert).toBeDefined(); | |
| 130 | expect(made.value).toMatchObject({ | |
| 131 | number: revert.number, | |
| 132 | title: 'Revert "Make it loud"', | |
| 133 | }); | |
| 134 | expect(made.value.url).toEndWith(`/ada/svc/pulls/${revert.number}`); | |
| 135 | expect(made.value.says).toBe('Opened "Revert "Make it loud"" — ship it to take "Make it loud" back.'); | |
| 136 | expect(revert.headBranch).toBe(`revert-${shipped.number}`); | |
| 137 | expect(revert.state).toBe("open"); | |
| 138 | ||
| 139 | const again = await takeBack(tok["ada"]!, { repo: "ada/svc", number: shipped.number }); | |
| 140 | expect(again.error).toBe(`already_reverted: That could not be taken back: ${takeBackWords("already_reverted")}.`); | |
| 141 | }); | |
| 142 | ||
| 143 | test("a revert that no longer applies is a conflict, and leaves nothing behind", async () => { | |
| 144 | const ship = async (branch: string, content: string): Promise<Pull> => { | |
| 145 | const made = await change(branch, content); | |
| 146 | ok(await mergePull(app, ada, repo, made, "squash")); | |
| 147 | return getPull(app.db, repo.id, made.number)!; | |
| 148 | }; | |
| 149 | // `later` rewrites the very line `earlier` wrote, so undoing `earlier` cannot apply. | |
| 150 | const earlier = await ship("gamma", "alpha\nGAMMA\n"); | |
| 151 | await ship("delta", "alpha\nDELTA\n"); | |
| 152 | const refused = await takeBack(tok["ada"]!, { repo: "ada/svc", number: earlier.number }); | |
| 153 | expect(refused.error).toBe("conflict: That could not be taken back: it conflicts with the current code in code.txt."); | |
| 154 | expect(await resolveOid(dir(), `refs/heads/revert-${earlier.number}`)).toBeUndefined(); | |
| 155 | expect(revertOf(app.db, repo.id, earlier.id)).toBeUndefined(); | |
| 156 | }); |
test/mcp.test.ts +1
| @@ -245,6 +245,7 @@ describe("the forge over MCP", () => { | ||
| 245 | 245 | "spawn", |
| 246 | 246 | "forges", |
| 247 | 247 | "claim_link", |
| 248 | "take_back", | |
| 248 | 249 | "take_updates", |
| 249 | 250 | "sync", |
| 250 | 251 | "sever", |
new above
offline
This structured-error branch (
e.kind) is the only mapping that is new to this tool, and no test reaches it. Add aconflictcase that asserts the exact code and words, and that norevert-Nbranch is left behind.