From d09bc9c3ee255ad13c0658da25c3404941dbb85c Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Tue, 3 Nov 2026 11:22:30 +0000 Subject: [PATCH] :bug: Keep reporting rollbacks the MCP agent has not heard about The MCP plugin sends the rollbacks it saw with its next task response and then forgets them. When that response came after the server had given up on the task, the server dropped it and the agent never heard about the rollback. A reload of the tab or of the plugin lost them too. The server now sends the rollbacks of such a late response back to the plugin, which reports them again with its next response. The plugin keeps the rollbacks not reported yet in its local storage, one entry per file, so a reload does not lose them. Both MCP plugin manifests ask for the allow:localstorage permission for that. --- .serena/memories/mcp/core.md | 4 + frontend/src/app/plugins/register.cljs | 4 +- mcp/packages/common/src/types.ts | 13 ++ mcp/packages/plugin/public/manifest.json | 2 +- .../plugin/src/ConnectionMetadata.test.ts | 1 + mcp/packages/plugin/src/RollbackLog.test.ts | 124 ++++++++++++++- mcp/packages/plugin/src/RollbackLog.ts | 150 +++++++++++++++++- mcp/packages/plugin/src/plugin.ts | 37 ++++- .../server/src/PenpotMcpServer.http.test.ts | 29 ++++ mcp/packages/server/src/PluginBridge.ts | 34 +++- 10 files changed, 382 insertions(+), 16 deletions(-) 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 c45cae623d..ea87d92ca6 100644 --- a/mcp/packages/plugin/src/ConnectionMetadata.test.ts +++ b/mcp/packages/plugin/src/ConnectionMetadata.test.ts @@ -32,6 +32,7 @@ function pluginRequire(name: string) { if (name === "./VersionCompatibility") return VersionCompatibility; if (name === "./RollbackLog") return { + 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. *