Skip to content

Commit 0cefc9b

Browse files
davidjayanaclaude
authored andcommitted
fix(mcp): compare the whole project file before saving an MCP edit to it
The guard against overwriting a closed project compared only `project.updatedAt`. Two saves inside one millisecond share that stamp, and a writer outside the app (a sync tool, a restored copy) may not change it, so an MCP edit could still land on top of either. Compare the re-read document in full instead. Raised by CodeRabbit on #1040. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent ba92105 commit 0cefc9b

3 files changed

Lines changed: 31 additions & 3 deletions

File tree

‎electron/mcp/openscreen-mcp-server.test.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// ephemeral port, with an in-memory stand-in for the editor window and a real
33
// DocumentService on a temp directory for the projects that are not open.
44

5-
import { mkdtempSync, rmSync } from "node:fs";
5+
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
66
import os from "node:os";
77
import path from "node:path";
88
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
@@ -520,6 +520,31 @@ describe("projects other than the open one", () => {
520520
expect(onDisk.project.title).toBe("Renamed");
521521
expect(onDisk.timeline.trimRanges).toHaveLength(0);
522522
});
523+
524+
it("does not save over a change that kept the file's updatedAt", async () => {
525+
const saved = await projects.saveProject(fixtureDocument("proj_2"));
526+
const file = path.join(dir, "projects", "proj_2.openscreen");
527+
const getProject = projects.getProject.bind(projects);
528+
let reads = 0;
529+
projects.getProject = async (id) => {
530+
const document = await getProject(id);
531+
if (++reads === 1) {
532+
// A writer outside the app (a sync tool, a restored copy): same stamp, new content.
533+
const renamed = { ...saved, project: { ...saved.project, title: "Renamed" } };
534+
writeFileSync(file, JSON.stringify(renamed));
535+
}
536+
return document;
537+
};
538+
const mcp = await connect(new FakeEditor());
539+
const result = await mcp.callTool({
540+
name: "addTrim",
541+
arguments: { ...addTrimArgs, projectId: "proj_2" },
542+
});
543+
expect(result.isError).toBe(true);
544+
const onDisk = await getProject("proj_2");
545+
expect(onDisk.project.title).toBe("Renamed");
546+
expect(onDisk.timeline.trimRanges).toHaveLength(0);
547+
});
523548
});
524549

525550
describe("the HTTP guard", () => {

‎electron/mcp/openscreen-mcp-server.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,8 +276,11 @@ export function createToolRunner(deps: McpToolDeps) {
276276
if (openProjectId(await deps.host.snapshot()) === projectId) {
277277
return textResult(PROJECT_CHANGED_MESSAGE, true);
278278
}
279+
// The whole document, not just `updatedAt`: two saves inside one
280+
// millisecond share a stamp, and a writer outside the app (a sync tool,
281+
// a restored copy) may not touch it at all.
279282
const onDisk = await deps.projects.getProject(projectId);
280-
if (onDisk.project.updatedAt !== document.project.updatedAt) {
283+
if (JSON.stringify(onDisk) !== JSON.stringify(document)) {
281284
return textResult(PROJECT_CHANGED_MESSAGE, true);
282285
}
283286
await deps.projects.saveProject(execution.document);

‎technical-documentation/architecture/mcp-server.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ Only the webContents that registered on `ai-edition.mcp-host` is asked, and only
5353
Read with `DocumentService.getProject` (migrated and relinked, like any open) and, if the tool changed it, saved with `saveProject` — the same instance every other save in the app goes through, so its per-project write queue still holds. There is no revision to guard with, so right before the save two checks run, and either one refuses the edit with "NOT applied … re-read, then retry":
5454

5555
1. **The editor has not opened it meanwhile.** The editor is asked for a snapshot again; if its project is now this one, the edit is left to it. The client's retry, with the same `projectId`, then goes through the editor.
56-
2. **Nothing else saved it meanwhile.** The file is re-read and its `project.updatedAt` must still be the one the tool ran against.
56+
2. **Nothing else changed it meanwhile.** The file is re-read and must still be, in full, the document the tool ran against. Comparing `project.updatedAt` alone is not enough: two saves inside one millisecond share a stamp, and a writer outside the app (a sync tool, a restored copy) may not touch it.
5757

5858
An edit made this way is saved, but it is not on any undo stack: the editor never held it. It is in the project the next time it is opened.
5959

0 commit comments

Comments
 (0)