Skip to content

Commit 84c74c9

Browse files
committed
fix(mcp): read a named project before asking the editor about it
Reads of a project now queue behind each other as well as behind its saves, and a call naming a project reads the file before taking the editor's snapshot. An editor open already under way has then returned and been installed, so the snapshot shows it and the edit goes through the editor instead of a file the editor is about to overwrite.
1 parent bcc40cc commit 84c74c9

4 files changed

Lines changed: 82 additions & 25 deletions

File tree

‎electron/ai-edition/document-service.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,16 @@ describe("DocumentService", () => {
800800
expect((await service.getProject(doc.project.id)).annotations).toHaveLength(7);
801801
});
802802

803+
it("makes a read wait for a read asked for before it", async () => {
804+
const doc = await service.createProject("Reads in order");
805+
const order: string[] = [];
806+
await Promise.all([
807+
service.getProject(doc.project.id).then(() => order.push("first")),
808+
service.getProject(doc.project.id).then(() => order.push("second")),
809+
]);
810+
expect(order).toEqual(["first", "second"]);
811+
});
812+
803813
it("makes a read wait for a save already queued", async () => {
804814
const doc = await service.createProject("Read after write");
805815
const saving = service.saveProject({ ...doc, project: { ...doc.project, title: "Saved" } });

‎electron/ai-edition/document-service.ts‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class DocumentService {
167167
private readonly projectsRoot: string;
168168
private readonly mediaRegistryDir: string;
169169
private legacyMigrationDone = false;
170-
/** Tail of the in-flight save chain per project id — see writeProject. */
170+
/** Tail of the in-flight read and save chain per project id — see writeProject. */
171171
private readonly writeQueues = new Map<string, Promise<void>>();
172172
/** Bumped as each read or save of a project starts — see saveProjectIfUnchanged. */
173173
private readonly generations = new Map<string, number>();
@@ -307,10 +307,16 @@ export class DocumentService {
307307
projectId: string,
308308
): Promise<{ document: AxcutDocument; version: ProjectVersion }> {
309309
const generation = this.touch(projectId);
310-
// Behind any save already queued for this project, so a read never returns the
311-
// file such a save is about to replace. saveProjectIfUnchanged relies on it.
312-
await this.writeQueues.get(projectId);
313-
const raw = await this.readProjectFile(projectId);
310+
// In the project's queue: behind its saves, so a read never returns the file one
311+
// is about to replace, and behind its other reads, so a read asked for earlier
312+
// has returned first. saveProjectIfUnchanged and the MCP server rely on both.
313+
return this.enqueue(projectId, async () => {
314+
const raw = await this.readProjectFile(projectId);
315+
return { document: await this.parseProjectFile(raw), version: { generation, raw } };
316+
});
317+
}
318+
319+
private async parseProjectFile(raw: string): Promise<AxcutDocument> {
314320
// Relink here rather than in the .openscreen import handlers, because this
315321
// is the one place every open funnels through — the project picker, the
316322
// agent, and the auto-load-last-project effect on launch. A document whose
@@ -326,13 +332,13 @@ export class DocumentService {
326332
);
327333
// AFTER the relink, so what is granted is the path the renderer will actually ask for.
328334
this.onProjectRead?.(document);
329-
return { document, version: { generation, raw } };
335+
return document;
330336
}
331337

332338
/**
333339
* For a read of a project file that bypasses getProject: the open-file dialog,
334340
* which then saves back what it read. If the file is one of this service's
335-
* projects, that counts as a read of it, behind its queued saves, exactly as in
341+
* projects, that counts as a read of it, behind its queued reads and saves, as in
336342
* getProject. Any other file is not this service's business.
337343
*/
338344
async beforeProjectFileRead(filePath: string): Promise<void> {
@@ -557,15 +563,18 @@ export class DocumentService {
557563
doc: AxcutDocument,
558564
precondition?: () => Promise<boolean>,
559565
): Promise<boolean> {
560-
const projectId = doc.project.id;
561-
const tail = this.writeQueues.get(projectId) ?? Promise.resolve();
562-
const write = async () => {
566+
return this.enqueue(doc.project.id, async () => {
563567
if (precondition && !(await precondition())) return false;
564568
await this.writeProjectNow(doc);
565569
return true;
566-
};
570+
});
571+
}
572+
573+
/** Runs `step` once every read and save of the project asked for before it is done. */
574+
private enqueue<T>(projectId: string, step: () => Promise<T>): Promise<T> {
575+
const tail = this.writeQueues.get(projectId) ?? Promise.resolve();
567576
// Chained on both settlements: one save failing must not cancel the next.
568-
const run = tail.then(write, write);
577+
const run = tail.then(step, step);
569578
const settled = run.then(
570579
() => undefined,
571580
() => undefined,

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

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
// DocumentService on a temp directory for the projects that are not open.
44

55
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
6+
import fsPromises from "node:fs/promises";
67
import os from "node:os";
78
import path from "node:path";
89
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
910
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
10-
import { afterEach, beforeEach, describe, expect, it } from "vitest";
11+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
1112
import {
1213
type AxcutDocument,
1314
createEmptyDocument,
@@ -564,6 +565,35 @@ describe("projects other than the open one", () => {
564565
expect(editor.applied).toHaveLength(0);
565566
});
566567

568+
// The editor was already reading the file when the call came in, and installs what
569+
// it read as soon as that read returns.
570+
it("waits out an editor open already under way, then edits through the editor", async () => {
571+
await projects.saveProject(fixtureDocument("proj_2"));
572+
const editor = new FakeEditor();
573+
const mcp = await connect(editor);
574+
await checkpoint(mcp, "proj_2");
575+
const readFile = fsPromises.readFile;
576+
const slowRead = vi.spyOn(fsPromises, "readFile").mockImplementationOnce((async (
577+
...args: Parameters<typeof readFile>
578+
) => {
579+
await new Promise((resolve) => setTimeout(resolve, 100));
580+
return readFile(...args);
581+
}) as typeof readFile);
582+
const opening = projects.getProject("proj_2").then((document) => {
583+
editor.document = document;
584+
});
585+
const result = await mcp.callTool({
586+
name: "addTrim",
587+
arguments: { ...addTrimArgs, projectId: "proj_2" },
588+
});
589+
await opening;
590+
slowRead.mockRestore();
591+
expect(result.isError).toBeFalsy();
592+
// The editor holds the project now, so the edit has to be in its copy.
593+
expect(editor.applied).toHaveLength(1);
594+
expect(editor.document?.timeline.trimRanges).toHaveLength(1);
595+
});
596+
567597
// The editor's open has read the file but not yet installed it, so it does not
568598
// show in a snapshot: only the read itself gives it away.
569599
it("does not save over a project the editor is opening mid-call", async () => {

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

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ export interface McpProjectStore {
9090
): Promise<AxcutDocument | null>;
9191
}
9292

93+
type ProjectRead = Awaited<ReturnType<McpProjectStore["getProjectForUpdate"]>>;
94+
9395
export interface McpToolDeps {
9496
host: McpDocumentHost;
9597
projects: McpProjectStore;
@@ -276,19 +278,10 @@ export function createToolRunner(deps: McpToolDeps) {
276278
/** Any other project: read from and saved to its file. */
277279
async function runOnFile(
278280
projectId: string,
281+
read: ProjectRead,
279282
name: string,
280283
args: unknown,
281284
): Promise<CallToolResult> {
282-
const read = await deps.projects.getProjectForUpdate(projectId).catch((error) => {
283-
if (error instanceof DocumentNotFoundError) return null;
284-
throw error;
285-
});
286-
if (!read) {
287-
return textResult(
288-
`No project has the id "${projectId}". Call listProjects for the valid ids.`,
289-
true,
290-
);
291-
}
292285
const execution = await execute(read.document, name, args);
293286
if (execution.document) {
294287
// A restore IS the way back, so it needs no checkpoint of its own.
@@ -322,14 +315,29 @@ export function createToolRunner(deps: McpToolDeps) {
322315
async function run(name: string, rawArgs: unknown): Promise<CallToolResult> {
323316
if (name === LIST_PROJECTS_TOOL) return listProjects();
324317
const { projectId, args } = splitProjectId(rawArgs);
325-
const snapshot = await deps.host.snapshot();
326318
if (projectId === undefined) {
319+
const snapshot = await deps.host.snapshot();
327320
return snapshot ? runOnEditor(snapshot, name, args) : textResult(NO_PROJECT_MESSAGE, true);
328321
}
322+
// The file first, the editor second. Reads of a project queue behind each other,
323+
// so an editor open of it already under way has returned by now, and the editor
324+
// installs what it read before it answers the snapshot: either the snapshot shows
325+
// the project, or saveProjectIfUnchanged sees any open that starts after this read.
326+
const read = await deps.projects.getProjectForUpdate(projectId).catch((error) => {
327+
if (error instanceof DocumentNotFoundError) return null;
328+
throw error;
329+
});
330+
const snapshot = await deps.host.snapshot();
329331
if (snapshot && openProjectId(snapshot) === projectId) {
330332
return runOnEditor(snapshot, name, args);
331333
}
332-
return runOnFile(projectId, name, args);
334+
if (!read) {
335+
return textResult(
336+
`No project has the id "${projectId}". Call listProjects for the valid ids.`,
337+
true,
338+
);
339+
}
340+
return runOnFile(projectId, read, name, args);
333341
}
334342

335343
return (name: string, args: unknown): Promise<CallToolResult> => {

0 commit comments

Comments
 (0)