diff --git a/.serena/memories/mcp/core.md b/.serena/memories/mcp/core.md index dbc4b85b92..a13d2a2946 100644 --- a/.serena/memories/mcp/core.md +++ b/.serena/memories/mcp/core.md @@ -136,3 +136,7 @@ For parallel devenvs, prefer same-origin MCP routing: each Penpot instance shoul - The MCP server emits `1008` for a missing `userToken` in multi-user mode, for missing/invalid connection metadata, and to close a connection displaced by a newer one with the same user and session ID. - Session IDs are deterministic per tab and file, so a reconnect reuses its ID while the old socket may be half-open: the newest connection wins (`PluginBridge.displaceConnection`). Across instances, each new connection publishes a session claim on a per-user Redis channel; an instance displaces its local connection only on a foreign claim that arrives after its own claim (channel order, no clocks). Discovery deduplicates by session ID. - A tab rejected with `1008` never reaches `connected`, so the frontend's 60s reconnect watcher (`start-reconnect-watcher` in `app.main.data.workspace.mcp`, started only on `connected`) does not engage; recovery is manual via the MCP menu. + +## Rollback warnings + +- The plugin's `RollbackLog` (`packages/plugin/src/RollbackLog.ts`) holds the `contentrollback` events not yet sent with a task response; `Task.sendResponse` attaches them and acknowledges them once sent, and the server's `unclaimed-rollbacks` message (a response that came after its task timed out) puts them back. With a `RollbackStore` (`LocalStorageRollbackStore` over `penpot.localStorage`, one entry per file) every change is written through, so a reload of the tab or plugin keeps them; storage failures fall back to memory. Plugin tests run with `node --experimental-strip-types`, which rejects TypeScript parameter properties: declare fields explicitly. diff --git a/frontend/src/app/plugins/register.cljs b/frontend/src/app/plugins/register.cljs index 283b11a575..3c919eed50 100644 --- a/frontend/src/app/plugins/register.cljs +++ b/frontend/src/app/plugins/register.cljs @@ -25,7 +25,9 @@ (def mcp-permissions #{"user:read" "library:read" "library:write" "comment:read" "comment:write" "content:read" "content:write" - "allow:global" "manage:projects" "manage:teams" "manage:delete"}) + "allow:global" "manage:projects" "manage:teams" "manage:delete" + ;; Keeps the rollbacks not reported yet across reloads + "allow:localstorage"}) ;; Promise that resolves when plugins runtime is initialized. ;; Lives here to avoid circular dependency: workspace.mcp -> app.plugins -> app.plugins.api -> workspace diff --git a/mcp/packages/common/src/types.ts b/mcp/packages/common/src/types.ts index 42ebf1ff06..278e272951 100644 --- a/mcp/packages/common/src/types.ts +++ b/mcp/packages/common/src/types.ts @@ -29,6 +29,19 @@ export interface PluginTaskResult { rollbacks?: ContentRollbackEvent[]; } +/** + * Message from the MCP server giving the plugin back the rollbacks of a task response that came + * after its task timed out, so that the plugin reports them with its next response. + */ +export interface UnclaimedRollbacksMessage { + type: "unclaimed-rollbacks"; + + /** + * The rollbacks the late response carried, oldest first. + */ + rollbacks: ContentRollbackEvent[]; +} + /** * Rollback of rejected changes in the Penpot workspace. * diff --git a/mcp/packages/plugin/public/manifest.json b/mcp/packages/plugin/public/manifest.json index c67296f1db..3512312eca 100644 --- a/mcp/packages/plugin/public/manifest.json +++ b/mcp/packages/plugin/public/manifest.json @@ -5,5 +5,5 @@ "version": 2, "scope": "global", "description": "This plugin enables interaction with the Penpot MCP server", - "permissions": ["user:read", "content:read", "content:write", "library:read", "library:write", "comment:read", "comment:write"] + "permissions": ["user:read", "content:read", "content:write", "library:read", "library:write", "comment:read", "comment:write", "allow:localstorage"] } diff --git a/mcp/packages/plugin/src/ConnectionMetadata.test.ts b/mcp/packages/plugin/src/ConnectionMetadata.test.ts index 37489a9a9b..4eb072a38d 100644 --- a/mcp/packages/plugin/src/ConnectionMetadata.test.ts +++ b/mcp/packages/plugin/src/ConnectionMetadata.test.ts @@ -163,6 +163,7 @@ test("answers heartbeat requests once initialized, without forwarding them to th */ const pluginModules = { ExecuteCodeTaskHandler: class {}, + LocalStorageRollbackStore: class {}, RollbackLog: class { listen() {} }, diff --git a/mcp/packages/plugin/src/RollbackLog.test.ts b/mcp/packages/plugin/src/RollbackLog.test.ts index 4bfb8f50da..da42e8c548 100644 --- a/mcp/packages/plugin/src/RollbackLog.test.ts +++ b/mcp/packages/plugin/src/RollbackLog.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { RollbackLog } from "./RollbackLog.ts"; +import { LocalStorageRollbackStore, RollbackLog, type RollbackStore } from "./RollbackLog.ts"; const rollback = (id: string) => ({ fileIds: [`file-${id}`], pageIds: [`page-${id}`], shapeIds: [`shape-${id}`] }); @@ -45,3 +45,125 @@ test("keeps the rollbacks recorded after the acknowledged ones", () => { assert.deepEqual(log.pending(), [rollback("b")]); }); + +test("puts returned rollbacks back ahead of the ones recorded since", () => { + const log = new RollbackLog(); + log.record(rollback("a")); + const sent = log.pending(); + log.acknowledge(sent.length); + log.record(rollback("b")); + + log.restore(sent); + + assert.deepEqual(log.pending(), [rollback("a"), rollback("b")]); +}); + +/** + * Rollback store that keeps its events in memory, standing in for the storage that outlives a reload. + */ +class MemoryRollbackStore implements RollbackStore { + events: unknown = []; + + load() { + return this.events as any; + } + + save(events: any) { + this.events = JSON.parse(JSON.stringify(events)); + } +} + +/** + * Plugin local storage held in a map. + */ +function memoryStorage(): { + items: Map; + getItem(key: string): string; + setItem(key: string, value: unknown): void; + removeItem(key: string): void; +} { + const items = new Map(); + return { + items, + getItem: (key) => items.get(key) ?? (null as any), + setItem: (key, value) => void items.set(key, String(value)), + removeItem: (key) => void items.delete(key), + }; +} + +test("keeps unsent rollbacks across a reload", () => { + const store = new MemoryRollbackStore(); + new RollbackLog(store).record(rollback("a")); + + const reloaded = new RollbackLog(store); + + assert.deepEqual(reloaded.pending(), [rollback("a")]); +}); + +test("forgets the stored rollbacks once a response carried them", () => { + const store = new MemoryRollbackStore(); + const log = new RollbackLog(store); + log.record(rollback("a")); + log.record(rollback("b")); + + log.acknowledge(1); + + assert.deepEqual(new RollbackLog(store).pending(), [rollback("b")]); + log.acknowledge(1); + assert.deepEqual(new RollbackLog(store).pending(), []); +}); + +test("stores the rollbacks the server returns", () => { + const store = new MemoryRollbackStore(); + new RollbackLog(store).restore([rollback("a")]); + + assert.deepEqual(new RollbackLog(store).pending(), [rollback("a")]); +}); + +test("keeps rollbacks in memory when the storage fails", () => { + const failing: RollbackStore = { + load: () => { + throw new Error("storage unavailable"); + }, + save: () => { + throw new Error("storage unavailable"); + }, + }; + const log = new RollbackLog(failing); + + assert.doesNotThrow(() => log.record(rollback("a"))); + assert.deepEqual(log.pending(), [rollback("a")]); +}); + +test("stores rollbacks per file in the plugin local storage", () => { + const storage = memoryStorage(); + const fileA = new LocalStorageRollbackStore(storage, "file-a"); + const fileB = new LocalStorageRollbackStore(storage, "file-b"); + + fileA.save([rollback("a")]); + + assert.deepEqual(new LocalStorageRollbackStore(storage, "file-a").load(), [rollback("a")]); + assert.deepEqual(fileB.load(), []); +}); + +test("removes the entry of a file with no rollbacks left", () => { + const storage = memoryStorage(); + const store = new LocalStorageRollbackStore(storage, "file-a"); + store.save([rollback("a")]); + + store.save([]); + + assert.equal(storage.items.size, 0); +}); + +test("ignores a stored entry that is not a rollback list", () => { + const storage = memoryStorage(); + const store = new LocalStorageRollbackStore(storage, "file-a"); + + for (const raw of ["not json", '{"fileIds": []}', '[{"fileIds": "x"}]']) { + storage.items.clear(); + store.save([rollback("a")]); + storage.items.set([...storage.items.keys()][0], raw); + assert.deepEqual(store.load(), [], raw); + } +}); diff --git a/mcp/packages/plugin/src/RollbackLog.ts b/mcp/packages/plugin/src/RollbackLog.ts index 600fcaab72..c4f49d3101 100644 --- a/mcp/packages/plugin/src/RollbackLog.ts +++ b/mcp/packages/plugin/src/RollbackLog.ts @@ -1,13 +1,107 @@ +import type { LocalStorage } from "@penpot/plugin-types"; import type { ContentRollbackEvent } from "../../common/src"; +/** + * Storage of the rollbacks a {@link RollbackLog} has not handed over yet, which outlives a reload + * of the plugin or of the browser tab. + */ +export interface RollbackStore { + /** + * Returns the stored rollbacks, oldest first. + */ + load(): ContentRollbackEvent[]; + + /** + * Replaces the stored rollbacks. + * + * @param events - the rollbacks not handed over yet, oldest first + */ + save(events: ContentRollbackEvent[]): void; +} + +/** + * Rollback store in Penpot's plugin local storage, with one entry per file. + * + * The plugin runs for one open file, and a reload opens the same file again, so the entry of that + * file holds what the reload would otherwise lose. The storage is the browser's, so it is kept + * across reloads but not shared with other browsers. + */ +export class LocalStorageRollbackStore implements RollbackStore { + private static readonly KEY_PREFIX = "rollbacks/"; + + private readonly storage: Pick; + private readonly fileId: string; + + /** + * @param storage - the plugin local storage (`penpot.localStorage`) + * @param fileId - the id of the file the plugin runs for + */ + constructor(storage: Pick, fileId: string) { + this.storage = storage; + this.fileId = fileId; + } + + private get key(): string { + return LocalStorageRollbackStore.KEY_PREFIX + this.fileId; + } + + load(): ContentRollbackEvent[] { + const raw = this.storage.getItem(this.key); + if (!raw) { + return []; + } + try { + const events = JSON.parse(raw); + return Array.isArray(events) && events.every(isRollbackEvent) ? events : []; + } catch { + return []; + } + } + + save(events: ContentRollbackEvent[]): void { + if (events.length === 0) { + this.storage.removeItem(this.key); + } else { + this.storage.setItem(this.key, JSON.stringify(events)); + } + } +} + +/** + * Tells whether the given value has the shape of a rollback event. + * + * @param value - a value read from storage + */ +function isRollbackEvent(value: any): value is ContentRollbackEvent { + const isIdList = (ids: unknown) => Array.isArray(ids) && ids.every((id) => typeof id === "string"); + return ( + typeof value === "object" && + value !== null && + isIdList(value.fileIds) && + isIdList(value.pageIds) && + isIdList(value.shapeIds) + ); +} + /** * Log of the rollbacks of rejected changes that the MCP server has not been told about yet. * * A rollback happens after the task that made the rejected changes has returned (the backend - * rejects a save seconds later), so the log hands its events to the next task response. + * rejects a save seconds later), so the log hands its events to the next task response. With a + * store, the log also keeps its events across a reload, until a response carries them. */ export class RollbackLog { - private events: ContentRollbackEvent[] = []; + private readonly store?: RollbackStore; + private events: ContentRollbackEvent[]; + + /** + * @param store - the storage that keeps the events across reloads; without one, or when it + * fails, the events live in memory only + */ + constructor(store?: RollbackStore) { + this.store = store; + this.events = this.load(); + } /** * Starts recording the rollbacks Penpot reports. @@ -30,11 +124,19 @@ export class RollbackLog { * @param event - the rollback as reported by Penpot */ record(event: ContentRollbackEvent): void { - this.events.push({ - fileIds: [...event.fileIds], - pageIds: [...event.pageIds], - shapeIds: [...event.shapeIds], - }); + this.events.push(copyEvent(event)); + this.save(); + } + + /** + * Puts back rollbacks that a sent response carried but no tool execution received (the + * server had given up on that task), ahead of the ones recorded since. + * + * @param events - the returned rollbacks, oldest first + */ + restore(events: ContentRollbackEvent[]): void { + this.events = [...events.map(copyEvent), ...this.events]; + this.save(); } /** @@ -50,6 +152,40 @@ export class RollbackLog { * @param count - the number of rollbacks the sent response carried */ acknowledge(count: number): void { + if (count === 0) { + return; + } this.events = this.events.slice(count); + this.save(); + } + + private load(): ContentRollbackEvent[] { + try { + return this.store?.load() ?? []; + } catch (error) { + console.warn("Unable to read the stored rollbacks:", error); + return []; + } + } + + private save(): void { + try { + this.store?.save(this.events); + } catch (error) { + console.warn("Unable to store the rollbacks:", error); + } } } + +/** + * Copies a rollback event into a plain object. + * + * @param event - the rollback, possibly a proxy or array-like from Penpot + */ +function copyEvent(event: ContentRollbackEvent): ContentRollbackEvent { + return { + fileIds: [...event.fileIds], + pageIds: [...event.pageIds], + shapeIds: [...event.shapeIds], + }; +} diff --git a/mcp/packages/plugin/src/plugin.ts b/mcp/packages/plugin/src/plugin.ts index aa39253922..f659baa208 100644 --- a/mcp/packages/plugin/src/plugin.ts +++ b/mcp/packages/plugin/src/plugin.ts @@ -2,9 +2,9 @@ import { ExecuteCodeTaskHandler } from "./task-handlers/ExecuteCodeTaskHandler"; import { Task, TaskHandler } from "./TaskHandler"; import { formatTaskError } from "./ErrorUtils"; import { extractMinorVersion, isCompatibleVersion } from "./VersionCompatibility"; -import type { PluginConnectionInit } from "../../common/src"; +import type { ContentRollbackEvent, PluginConnectionInit } from "../../common/src"; import type { WorkspaceContext } from "@penpot/plugin-types"; -import { RollbackLog } from "./RollbackLog"; +import { LocalStorageRollbackStore, RollbackLog, type RollbackStore } from "./RollbackLog"; /** * indicates whether the plugin is running in an environment with the Penpot-integrated remote MCP server @@ -27,10 +27,27 @@ const taskHandlers: TaskHandler[] = [new ExecuteCodeTaskHandler()]; /** * Rollbacks of rejected changes, reported to the MCP server with the next task response. + * The ones not reported yet are kept in the plugin local storage of the open file, so a reload + * of the tab or of the plugin does not lose them. */ -const rollbackLog = new RollbackLog(); +const rollbackLog = new RollbackLog(createRollbackStore()); rollbackLog.listen(); +/** + * Creates the store of the rollbacks not reported yet, for the open file. + * + * @returns the store, or undefined when no file is open or the local storage is unavailable + */ +function createRollbackStore(): RollbackStore | undefined { + try { + const fileId = penpot.currentFile?.id; + return fileId ? new LocalStorageRollbackStore(penpot.localStorage, fileId) : undefined; + } catch (error) { + console.warn("Rollbacks are kept in memory only:", error); + return undefined; + } +} + // Open the plugin UI (main.ts) penpot.ui.open("Penpot MCP Plugin", `?theme=${penpot.theme}`, { width: 236, @@ -40,7 +57,16 @@ penpot.ui.open("Penpot MCP Plugin", `?theme=${penpot.theme}`, { // Register message handlers penpot.ui.onMessage< - string | { id: string; type?: string; status?: string; sessionId?: string; task: string; params: any } + | string + | { + id: string; + type?: string; + status?: string; + sessionId?: string; + task: string; + params: any; + rollbacks?: ContentRollbackEvent[]; + } >((message) => { if (typeof message === "object" && message.type === "ui-initialized") { uiInitialized = true; @@ -85,6 +111,9 @@ penpot.ui.onMessage< } } else if (typeof message === "object" && message.type === "context-request") { sendWorkspaceContext(); + } else if (typeof message === "object" && message.type === "unclaimed-rollbacks") { + // the server got these with a response that came too late: report them again + rollbackLog.restore(message.rollbacks ?? []); } else if (typeof message === "object" && message.type === "update-connection-status") { mcp?.setMcpStatus(message.status || "unknown", message.sessionId); } else if (typeof message === "object" && message.task && message.id) { diff --git a/mcp/packages/server/src/PenpotMcpServer.http.test.ts b/mcp/packages/server/src/PenpotMcpServer.http.test.ts index 8c0c83f9b9..d5534de879 100644 --- a/mcp/packages/server/src/PenpotMcpServer.http.test.ts +++ b/mcp/packages/server/src/PenpotMcpServer.http.test.ts @@ -230,3 +230,32 @@ test("puts a rollback warning first in a failed tool response too", async (t) => assert.equal(failure.type, "text"); assert.match(failure.text, /Tool execution failed: .*Shape not found/); }); + +/** + * A plugin connection whose socket keeps the messages sent to it. + */ +function fakeConnection(): { socket: { readyState: number; send: (message: string) => void }; sent: unknown[] } { + const sent: unknown[] = []; + return { socket: { readyState: 1, send: (message: string) => sent.push(JSON.parse(message)) }, sent }; +} + +test("gives the rollbacks of a response that came after its task timed out back to the plugin", () => { + const bridge = server.pluginBridge as any; + const connection = fakeConnection(); + + bridge.handlePluginTaskResponse( + { id: "timed-out", success: true, data: { result: 1, log: "" }, rollbacks: [rollback] }, + connection + ); + + assert.deepEqual(connection.sent, [{ type: "unclaimed-rollbacks", rollbacks: [rollback] }]); +}); + +test("sends nothing back for a late response without rollbacks", () => { + const bridge = server.pluginBridge as any; + const connection = fakeConnection(); + + bridge.handlePluginTaskResponse({ id: "timed-out", success: true, data: { result: 1, log: "" } }, connection); + + assert.deepEqual(connection.sent, []); +}); diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index eaad21ee82..340016f44f 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -3,7 +3,12 @@ import * as http from "http"; import { z } from "zod"; import { AbstractPluginTask, PluginTask, PluginTaskError } from "./PluginTask"; import { RollbackNotices } from "./RollbackNotices"; -import { PluginTaskResponse, PluginTaskResult } from "@penpot/mcp-common"; +import { + ContentRollbackEvent, + PluginTaskResponse, + PluginTaskResult, + UnclaimedRollbacksMessage, +} from "@penpot/mcp-common"; import { createLogger } from "./logger"; import type { PenpotMcpServer } from "./PenpotMcpServer"; import type { RedisBridge } from "./RedisBridge"; @@ -317,7 +322,9 @@ export class PluginBridge implements TaskDispatchHost { * Handles responses from the plugin for completed tasks. * * Finds the pending task by ID and resolves or rejects its promise - * based on the execution result. + * based on the execution result. A response that comes after its task + * timed out has no task to report its rollbacks to, so they go back to + * the plugin, which sends them with its next response. * * @param response - The plugin task response containing ID and result * @param connection - The responding local connection, or undefined for Redis responses @@ -326,6 +333,7 @@ export class PluginBridge implements TaskDispatchHost { const task = this.pendingTasks.get(response.id); if (!task) { this.logger.info(`Received response for unknown task ID: ${response.id}`); + this.returnRollbacks(connection, response.rollbacks); return; } @@ -356,6 +364,28 @@ export class PluginBridge implements TaskDispatchHost { this.logger.info(`Task ${response.id} completed: success=${response.success}`); } + /** + * Sends rollbacks that no tool execution received back to the plugin. + * + * @param connection - The plugin connection that reported them, if known + * @param rollbacks - The rollbacks of a response whose task is gone + */ + private returnRollbacks( + connection: PenpotConnection | undefined, + rollbacks: ContentRollbackEvent[] | undefined + ): void { + if (!connection || !rollbacks || rollbacks.length === 0) { + return; + } + if (connection.socket.readyState !== 1) { + this.logger.warn("Plugin disconnected; %d rollback(s) of a late response are lost", rollbacks.length); + return; + } + const message: UnclaimedRollbacksMessage = { type: "unclaimed-rollbacks", rollbacks }; + connection.socket.send(JSON.stringify(message)); + this.logger.info("Returned %d rollback(s) of a late response to the plugin", rollbacks.length); + } + /** * Rejects a still-pending task with the given error, releasing its correlation state. *