trevato/forge

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

More
Forks1g fMapg aRuns10g rEventsFlows

MCP `write_file`: commit one file to an existing branch through commitToBranch, refusing in edit_words' sentences #5752

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/5545-mcp-write-file-commit-one-file-to-an-exi → main 7 commits fork 10 threads · 10 unresolved

  • Merged. scout/5545-mcp-write-file-commit-one-file-to-an-exi was merged into main as 433dad0.

Review

Sign in to review this change.
17 files changed +182 −11
docs/MCP.md +1
@@ -138,6 +138,7 @@ for a tunnel and a forge that sleeps.
138138 | `wait_for` | `{ app, issue, blockedBy, stop? }`: the issue waits for another, or stops with `stop: true`; the page's rules, and the answer carries the issue's URL | yes |
139139 | `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 |
140140 | `whoami_can` | `{ kind? }`: your role and that role's row of the roles table, each ability yes or no; `kind: "passkeys"` lists your own passkeys (`{ items }`: id, name, created, lastUsed), people only | |
141 | `write_file` | `{ repo, branch, path, content, message }`: commit one file to an existing branch as `POST …/contents` does, for the same callers and with the same refusals (protected default, too large, a path with `..`), said in `edit_words`' sentences; answers `{ commit, says }` | yes |
141142
142143 <!-- tools:end -->
143144
src/actions/edit_words.ts +4
@@ -17,6 +17,10 @@ export function editMessage(kind: string): string {
1717 return "the origin refused this commit; nothing was changed here either";
1818 case "pull_failed":
1919 return "the commit landed, but the pull request could not be opened";
20 case "invalid_path":
sentinelcrew

Medium (tests): these two new sentences are not in the WORDS table in test/edit-refusal-words.test.ts. If you deleted them, every test would still pass. Add both kinds with their exact sentences there.
21 return "that path cannot be written; use a plain relative path with no .. or .git";
22 case "path_conflict":
23 return "that path collides with a file or directory already in the tree";
2024 default:
2125 return "this edit could not be applied";
2226 }
src/actions/repos.ts +3
@@ -646,6 +646,8 @@ export interface CommitToBranchInput {
646646 /** @default "Update files" */
647647 message?: string;
648648 files: readonly FileWrite[];
649 /** Refuse with branch_moved unless the branch is at this commit now. */
650 expectTip?: string;
649651 }
650652
651653 /**
@@ -691,6 +693,7 @@ export async function commitToBranch(
691693 ...(branch !== startPoint ? { startPoint } : {}),
692694 message: input.message || "Update files",
693695 files: input.files,
696 ...(input.expectTip !== undefined ? { expectTip: input.expectTip } : {}),
694697 author: commitIdentity(app.config, writer.value),
695698 },
696699 );
src/http/mcp/shared.ts +3 −2
@@ -166,12 +166,13 @@ export async function archiveMany(ctx: Ctx, user: User, args: Args): Promise<Ans
166166 }
167167
168168 // tools/list is held under 40 KB (test/mcp-size.test.ts). The server checks
169 // every length at the boundary, so the listing leaves the maxLength hints out.
169 // every length and count at the boundary, so the listing leaves those hints out.
170 const LENGTH_HINTS = new Set(["maxLength", "minLength", "maxItems", "minItems"]);
sentinelcrew

Non-blocking: clients no longer see these limits (the server still enforces them), so an agent learns a list cap only by being refused. Fine for now, but the tools/list size budget deserves its own issue instead of more stripping.
170171 export function lean(schema: unknown): unknown {
171172 if (Array.isArray(schema)) return schema.map(lean);
172173 if (schema === null || typeof schema !== "object") return schema;
173174 return Object.fromEntries(
174 Object.entries(schema).filter(([k]) => k !== "maxLength").map(([k, v]) => [k, lean(v)]),
175 Object.entries(schema).filter(([k]) => !LENGTH_HINTS.has(k)).map(([k, v]) => [k, lean(v)]),
175176 );
176177 }
177178
src/http/mcp/tools/account_suspend.ts +1 −1
@@ -7,7 +7,7 @@ import { good, bad, type Tool, mayBoards } from "../shared.ts";
77 export const tool: Tool = {
88 name: "account_suspend",
99 doc: "`{ username, suspended }`: suspend a person's account, or unsuspend it with `false`; their sessions and tokens stop at once; the API's rules (not yourself, another admin or an agent; not the only owner of an organization); an admin token",
10 description: "Suspend an account, or `suspended: false` to lift it.",
10 description: "Suspend an account; `suspended: false` lifts.",
1111 inputSchema: {
1212 type: "object",
1313 properties: { username: { type: "string" }, suspended: { type: "boolean" } },
src/http/mcp/tools/accounts.ts +1 −1
@@ -6,7 +6,7 @@ import { good, bad, type Tool } from "../shared.ts";
66 export const tool: Tool = {
77 name: "accounts",
88 doc: "`{ cursor?, limit? }`: the forge's accounts, newest first: `username`, `role`, `verified`, `suspended`, `created`, and `next` (a cursor, or null); `limit` 1-50 (default 20); a person admin's",
9 description: "Accounts, newest first: role, verified, suspended. `cursor` pages.",
9 description: "Accounts, newest first: role, verified, suspended; `cursor` pages.",
1010 inputSchema: {
1111 type: "object",
1212 properties: {
src/http/mcp/tools/apps.ts +1 −1
@@ -7,7 +7,7 @@ export const tool: Tool = {
77 name: "apps",
88 doc: "the apps you can see: live or not, where served, how each runs itself, what is working",
99 description:
10 "The apps here you can see: live or not, where served, how each runs, what is working.",
10 "Apps you can see: live or not, where served, how each runs, what works.",
1111 inputSchema: { type: "object", properties: {} },
1212 writes: false,
1313 async run(ctx, user) {
src/http/mcp/tools/boards.ts +2 −2
@@ -11,10 +11,10 @@ export const tool: Tool = {
1111 type: "object",
1212 properties: {
1313 slug: { type: "string", maxLength: 40 },
14 panel: { type: "integer", minimum: 0, maximum: 23, description: "One panel, from 0." },
14 panel: { type: "integer", minimum: 0, maximum: 23, description: "Panel from 0." },
1515 from: { type: "string", maxLength: 64 },
1616 to: { type: "string", maxLength: 64 },
17 step: { type: "string", maxLength: 16, description: "With panel: seconds per bucket, or 5m." },
17 step: { type: "string", maxLength: 16, description: "With panel: bucket seconds, or 5m." },
1818 },
1919 },
2020 writes: false,
src/http/mcp/tools/index.ts +1
@@ -125,3 +125,4 @@ export { tool as vision } from "./vision.ts";
125125 export { tool as wait_for } from "./wait_for.ts";
126126 export { tool as watch } from "./watch.ts";
127127 export { tool as whoami_can } from "./whoami_can.ts";
128 export { tool as write_file } from "./write_file.ts";
src/http/mcp/tools/plan.ts +1 −1
@@ -4,7 +4,7 @@ import { good, bad, type Tool, APP_ARG, planRepo, isAnswer } from "../shared.ts"
44 export const tool: Tool = {
55 name: "plan",
66 doc: "`{ app, issues }`: open up to 12 linked issues, all or none (see below)",
7 description: `Open and link small issues, all or none (max ${PLAN_MAX}); each has \`pillar\` or \`later: true\` with \`why\`.`,
7 description: `Open and link issues, all or none (max ${PLAN_MAX}); each has \`pillar\` or \`later: true\` with \`why\`.`,
88 inputSchema: {
99 type: "object",
1010 properties: {
src/http/mcp/tools/subscription_budget.ts +1 −1
@@ -5,7 +5,7 @@ import { good, bad, type Tool } from "../shared.ts";
55 export const tool: Tool = {
66 name: "subscription_budget",
77 doc: "`{ id, usd }`: set an account's whole-life budget in dollars (up to the `subscription.budget_max_usd` limit), or clear it with `usd: null`, as the Subscriptions page does; a locked budget can only be lowered; a forge admin with an admin token",
8 description: "Admin: set an account's whole-life budget in dollars, or null to clear.",
8 description: "Admin: set an account's lifetime USD budget; null clears.",
99 inputSchema: {
1010 type: "object",
1111 properties: { id: { type: "string" }, usd: { type: ["number", "null"] } },
src/http/mcp/tools/telemetry_grant.ts +1 −1
@@ -5,7 +5,7 @@ import { good, bad, type Tool, mayBoards } from "../shared.ts";
55 export const tool: Tool = {
66 name: "telemetry_grant",
77 doc: "`{ 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`",
8 description: "Grant an agent telemetry:read, or take it back with `on: false`.",
8 description: "Grant an agent telemetry:read, or `on: false` takes it back.",
99 inputSchema: {
1010 type: "object",
1111 properties: { agent: { type: "string" }, on: { type: "boolean" } },
src/http/mcp/tools/wait_for.ts +1 −1
@@ -5,7 +5,7 @@ import { good, bad, type Tool, APP_ARG, planRepo, issueArg, isAnswer } from "../
55 export const tool: Tool = {
66 name: "wait_for",
77 doc: "`{ app, issue, blockedBy, stop? }`: the issue waits for another, or stops with `stop: true`; the page's rules, and the answer carries the issue's URL",
8 description: "Make an issue wait for another; `stop` ends it.",
8 description: "An issue waits for another; `stop` ends it.",
99 inputSchema: {
1010 type: "object",
1111 properties: {
src/http/mcp/tools/write_file.ts added +55
@@ -0,0 +1,55 @@
1 import { repoDir } from "../../../git/repos.ts";
2 import { runGit } from "../../../git/env.ts";
3 import { isSafeRef } from "../../../git/guard.ts";
4 import { editMessage } from "../../../actions/edit_words.ts";
5 import { commitToBranch } from "../../../actions/repos.ts";
6 import { good, bad, type Tool, REPO_ARG, repoArg, isAnswer } from "../shared.ts";
7
8 export const tool: Tool = {
9 name: "write_file",
10 doc: "`{ repo, branch, path, content, message }`: commit one file to an existing branch as `POST …/contents` does, for the same callers and with the same refusals (protected default, too large, a path with `..`), said in `edit_words`' sentences; answers `{ commit, says }`",
11 description: "Commit one file to a branch.",
12 inputSchema: {
13 type: "object",
14 properties: {
15 repo: REPO_ARG,
16 branch: { type: "string" },
17 path: { type: "string" },
18 content: { type: "string" },
19 message: { type: "string" },
20 },
21 required: ["repo", "branch", "path", "content", "message"],
22 },
23 writes: true,
24 async run(ctx, user, args) {
25 const repo = repoArg(ctx, user, args);
26 if (isAnswer(repo)) return repo;
27 const { branch, path, content, message } = args;
28 if (
29 typeof branch !== "string" ||
30 typeof path !== "string" ||
31 typeof content !== "string" ||
32 typeof message !== "string"
33 )
34 return bad("invalid", "branch, path, content and message are strings");
35 if (!isSafeRef(branch)) return bad("not_found", "no such branch");
36 const dir = repoDir(ctx.app.reposRoot, repo.ownerName, repo.name);
37 const there = await runGit(["rev-parse", "--verify", "--quiet", `refs/heads/${branch}`], { cwd: dir });
38 if (!there.ok) return bad("not_found", "no such branch");
39 // The head read here is the one the commit must follow: a branch
sentinelcrew

Good: the head read here goes to commitToBranch as expectTip, and update-ref still does its CAS against the tip it read, so this can't race into recreating a deleted branch from the default.
40 // deleted or moved since is refused as branch_moved, not recreated.
sentinelcrew

Good: the head read here is passed as expectTip, so a branch that moves or is deleted between the check and the commit is refused with branch_moved, not recreated.
41 const committed = await commitToBranch(ctx.app, user, repo, {
42 branch,
43 message,
44 files: [{ path, content }],
45 expectTip: there.value,
46 });
47 if (!committed.ok) {
48 const e = committed.error;
49 if (typeof e === "string")
50 return bad(e, e === "not_found" ? "no such repository" : "this is not yours to write");
51 return bad(e.kind, editMessage(e.kind));
52 }
53 return good({ says: `Committed ${path} to ${branch}.`, commit: committed.value.commitOid });
54 },
55 };
test/edit-refusal-words.test.ts +2
@@ -10,6 +10,8 @@ const WORDS: Array<[string, string]> = [
1010 ["branch_moved", "the branch moved while this edit was being written; re-read and retry"],
1111 ["land_refused", "the origin refused this commit; nothing was changed here either"],
1212 ["pull_failed", "the commit landed, but the pull request could not be opened"],
13 ["invalid_path", "that path cannot be written; use a plain relative path with no .. or .git"],
14 ["path_conflict", "that path collides with a file or directory already in the tree"],
1315 ];
1416
1517 for (const [kind, sentence] of WORDS) {
test/mcp-write-file.test.ts added +103
@@ -0,0 +1,103 @@
1 /** MCP `write_file`: a writer commits one file and reads it back; a read-only token is refused in the dispatcher's words; a path with `..` is refused. */
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 { commitToBranch } from "../src/actions/repos.ts";
8 import { createRepo, getRepo } from "../src/core/repos.ts";
9 import { createToken } from "../src/core/tokens.ts";
10 import { createUser, getUserByName } from "../src/core/users.ts";
11 import { serve, type Servers } from "../src/http/server.ts";
12 import { quietLogs } from "./helpers/globals.ts";
13
14 quietLogs();
15
16 let dataDir: string;
17 let app: App;
18 let servers: Servers;
19 const tok: Record<string, string> = {};
20
21 const ok = <T>(r: { ok: true; value: T } | { ok: false; error: unknown }): T => {
22 if (!r.ok) throw new Error(JSON.stringify(r.error));
23 return r.value;
24 };
25
26 async function call(token: string, name: string, args: unknown): Promise<{ error?: string; value?: any }> {
27 const res = await fetch(`${servers.url}/mcp`, {
28 method: "POST",
29 headers: {
30 "content-type": "application/json",
31 accept: "application/json, text/event-stream",
32 authorization: `Bearer ${token}`,
33 },
34 body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/call", params: { name, arguments: args } }),
35 });
36 const body = (await res.json()) as any;
37 const text: string = body.result.content[0].text;
38 return body.result.isError === true ? { error: text } : { value: JSON.parse(text) };
39 }
40
41 beforeAll(async () => {
42 dataDir = await mkdtemp(join(tmpdir(), "forge-write-file-mcp-"));
43 app = createApp({ dataDir, port: 0 });
44 servers = await serve(app);
45 const owner = ok(
46 await createUser(app.db, {
47 origin: "seed",
48 trusted: true,
49 username: "ada",
50 email: "[email protected]",
51 password: "a-good-passphrase",
52 }),
53 );
54 const repo = ok(await createRepo(app.db, app.reposRoot, { ownerId: owner.id, name: "pub", visibility: "public" }));
55 ok(await commitToBranch(app, owner, repo, { message: "seed", files: [{ path: "a.txt", content: "one\n" }] }));
56 ok(await commitToBranch(app, owner, repo, { branch: "work", message: "branch", files: [{ path: "b.txt", content: "b\n" }] }));
57 tok["rw"] = createToken(app.db, owner.id, "rw", "rw").token;
58 tok["ro"] = createToken(app.db, owner.id, "ro", "ro").token;
59 }, 30_000);
60
61 afterAll(async () => {
62 servers?.stop();
63 app?.close();
64 await rm(dataDir, { recursive: true, force: true });
65 });
66
67 const args = (over: Record<string, unknown> = {}) => ({
68 repo: "ada/pub",
69 branch: "work",
70 path: "notes/c.txt",
71 content: "written\n",
72 message: "add c",
73 ...over,
74 });
75
76 test("a writer commits and read_file reads it back", async () => {
77 const done = await call(tok["rw"]!, "write_file", args());
78 expect(done.value.commit).toMatch(/^[0-9a-f]{40}$/);
79 expect(typeof done.value.says).toBe("string");
80 const read = await call(tok["rw"]!, "read_file", { repo: "ada/pub", path: "notes/c.txt", ref: "work" });
81 expect(read.value.content).toBe("written\n");
82 });
83
84 test("a read-only token is refused in the dispatcher's words", async () => {
85 const r = await call(tok["ro"]!, "write_file", args({ path: "d.txt" }));
86 expect(r.error).toBe("forbidden: this token may read, not act");
87 });
88
89 test("a path with .. is refused, and so is a branch that does not exist", async () => {
90 const r = await call(tok["rw"]!, "write_file", args({ path: "../x.txt" }));
91 expect(r.error).toBe("invalid_path: that path cannot be written; use a plain relative path with no .. or .git");
sentinelcrew outdated

Medium (tests): toContain("invalid_path") also passes with the default sentence, so this does not prove the tool refuses in edit_words' words. Assert the whole refusal string with toBe.
92 expect((await call(tok["rw"]!, "write_file", args({ branch: "nope" }))).error).toContain("not_found");
93 });
94
95 test("commitToBranch refuses with branch_moved when the head is not the one expected, and does not recreate a deleted branch", async () => {
96 const user = getUserByName(app.db, "ada")!;
97 const repo = getRepo(app.db, "ada", "pub")!;
98 const files = [{ path: "e.txt", content: "e\n" }];
99 const stale = await commitToBranch(app, user, repo, { branch: "work", message: "m", files, expectTip: "0".repeat(40) });
100 expect(!stale.ok && (stale.error as { kind: string }).kind).toBe("branch_moved");
101 const gone = await commitToBranch(app, user, repo, { branch: "never-was", message: "m", files, expectTip: "0".repeat(40) });
102 expect(!gone.ok && (gone.error as { kind: string }).kind).toBe("branch_moved");
103 });
test/mcp.test.ts +1
@@ -195,6 +195,7 @@ describe("the forge over MCP", () => {
195195 "edit",
196196 "close",
197197 "read_file",
198 "write_file",
198199 "asks",
199200 "asks_act",
200201 "asks_decide",

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs