Return a compact success payload from MCP mutation tools (#13257)

* fix: Return a compact success payload from MCP mutation tools

Mutation tools serialized the entire resource — and for documents the
full markdown body — back to the client on every write, which the caller
already has. They now acknowledge the write with the identifying fields
needed to reference the resource.

The one exception is update_document with editMode "patch", which still
echoes the resulting content so the caller can verify what was replaced.

For move_document and restore_document the breadcrumb is now resolved
after the transaction commits, so it reflects the new location rather
than the cached pre-move document structure.

* test: Update MCP scope enforcement tests for the new response shape

* test: Assert the success acknowledgement across all mutation tool variants
This commit is contained in:
Tom Moor
2026-08-02 15:06:25 -04:00
committed by GitHub
parent d1ed467ba0
commit 03ec96fd91
7 changed files with 189 additions and 248 deletions
+2 -2
View File
@@ -294,7 +294,7 @@ describe("POST /mcp/", () => {
});
expect(res?.result?.isError).toBeUndefined();
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.document.title).toEqual("Created Document");
expect(data.title).toEqual("Created Document");
});
it("create-scoped token does not have update_document tool", async () => {
@@ -351,7 +351,7 @@ describe("POST /mcp/", () => {
accessToken,
"update_document",
{
id: created.document.id,
id: created.id,
title: "Updated by Write Token",
}
);
+14 -5
View File
@@ -1,3 +1,4 @@
import { Collection } from "@server/models";
import { buildCollection, buildUser } from "@server/test/factories";
import { getTestServer } from "@server/test/support";
import {
@@ -56,14 +57,18 @@ describe("collection tools", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.success).toBe(true);
expect(data.name).toEqual("Test Collection");
expect(data.description).toEqual("A **test** description");
expect(data.data).toBeUndefined();
expect(data.icon).toEqual("rocket");
expect(data.color).toEqual("#FF0000");
expect(data.id).toBeDefined();
expect(data.url).toMatch(/^https?:\/\//);
expect(data.permission).toEqual(null);
const collection = await Collection.findByPk(data.id, {
rejectOnEmpty: true,
});
expect(collection.description).toEqual("A **test** description");
expect(collection.icon).toEqual("rocket");
expect(collection.color).toEqual("#FF0000");
expect(collection.permission).toEqual(null);
});
it("update_collection updates fields on existing collection", async () => {
@@ -80,8 +85,12 @@ describe("collection tools", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.success).toBe(true);
expect(data.name).toEqual("Updated Name");
expect(data.url).toMatch(/^https?:\/\//);
await collection.reload();
expect(collection.description).toEqual("Updated description");
});
it("update_collection errors when no fields are provided to update", async () => {
+15 -19
View File
@@ -13,7 +13,6 @@ import {
error,
getActorFromContext,
buildAPIContext,
getPublicShareUrlForCollection,
getPublicShareUrlsForCollections,
optionalString,
pathToUrl,
@@ -209,16 +208,14 @@ export function collectionTools(server: McpServer, scopes: string[]) {
await collection.saveWithCtx(ctx);
const reloaded = await Collection.findByPk(collection.id, {
userId: user.id,
rejectOnEmpty: true,
return success({
success: true,
...pathToUrl(user.team, {
id: collection.id,
name: collection.name,
url: collection.path,
}),
});
const presented = pathToUrl(
user.team,
await presentCollection(reloaded)
);
return success(presented);
} catch (message) {
return error(message);
}
@@ -298,15 +295,14 @@ export function collectionTools(server: McpServer, scopes: string[]) {
await collection.saveWithCtx(ctx);
const shareUrl = await getPublicShareUrlForCollection(
user.team,
collection.id
);
const presented = {
...pathToUrl(user.team, await presentCollection(collection)),
...(shareUrl !== undefined && { shareUrl }),
};
return success(presented);
return success({
success: true,
...pathToUrl(user.team, {
id: collection.id,
name: collection.name,
url: collection.path,
}),
});
} catch (message) {
return error(message);
}
+11 -69
View File
@@ -1,5 +1,6 @@
import { Scope } from "@shared/types";
import type { ProsemirrorData } from "@shared/types";
import { Comment } from "@server/models";
import {
buildCollection,
buildComment,
@@ -136,10 +137,12 @@ describe("create_comment", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.success).toBe(true);
expect(data.id).toBeDefined();
expect(data.documentId).toEqual(document.id);
expect(data.text).toEqual("This is a **test** comment");
expect(data.data).toBeUndefined();
const comment = await Comment.findByPk(data.id, { rejectOnEmpty: true });
expect(comment.toMarkdown()).toContain("This is a **test** comment");
});
it("creates a reply to an existing comment", async () => {
@@ -165,31 +168,11 @@ describe("create_comment", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.success).toBe(true);
expect(data.id).toBeDefined();
expect(data.parentCommentId).toEqual(parentComment.id);
});
it("includes anchorText in response", async () => {
const { user, accessToken } = await buildOAuthUser();
const collection = await buildCollection({
teamId: user.teamId,
userId: user.id,
});
const document = await buildDocument({
teamId: user.teamId,
userId: user.id,
collectionId: collection.id,
});
const res = await callMcpTool(server, accessToken, "create_comment", {
documentId: document.id,
text: "A new comment",
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
// New comments have no anchor mark in the document, so anchorText is undefined
expect(data.id).toBeDefined();
expect(data.anchorText).toBeUndefined();
const comment = await Comment.findByPk(data.id, { rejectOnEmpty: true });
expect(comment.parentCommentId).toEqual(parentComment.id);
});
});
@@ -216,52 +199,11 @@ describe("update_comment", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.success).toBe(true);
expect(data.id).toEqual(comment.id);
expect(data.text).toContain("Updated comment text");
});
it("includes anchorText in response", async () => {
const { user, accessToken } = await buildOAuthUser();
const collection = await buildCollection({
teamId: user.teamId,
userId: user.id,
});
const document = await buildDocument({
teamId: user.teamId,
userId: user.id,
collectionId: collection.id,
});
const comment = await buildComment({
userId: user.id,
documentId: document.id,
});
const anchorText = "anchored content";
const content = {
type: "doc",
content: [
{
type: "paragraph",
content: [
{
type: "text",
text: anchorText,
marks: [buildCommentMark({ id: comment.id, userId: user.id })],
},
],
},
],
} as ProsemirrorData;
await document.update({ content });
const res = await callMcpTool(server, accessToken, "update_comment", {
id: comment.id,
text: "Updated text",
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.id).toEqual(comment.id);
expect(data.anchorText).toEqual(anchorText);
await comment.reload();
expect(comment.toMarkdown()).toContain("Updated comment text");
});
it("errors when no fields are provided to update", async () => {
+11 -19
View File
@@ -4,7 +4,6 @@ import { Op, Transaction } from "sequelize";
import type { FindOptions, WhereOptions } from "sequelize";
import { sequelize } from "@server/storage/database";
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js";
import { CommentStatusFilter } from "@shared/types";
import type { CommentMark } from "@shared/utils/ProsemirrorHelper";
import { commentParser } from "@server/editor";
@@ -323,25 +322,20 @@ export function commentTools(server: McpServer, scopes: string[]) {
});
}
const created = await Comment.createWithCtx(ctx, {
return Comment.createWithCtx(ctx, {
id: commentId,
data,
createdById: user.id,
documentId,
parentCommentId,
});
created.createdBy = user;
created.document = document!;
return created;
});
const presented = presentCommentWithText(comment);
return {
content: [
{ type: "text" as const, text: JSON.stringify(presented) },
],
} satisfies CallToolResult;
return success({
success: true,
id: comment.id,
documentId: comment.documentId,
});
} catch (err) {
return error(err);
}
@@ -422,13 +416,11 @@ export function commentTools(server: McpServer, scopes: string[]) {
await comment.saveWithCtx(ctx, status ? { silent: true } : undefined);
comment.document = document!;
const presented = presentCommentWithText(comment);
return {
content: [
{ type: "text" as const, text: JSON.stringify(presented) },
],
} satisfies CallToolResult;
return success({
success: true,
id: comment.id,
documentId: comment.documentId,
});
} catch (err) {
return error(err);
}
+78 -40
View File
@@ -310,10 +310,13 @@ describe("create_document", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.document.title).toEqual("New Document");
expect(data.document.collectionId).toEqual(collection.id);
expect(data.document.id).toBeDefined();
expect(data.document.url).toMatch(/^https?:\/\//);
expect(data.success).toBe(true);
expect(data.title).toEqual("New Document");
expect(data.id).toBeDefined();
expect(data.url).toMatch(/^https?:\/\//);
const document = await Document.findByPk(data.id, { rejectOnEmpty: true });
expect(document.collectionId).toEqual(collection.id);
});
it("creates from HTML and preserves images as attachments", async () => {
@@ -341,13 +344,14 @@ describe("create_document", () => {
});
expect(res?.result?.isError).toBeUndefined();
expect(data.document.title).toEqual("HTML Document");
expect(data.document.collectionId).toEqual(collection.id);
expect(res?.result?.content?.[1]?.text).toContain("Hello **HTML**");
expect(res?.result?.content?.[1]?.text).toContain(
"/api/attachments.redirect?id="
);
expect(data.success).toBe(true);
expect(data.title).toEqual("HTML Document");
expect(attachmentCount).toEqual(1);
const document = await Document.findByPk(data.id, { rejectOnEmpty: true });
expect(document.collectionId).toEqual(collection.id);
expect(document.text).toContain("Hello **HTML**");
expect(document.text).toContain("/api/attachments.redirect?id=");
});
it("creates nested under parent document", async () => {
@@ -369,8 +373,11 @@ describe("create_document", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.document.title).toEqual("Child Document");
expect(data.document.parentDocumentId).toEqual(parent.id);
expect(data.success).toBe(true);
expect(data.title).toEqual("Child Document");
const document = await Document.findByPk(data.id, { rejectOnEmpty: true });
expect(document.parentDocumentId).toEqual(parent.id);
});
it("creates from a template", async () => {
@@ -392,12 +399,14 @@ describe("create_document", () => {
templateId: template.id,
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
const text = res?.result?.content?.[1]?.text ?? "";
expect(res?.result?.isError).not.toBe(true);
expect(data.document.title).toEqual("From Template");
expect(data.document.templateId).toEqual(template.id);
expect(text).toContain("Content from the template");
expect(data.success).toBe(true);
expect(data.title).toEqual("From Template");
const document = await Document.findByPk(data.id, { rejectOnEmpty: true });
expect(document.templateId).toEqual(template.id);
expect(document.text).toContain("Content from the template");
});
it("defaults the title to the template title", async () => {
@@ -420,7 +429,8 @@ describe("create_document", () => {
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(res?.result?.isError).not.toBe(true);
expect(data.document.title).toEqual("Template Title");
expect(data.success).toBe(true);
expect(data.title).toEqual("Template Title");
});
it("does not allow creating from a template the user cannot access", async () => {
@@ -525,8 +535,37 @@ describe("update_document", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.document.title).toEqual("Updated Title");
expect(data.document.url).toMatch(/^https?:\/\//);
expect(data.success).toBe(true);
expect(data.title).toEqual("Updated Title");
expect(data.url).toMatch(/^https?:\/\//);
expect(res?.result?.content?.length).toEqual(1);
await document.reload();
expect(document.text).toContain("Updated content");
});
it("returns the resulting content when patching", async () => {
const { user, accessToken } = await buildOAuthUser();
const collection = await buildCollection({
teamId: user.teamId,
userId: user.id,
});
const document = await buildDocument({
teamId: user.teamId,
userId: user.id,
collectionId: collection.id,
text: "the original sentence",
});
const res = await callMcpTool(server, accessToken, "update_document", {
id: document.id,
editMode: "patch",
findText: "original",
text: "patched",
});
expect(res?.result?.isError).toBeUndefined();
expect(res?.result?.content?.[1]?.text).toContain("the patched sentence");
});
it("errors when no fields are provided to update", async () => {
@@ -596,7 +635,8 @@ describe("update_document", () => {
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(data.document.id).toEqual(document.id);
expect(data.success).toBe(true);
expect(data.id).toEqual(document.id);
expect(res?.result?.isError).toBeUndefined();
});
@@ -683,16 +723,15 @@ describe("move_document", () => {
id: document.id,
collectionId: collection2.id,
});
const data = (res?.result?.content ?? []).map((c: { text: string }) =>
JSON.parse(c.text)
);
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(res?.result?.isError).toBeUndefined();
const moved = data.find(
(d: { document: { id: string } }) => d.document.id === document.id
) as { document: { collectionId: string } };
expect(moved).toBeDefined();
expect(moved.document.collectionId).toEqual(collection2.id);
expect(data.success).toBe(true);
expect(data.id).toEqual(document.id);
expect(data.breadcrumb).toEqual(collection2.name);
await document.reload();
expect(document.collectionId).toEqual(collection2.id);
});
it("moves under a parent document", async () => {
@@ -716,16 +755,14 @@ describe("move_document", () => {
id: child.id,
parentDocumentId: parent.id,
});
const data = (res?.result?.content ?? []).map((c: { text: string }) =>
JSON.parse(c.text)
);
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(res?.result?.isError).toBeUndefined();
const moved = data.find(
(d: { document: { id: string } }) => d.document.id === child.id
) as { document: { parentDocumentId: string } };
expect(moved).toBeDefined();
expect(moved.document.parentDocumentId).toEqual(parent.id);
expect(data.success).toBe(true);
expect(data.id).toEqual(child.id);
await child.reload();
expect(child.parentDocumentId).toEqual(parent.id);
});
it("fails without collectionId or parentDocumentId", async () => {
@@ -788,7 +825,8 @@ describe("restore_document", () => {
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(res?.result?.isError).toBeUndefined();
expect(data.document.id).toEqual(document.id);
expect(data.success).toBe(true);
expect(data.id).toEqual(document.id);
const reloaded = await Document.unscoped().findByPk(document.id);
expect(reloaded?.archivedAt).toBeNull();
@@ -840,10 +878,10 @@ describe("restore_document", () => {
id: document.id,
collectionId: destination.id,
});
const data = JSON.parse(res?.result?.content?.[0]?.text ?? "{}");
expect(res?.result?.isError).toBeUndefined();
expect(data.document.collectionId).toEqual(destination.id);
const reloaded = await Document.unscoped().findByPk(document.id);
expect(reloaded?.collectionId).toEqual(destination.id);
});
it("fails when the document is not archived or trashed", async () => {
+58 -94
View File
@@ -33,6 +33,7 @@ import {
pathToUrl,
withTracing,
} from "./util";
import { ValidationError } from "@server/errors";
import { StatusFilter, TextEditMode } from "@shared/types";
import SearchProviderManager from "@server/utils/SearchProviderManager";
@@ -443,29 +444,16 @@ export function documentTools(server: McpServer, scopes: string[]) {
: undefined,
});
const [{ text, ...attributes }, breadcrumb] = await Promise.all([
presentDocument(document, {
includeData: false,
includeText: true,
includeUpdatedAt: true,
const breadcrumb = await getDocumentBreadcrumb(document, user);
return success({
success: true,
...pathToUrl(user.team, {
id: document.id,
title: document.title,
url: document.url,
}),
getDocumentBreadcrumb(document, user),
]);
return {
content: [
{
type: "text" as const,
text: JSON.stringify({
document: pathToUrl(user.team, attributes),
...(breadcrumb !== undefined && { breadcrumb }),
}),
},
{
type: "text" as const,
text: typeof text === "string" ? text : "",
},
],
} satisfies CallToolResult;
...(breadcrumb !== undefined && { breadcrumb }),
});
} catch (message) {
return error(message);
}
@@ -509,7 +497,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
const ctx = buildAPIContext(context);
const { user } = ctx.state.auth;
return await sequelize.transaction(async (transaction) => {
const document = await sequelize.transaction(async (transaction) => {
ctx.state.transaction = transaction;
ctx.context.transaction = transaction;
@@ -525,7 +513,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
if (input.parentDocumentId) {
if (input.parentDocumentId === input.id) {
return error("Cannot nest a document inside itself");
throw ValidationError("Cannot nest a document inside itself");
}
const parent = await Document.findByPk(input.parentDocumentId, {
@@ -538,10 +526,10 @@ export function documentTools(server: McpServer, scopes: string[]) {
collectionId = parent.collectionId!;
if (!parent.publishedAt) {
return error("Cannot move document inside a draft");
throw ValidationError("Cannot move document inside a draft");
}
} else if (!collectionId) {
return error(
throw ValidationError(
"Either collectionId or parentDocumentId is required"
);
} else {
@@ -553,53 +541,26 @@ export function documentTools(server: McpServer, scopes: string[]) {
authorize(user, "updateDocument", collection);
}
const { documents, collections } = await documentMover(ctx, {
await documentMover(ctx, {
document,
collectionId: collectionId ?? null,
parentDocumentId: input.parentDocumentId ?? null,
index: input.index,
});
const indexMap = new Map<string, number>();
for (const col of collections) {
if (col.documentStructure) {
for (const [id, idx] of buildSiblingIndexMap(
col.documentStructure
)) {
indexMap.set(id, idx);
}
}
}
return document;
});
const [breadcrumbs, shareUrls] = await Promise.all([
getBreadcrumbsForDocuments(documents, user),
getPublicShareUrlsForDocuments(
user.team,
documents.map((document) => document.id)
),
]);
const presented = await Promise.all(
documents.map(async (document) => {
const doc = pathToUrl(
user.team,
await presentDocument(document, {
includeData: false,
includeText: false,
})
);
const breadcrumb = breadcrumbs.get(document.id);
const shareUrl = shareUrls.get(document.id);
const siblingIndex = indexMap.get(document.id);
return {
document: doc,
...(breadcrumb !== undefined && { breadcrumb }),
...(shareUrl !== undefined && { shareUrl }),
...(siblingIndex !== undefined && { index: siblingIndex }),
};
})
);
return success(presented);
// Resolved after commit so the breadcrumb reflects the new location.
const breadcrumb = await getDocumentBreadcrumb(document, user);
return success({
success: true,
...pathToUrl(user.team, {
id: document.id,
title: document.title,
url: document.url,
}),
...(breadcrumb !== undefined && { breadcrumb }),
});
} catch (message) {
return error(message);
@@ -715,6 +676,22 @@ export function documentTools(server: McpServer, scopes: string[]) {
}
}
// A patch only rewrites part of the document, so the resulting
// content is echoed back for the caller to verify what was applied.
// Other modes have nothing to report beyond the write succeeding.
if (input.editMode !== TextEditMode.Patch) {
const breadcrumb = await getDocumentBreadcrumb(updated, user);
return success({
success: true,
...pathToUrl(user.team, {
id: updated.id,
title: updated.title,
url: updated.url,
}),
...(breadcrumb !== undefined && { breadcrumb }),
});
}
const [{ text, ...attributes }, breadcrumb, shareUrl] =
await Promise.all([
presentDocument(updated, {
@@ -828,7 +805,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
const ctx = buildAPIContext(context);
const { user } = ctx.state.auth;
return await sequelize.transaction(async (transaction) => {
const document = await sequelize.transaction(async (transaction) => {
ctx.state.transaction = transaction;
ctx.context.transaction = transaction;
@@ -840,37 +817,24 @@ export function documentTools(server: McpServer, scopes: string[]) {
});
if (!document.deletedAt && !document.archivedAt) {
return error("Document is not archived or trashed");
throw ValidationError("Document is not archived or trashed");
}
await documentRestorer(ctx, { document, collectionId });
const [{ text, ...attributes }, breadcrumb, shareUrl] =
await Promise.all([
presentDocument(document, {
includeData: false,
includeText: true,
includeUpdatedAt: true,
}),
getDocumentBreadcrumb(document, user),
getPublicShareUrlForDocument(user.team, document.id),
]);
return {
content: [
{
type: "text" as const,
text: JSON.stringify({
document: pathToUrl(user.team, attributes),
...(breadcrumb !== undefined && { breadcrumb }),
...(shareUrl !== undefined && { shareUrl }),
}),
},
{
type: "text" as const,
text: typeof text === "string" ? text : "",
},
],
} satisfies CallToolResult;
return document;
});
// Resolved after commit so the breadcrumb reflects the new location.
const breadcrumb = await getDocumentBreadcrumb(document, user);
return success({
success: true,
...pathToUrl(user.team, {
id: document.id,
title: document.title,
url: document.url,
}),
...(breadcrumb !== undefined && { breadcrumb }),
});
} catch (message) {
return error(message);