From c0f0cf22f8872e57bf8f5f53aba527d7933404eb Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Tue, 3 Nov 2026 10:04:26 +0000 Subject: [PATCH] :sparkles: Tell plugins and the MCP agent when changes are rolled back The backend rejects a save after the call that made the changes has returned, and the workspace then undoes them. A plugin, or the MCP agent, went on working with shapes that were no longer there and never learned why. The rollback event now names the files that lost commits and the pages and page shapes their changes touched, and plugins get it as the new `contentrollback` event (`ContentRollback` type) through a subscription to the store stream, so no rollback is dropped by debouncing. The MCP plugin keeps the rollbacks it is told about and sends them with its next task response, failed or not. The server collects them per tool call and puts a warning first in that tool's response, naming what was affected; the instructions tell the agent to read the pages again and redo what is missing. --- .../frontend/plugin-api-to-cljs-binding.md | 1 + frontend/src/app/main/data/persistence.cljs | 5 +- .../app/main/data/persistence/commits.cljs | 35 ++++++++ frontend/src/app/plugins/events.cljs | 51 ++++++++--- .../data/persistence_commits_test.cljs | 12 +++ .../frontend_tests/plugins/events_test.cljs | 25 +++++- mcp/packages/common/src/types.ts | 33 +++++++ mcp/packages/plugin/src/RollbackLog.test.ts | 47 ++++++++++ mcp/packages/plugin/src/RollbackLog.ts | 55 ++++++++++++ mcp/packages/plugin/src/TaskHandler.ts | 11 ++- mcp/packages/plugin/src/main.ts | 1 + mcp/packages/plugin/src/plugin.ts | 9 +- .../server/data/initial_instructions.md | 11 ++- .../server/src/PenpotMcpServer.http.test.ts | 58 ++++++++++++ mcp/packages/server/src/PluginBridge.ts | 27 +++++- mcp/packages/server/src/PluginTask.ts | 22 ++++- mcp/packages/server/src/RemotePluginTask.ts | 4 +- .../server/src/RollbackNotices.test.ts | 61 +++++++++++++ mcp/packages/server/src/RollbackNotices.ts | 88 +++++++++++++++++++ mcp/packages/server/src/Tool.ts | 16 +++- plugins/CHANGELOG.md | 1 + plugins/libs/plugin-types/index.d.ts | 29 ++++++ .../libs/plugins-runtime/src/lib/api/index.ts | 1 + 23 files changed, 578 insertions(+), 25 deletions(-) create mode 100644 mcp/packages/plugin/src/RollbackLog.test.ts create mode 100644 mcp/packages/plugin/src/RollbackLog.ts create mode 100644 mcp/packages/server/src/RollbackNotices.test.ts create mode 100644 mcp/packages/server/src/RollbackNotices.ts diff --git a/.serena/memories/frontend/plugin-api-to-cljs-binding.md b/.serena/memories/frontend/plugin-api-to-cljs-binding.md index 8c460d270b..45d08666ca 100644 --- a/.serena/memories/frontend/plugin-api-to-cljs-binding.md +++ b/.serena/memories/frontend/plugin-api-to-cljs-binding.md @@ -33,4 +33,5 @@ - Plugin listeners are watches on the global store and callbacks are debounced about 10ms. Callback exceptions are caught and logged so plugin code does not crash the app. - `selectionchange` callbacks receive arrays of shape id strings, while `filechange`, `pagechange`, and `shapechange` return proxies. - `contentsave` fires only when persistence status transitions to `:saved`; it calls the callback with no value. +- `contentrollback` is not a state watch: it subscribes to `st/stream` for `::dps/rolled-back` (kept in `subscriptions`, disposed by `remove-listener`), not debounced, delivered async so the reverted state is in place. Payload `{fileIds pageIds shapeIds}` from `pcm/rollback-targets` (page shapes only, from the reverted commits' redo changes). The runtime's `validEvents` must list every event name, or `penpot.on` rejects it; a stale runtime build rejects new events. The MCP plugin forwards these rollbacks with the next task response and the server puts a warning first in that tool response (`RollbackNotices`). - Plugin history `undoBlockBegin` creates a workspace undo transaction with a JS `Symbol`; `undoBlockFinish` commits that symbol. Missing finish eventually relies on the workspace transaction timeout. \ No newline at end of file diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index e97eabbd38..c16b94c8e0 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -626,9 +626,10 @@ (rx/of (dwu/forget-undo-groups groups undo-index) (prune-removed-shapes (get (dsh/lookup-page-objects state) (dm/get-in state [:workspace-local :edition]))) - ;; Tells listeners (e.g. a library sync) which files lost commits + ;; Tells listeners (a library sync, plugins) which files, pages + ;; and shapes lost commits (ptk/data-event ::rolled-back - {:file-ids (into (set (keys files)) (map :file-id) compensate)})))) + (pcm/rollback-targets {:files files :compensate compensate}))))) (defn- rollback-rejected "Reverts the commits `rejected` in the save of `entry`, the later commits diff --git a/frontend/src/app/main/data/persistence/commits.cljs b/frontend/src/app/main/data/persistence/commits.cljs index ad86c1999a..7b2cc73b19 100644 --- a/frontend/src/app/main/data/persistence/commits.cljs +++ b/frontend/src/app/main/data/persistence/commits.cljs @@ -150,6 +150,41 @@ (count compensate)) :files (count (into (set (keys files)) (map :file-id) compensate))}) +(def ^:private page-change-types + "Changes whose `:id` is a page id." + #{:add-page :mod-page :del-page :mov-page}) + +(def ^:private shape-change-types + "Changes whose `:id` is a shape id." + #{:add-obj :mod-obj :del-obj :fix-obj}) + +(defn- change-page-id + [{:keys [type] :as change}] + (if (contains? page-change-types type) + (:id change) + (:page-id change))) + +(defn- change-shape-ids + "Ids of the page shapes `change` touches; none for component shapes." + [{:keys [type page-id] :as change}] + (when (some? page-id) + (cond + (contains? shape-change-types type) + [(:id change)] + + (contains? #{:mov-objects :reorder-children} type) + (:shapes change)))) + +(defn rollback-targets + "The files of the commits `plan` reverts, and the pages and page shapes + their changes touch." + [{:keys [files compensate]}] + (let [commits (into (vec compensate) (mapcat (comp :reverted val)) files) + changes (into [] (mapcat :redo-changes) commits)] + {:file-ids (into (set (keys files)) (map :file-id) compensate) + :page-ids (into #{} (keep change-page-id) changes) + :shape-ids (into #{} (mapcat change-shape-ids) changes)})) + (defn- drop-commits "Removes the commits in `ids` from the queue and the buffer; an entry left empty leaves the queue." diff --git a/frontend/src/app/plugins/events.cljs b/frontend/src/app/plugins/events.cljs index 884a1e0bac..5e541e3c29 100644 --- a/frontend/src/app/plugins/events.cljs +++ b/frontend/src/app/plugins/events.cljs @@ -8,6 +8,7 @@ (:require [app.common.data.macros :as dm] [app.main.data.helpers :as dsh] + [app.main.data.persistence :as dps] [app.main.store :as st] [app.plugins.file :as file] [app.plugins.page :as page] @@ -15,7 +16,9 @@ [app.plugins.shape :as shape] [app.util.object :as obj] [app.util.theme :as theme] - [goog.functions :as gf])) + [beicon.v2.core :as rx] + [goog.functions :as gf] + [potok.v2.core :as ptk])) (defmulti handle-state-change (fn [type _ _ _ _] type)) @@ -95,6 +98,25 @@ [props] {:shape-id (-> (obj/get props "shapeId") parser/parse-id)}) +;; Stream subscriptions of the listeners that follow events, not state +(defonce ^:private subscriptions (atom {})) + +(defn- format-rollback + [{:keys [file-ids page-ids shape-ids]}] + #js {:fileIds (apply array (map str file-ids)) + :pageIds (apply array (map str page-ids)) + :shapeIds (apply array (map str shape-ids))}) + +(defn- subscribe-rollbacks + "Calls `callback` after each rollback of rejected changes, once the + reverted data is in the state. Not debounced: every rollback counts." + [callback] + (->> st/stream + (rx/filter (ptk/type? ::dps/rolled-back)) + (rx/map (comp format-rollback deref)) + (rx/observe-on :async) + (rx/subs! callback))) + (defn add-listener [type plugin-id callback props] (let [plugin-id (parser/parse-id plugin-id) @@ -115,21 +137,26 @@ ;; We also debounce the callbacks so we don't get too many at the same time debounced-callback (gf/debounce safe-callback 10)] - (add-watch - st/state key - (fn [_ _ old-val new-val] - ;; The store notifies its watches with a plain iteration: a throw - ;; here starves the watches registered after this one. - (try - (let [result (handle-state-change type plugin-id old-val new-val props)] - (when (not= ::not-changed result) - (debounced-callback result))) - (catch :default cause - (.error js/console cause))))) + (if (= "contentrollback" type) + (swap! subscriptions assoc key (subscribe-rollbacks safe-callback)) + (add-watch + st/state key + (fn [_ _ old-val new-val] + ;; The store notifies its watches with a plain iteration: a throw + ;; here starves the watches registered after this one. + (try + (let [result (handle-state-change type plugin-id old-val new-val props)] + (when (not= ::not-changed result) + (debounced-callback result))) + (catch :default cause + (.error js/console cause)))))) ;; return the generated key key)) (defn remove-listener [key] + (when-let [subscription (get @subscriptions key)] + (rx/dispose! subscription) + (swap! subscriptions dissoc key)) (remove-watch st/state key)) diff --git a/frontend/test/frontend_tests/data/persistence_commits_test.cljs b/frontend/test/frontend_tests/data/persistence_commits_test.cljs index 16ec9bc145..2b6cde7258 100644 --- a/frontend/test/frontend_tests/data/persistence_commits_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_commits_test.cljs @@ -158,3 +158,15 @@ (t/is (empty? (get-in plan [:files file-id :redo]))) (t/is (= [(:id lib)] (mapv :id (get-in plan [:files lib-id :reverted]))) "the rest of the undo group goes too"))) + +(t/deftest rollback-targets-name-the-files-pages-and-page-shapes-of-the-reverted-changes + (let [[f1 f2 p1 p2 p3 s1 s2 s3 s4 s5] (repeatedly 10 uuid/next) + plan {:files {f1 {:reverted [{:redo-changes [{:type :add-obj :page-id p1 :id s1} + {:type :mov-objects :page-id p1 :shapes [s2 s3]} + {:type :mod-obj :component-id (uuid/next) :id s4} + {:type :mod-page :id p2}]}]}} + :compensate [{:file-id f2 :redo-changes [{:type :del-obj :page-id p3 :id s5}]}]}] + (t/is (= {:file-ids #{f1 f2} + :page-ids #{p1 p2 p3} + :shape-ids #{s1 s2 s3 s5}} + (pcm/rollback-targets plan))))) diff --git a/frontend/test/frontend_tests/plugins/events_test.cljs b/frontend/test/frontend_tests/plugins/events_test.cljs index b32a9914c7..765471f00d 100644 --- a/frontend/test/frontend_tests/plugins/events_test.cljs +++ b/frontend/test/frontend_tests/plugins/events_test.cljs @@ -8,9 +8,11 @@ "Unit tests for app.plugins.events, the plugin state-change listeners." (:require [app.common.uuid :as uuid] + [app.main.data.persistence :as dps] [app.main.store :as st] [app.plugins.events :as events] - [cljs.test :as t :include-macros true])) + [cljs.test :as t :include-macros true] + [potok.v2.core :as ptk])) (def ^:private plugin-id (str (uuid/next))) @@ -64,3 +66,24 @@ (remove-watch st/state ::probe-watch) (events/remove-listener key) (swap! st/state dissoc ::probe))))) + +(t/deftest contentrollback-listener-gets-every-rollback + (t/async done + (let [[file-id page-id shape-id] (repeatedly 3 uuid/next) + seen (atom []) + key (events/add-listener "contentrollback" plugin-id #(swap! seen conj (js->clj %)) nil)] + (st/emit! (ptk/data-event ::dps/rolled-back {:file-ids #{file-id} :page-ids #{page-id} :shape-ids #{shape-id}}) + (ptk/data-event ::dps/rolled-back {:file-ids #{file-id} :page-ids #{} :shape-ids #{}})) + (js/setTimeout + (fn [] + (events/remove-listener key) + (t/is (= [{"fileIds" [(str file-id)] "pageIds" [(str page-id)] "shapeIds" [(str shape-id)]} + {"fileIds" [(str file-id)] "pageIds" [] "shapeIds" []}] + @seen)) + (st/emit! (ptk/data-event ::dps/rolled-back {:file-ids #{file-id}})) + (js/setTimeout + (fn [] + (t/is (= 2 (count @seen)) "a removed listener gets nothing") + (done)) + 20)) + 20)))) diff --git a/mcp/packages/common/src/types.ts b/mcp/packages/common/src/types.ts index 36e52cc531..392cb575e5 100644 --- a/mcp/packages/common/src/types.ts +++ b/mcp/packages/common/src/types.ts @@ -8,6 +8,34 @@ export interface PluginTaskResult { * Optional result data from the task execution. */ data?: T; + + /** + * Rollbacks of rejected changes that happened in the workspace since the previous task response. + */ + rollbacks?: ContentRollbackEvent[]; +} + +/** + * Rollback of rejected changes in the Penpot workspace. + * + * The backend rejected some changes and the workspace undid them, together with the rest of their + * undo steps and any later change to the same file. Mirrors the plugin API's `ContentRollback`. + */ +export interface ContentRollbackEvent { + /** + * Ids of the files that lost changes. + */ + fileIds: string[]; + + /** + * Ids of the pages the undone changes touched. + */ + pageIds: string[]; + + /** + * Ids of the page shapes the undone changes touched. + */ + shapeIds: string[]; } /** @@ -57,6 +85,11 @@ export interface PluginTaskResponse { * The result of the task execution. */ data?: T; + + /** + * Rollbacks of rejected changes that happened in the workspace since the previous task response. + */ + rollbacks?: ContentRollbackEvent[]; } /** diff --git a/mcp/packages/plugin/src/RollbackLog.test.ts b/mcp/packages/plugin/src/RollbackLog.test.ts new file mode 100644 index 0000000000..4bfb8f50da --- /dev/null +++ b/mcp/packages/plugin/src/RollbackLog.test.ts @@ -0,0 +1,47 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { RollbackLog } from "./RollbackLog.ts"; + +const rollback = (id: string) => ({ fileIds: [`file-${id}`], pageIds: [`page-${id}`], shapeIds: [`shape-${id}`] }); + +/** + * Installs a `penpot` global whose `on` hands each subscription to `on`. + */ +function installPenpot(on: (type: string, callback: any) => void): void { + (globalThis as any).penpot = { on }; +} + +test("records the rollbacks Penpot reports", () => { + let callback: any; + installPenpot((type, cb) => { + assert.equal(type, "contentrollback"); + callback = cb; + }); + const log = new RollbackLog(); + log.listen(); + + callback(rollback("a")); + + assert.deepEqual(log.pending(), [rollback("a")]); +}); + +test("stays empty when Penpot has no rollback event", () => { + installPenpot(() => { + throw new Error("Invalid enum value"); + }); + const log = new RollbackLog(); + + assert.doesNotThrow(() => log.listen()); + assert.deepEqual(log.pending(), []); +}); + +test("keeps the rollbacks recorded after the acknowledged ones", () => { + const log = new RollbackLog(); + log.record(rollback("a")); + const sent = log.pending(); + log.record(rollback("b")); + + log.acknowledge(sent.length); + + assert.deepEqual(log.pending(), [rollback("b")]); +}); diff --git a/mcp/packages/plugin/src/RollbackLog.ts b/mcp/packages/plugin/src/RollbackLog.ts new file mode 100644 index 0000000000..600fcaab72 --- /dev/null +++ b/mcp/packages/plugin/src/RollbackLog.ts @@ -0,0 +1,55 @@ +import type { ContentRollbackEvent } from "../../common/src"; + +/** + * 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. + */ +export class RollbackLog { + private events: ContentRollbackEvent[] = []; + + /** + * Starts recording the rollbacks Penpot reports. + * + * Penpot versions without the `contentrollback` event reject the subscription; the log then + * stays empty. + */ + listen(): void { + try { + // @ts-ignore - contentrollback is not yet in the released plugin types + penpot.on("contentrollback", (event: ContentRollbackEvent) => this.record(event)); + } catch (error) { + console.warn("Penpot does not report rollbacks:", error); + } + } + + /** + * Adds a rollback to the log. + * + * @param event - the rollback as reported by Penpot + */ + record(event: ContentRollbackEvent): void { + this.events.push({ + fileIds: [...event.fileIds], + pageIds: [...event.pageIds], + shapeIds: [...event.shapeIds], + }); + } + + /** + * Returns the rollbacks not yet acknowledged, oldest first. + */ + pending(): ContentRollbackEvent[] { + return [...this.events]; + } + + /** + * Removes the oldest `count` rollbacks, once a response carrying them has been sent. + * + * @param count - the number of rollbacks the sent response carried + */ + acknowledge(count: number): void { + this.events = this.events.slice(count); + } +} diff --git a/mcp/packages/plugin/src/TaskHandler.ts b/mcp/packages/plugin/src/TaskHandler.ts index 7bc0aa3085..c5e6130e76 100644 --- a/mcp/packages/plugin/src/TaskHandler.ts +++ b/mcp/packages/plugin/src/TaskHandler.ts @@ -1,3 +1,5 @@ +import type { RollbackLog } from "./RollbackLog"; + /** * Represents a task received from the MCP server in the Penpot MCP plugin */ @@ -8,11 +10,13 @@ export class Task { * @param requestId Unique identifier for the task request * @param taskType The type of the task to execute * @param params Task parameters/arguments + * @param rollbackLog Log whose pending rollbacks the response carries, if any */ constructor( public requestId: string, public taskType: string, - public params: TParams + public params: TParams, + private readonly rollbackLog?: RollbackLog ) {} /** @@ -24,6 +28,9 @@ export class Task { return; } + // rollbacks the server has not been told about, whatever the task did + const rollbacks = this.rollbackLog?.pending() ?? []; + // create response object const response = { type: "task-response", @@ -33,6 +40,7 @@ export class Task { success: success, data: data, error: error, + ...(rollbacks.length > 0 ? { rollbacks } : {}), }, }; @@ -60,6 +68,7 @@ export class Task { } console.log("Sent task response:", response); this.isResponseSent = true; + this.rollbackLog?.acknowledge(rollbacks.length); } public sendSuccess(data: any = undefined): void { diff --git a/mcp/packages/plugin/src/main.ts b/mcp/packages/plugin/src/main.ts index f787cd9298..99f78dfeca 100644 --- a/mcp/packages/plugin/src/main.ts +++ b/mcp/packages/plugin/src/main.ts @@ -121,6 +121,7 @@ function sendTaskResponse(response: any): void { id: response.id, success: false, error: errorMessage, + rollbacks: response.rollbacks, }; responseString = JSON.stringify(response); } diff --git a/mcp/packages/plugin/src/plugin.ts b/mcp/packages/plugin/src/plugin.ts index 502116707f..805d07f88f 100644 --- a/mcp/packages/plugin/src/plugin.ts +++ b/mcp/packages/plugin/src/plugin.ts @@ -1,6 +1,7 @@ import { ExecuteCodeTaskHandler } from "./task-handlers/ExecuteCodeTaskHandler"; import { Task, TaskHandler } from "./TaskHandler"; import { formatTaskError } from "./ErrorUtils"; +import { RollbackLog } from "./RollbackLog"; /** * indicates whether the plugin is running in an environment with the Penpot-integrated remote MCP server @@ -26,6 +27,12 @@ mcp?.setMcpStatus("connecting"); */ const taskHandlers: TaskHandler[] = [new ExecuteCodeTaskHandler()]; +/** + * Rollbacks of rejected changes, reported to the MCP server with the next task response. + */ +const rollbackLog = new RollbackLog(); +rollbackLog.listen(); + // Open the plugin UI (main.ts) penpot.ui.open("Penpot MCP Plugin", `?theme=${penpot.theme}`, { width: 236, @@ -78,7 +85,7 @@ penpot.ui.onMessage { console.log("Executing plugin task:", request.task, request.params); - const task = new Task(request.id, request.task, request.params); + const task = new Task(request.id, request.task, request.params, rollbackLog); // Find the appropriate handler const handler = taskHandlers.find((h) => h.isApplicableTo(task)); diff --git a/mcp/packages/server/data/initial_instructions.md b/mcp/packages/server/data/initial_instructions.md index 68f5cf2ed1..460153689a 100644 --- a/mcp/packages/server/data/initial_instructions.md +++ b/mcp/packages/server/data/initial_instructions.md @@ -24,7 +24,16 @@ So if you need to read the result of your executions/observe properties affected before trying to observe changes. Every `execude_code` call waits for updates before applying your code, so you never need to call `waitForLayoutUpdate` at the beginning of your code. -## Read-Only Mode +## Rejected Changes and Read-Only Mode + +Penpot saves your changes to the backend after your `execute_code` call returns. +When the backend rejects a save, Penpot undoes the rejected changes, together with the rest of their undo steps +and any later change to the same file. This can happen a few seconds after the call that made the changes succeeded. +The next tool response then starts with a warning that names the files, pages and shapes involved. +When you get this warning: + * Do not trust what you know about the design from before: shapes you created may be gone, and removed ones may be back. + * Read the affected pages and shapes again (e.g. `penpotUtils.shapeStructure(page.root, 3)`) and check that they hold what you expect. + * Redo the missing changes, then check again; if the same changes are rejected again, stop and tell the user. While saves are failing, the workspace becomes read-only: every write through the API throws an error saying that the workspace is read-only. Do not retry in a loop; wait and try again later, or tell the user. diff --git a/mcp/packages/server/src/PenpotMcpServer.http.test.ts b/mcp/packages/server/src/PenpotMcpServer.http.test.ts index e738a49a2d..b3fbeb8b50 100644 --- a/mcp/packages/server/src/PenpotMcpServer.http.test.ts +++ b/mcp/packages/server/src/PenpotMcpServer.http.test.ts @@ -118,3 +118,61 @@ test("supports older Streamable HTTP clients without allocating a session", asyn await client.close(); } }); + +/** + * Makes the plugin bridge answer each task at once with the given responses, in order. + */ +function answerTasks(t: { mock: any }, responses: Array>): void { + const bridge = server.pluginBridge as any; + t.mock.method(bridge, "sendPluginTask", (task: { id: string }) => { + bridge.pendingTasks.set(task.id, task); + bridge.handlePluginTaskResponse({ id: task.id, ...responses.shift() }); + }); +} + +async function callExecuteCode(): Promise { + const response = await modernRequest( + "tools/call", + { name: "execute_code", arguments: { code: "return 1;" } }, + "?userToken=alice" + ); + const body = (await response.json()) as { result: CallToolResult }; + assert.equal(response.status, 200, JSON.stringify(body)); + return body.result; +} + +const rollback = { fileIds: ["file-1"], pageIds: ["page-1"], shapeIds: ["shape-1", "shape-2"] }; + +test("puts a warning first in the tool response that reports a rollback", async (t) => { + answerTasks(t, [ + { success: true, data: { result: 1, log: "" }, rollbacks: [rollback] }, + { success: true, data: { result: 2, log: "" } }, + ]); + + const first = await callExecuteCode(); + const second = await callExecuteCode(); + + assert.equal(first.content.length, 2); + const [notice, data] = first.content; + assert.equal(notice.type, "text"); + assert.match(notice.text, /CHANGES WERE ROLLED BACK/); + assert.match(notice.text, /Pages: page-1/); + assert.match(notice.text, /Shapes: shape-1, shape-2/); + assert.equal(data.type, "text"); + assert.equal(JSON.parse(data.text).result, 1); + + assert.equal(second.content.length, 1); +}); + +test("puts a rollback warning first in a failed tool response too", async (t) => { + answerTasks(t, [{ success: false, error: "Shape not found", rollbacks: [rollback] }]); + + const result = await callExecuteCode(); + + assert.equal(result.content.length, 2); + const [notice, failure] = result.content; + assert.equal(notice.type, "text"); + assert.match(notice.text, /CHANGES WERE ROLLED BACK/); + assert.equal(failure.type, "text"); + assert.match(failure.text, /Tool execution failed: .*Shape not found/); +}); diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index 305f5dc35e..9da4981f67 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -1,6 +1,7 @@ import { WebSocket, WebSocketServer } from "ws"; import * as http from "http"; -import { AbstractPluginTask, PluginTask } from "./PluginTask"; +import { AbstractPluginTask, PluginTask, PluginTaskError } from "./PluginTask"; +import { RollbackNotices } from "./RollbackNotices"; import { RemotePluginTask } from "./RemotePluginTask"; import { PluginTaskRequest, PluginTaskResponse, PluginTaskResult } from "@penpot/mcp-common"; import { createLogger } from "./logger"; @@ -76,6 +77,11 @@ export class PluginBridge { private readonly pendingTasks: Map> = new Map(); private readonly taskTimeouts: Map = new Map(); + /** + * Collector of the rollbacks that task responses report, per tool execution. + */ + public readonly rollbackNotices = new RollbackNotices(); + /** * Creates the plugin bridge and starts its WebSocket server. * @@ -255,9 +261,12 @@ export class PluginBridge { // Resolve or reject the task's promise based on the result if (response.success) { - task.resolveWithResult({ data: response.data }); + task.resolveWithResult({ data: response.data, rollbacks: response.rollbacks }); } else { - const error = new Error(response.error || "Task execution failed (details not provided)"); + const error = new PluginTaskError( + response.error || "Task execution failed (details not provided)", + response.rollbacks + ); task.rejectWithError(error); } @@ -339,6 +348,7 @@ export class PluginBridge { * Executes a plugin task by sending it to the connected Penpot plugin instance, * either directly via WebSocket or indirectly via Redis (depending on the configuration), * and awaiting the result. + * Rollbacks the response reports go to {@link rollbackNotices}. * * @param task - The plugin task to execute * @throws Error if no plugin instances are connected or available @@ -347,7 +357,16 @@ export class PluginBridge { task: PluginTask ): Promise { this.sendPluginTask(task, this.redisBridge !== undefined); - return await task.getResultPromise(); + try { + const result = await task.getResultPromise(); + this.rollbackNotices.record(result.rollbacks); + return result; + } catch (error) { + if (error instanceof PluginTaskError) { + this.rollbackNotices.record(error.rollbacks); + } + throw error; + } } /** diff --git a/mcp/packages/server/src/PluginTask.ts b/mcp/packages/server/src/PluginTask.ts index 0e0d327564..b5b33a9c8a 100644 --- a/mcp/packages/server/src/PluginTask.ts +++ b/mcp/packages/server/src/PluginTask.ts @@ -5,7 +5,7 @@ * A task defines a specific operation for the plugin to execute along with * strongly-typed parameters and provides request/response correlation. */ -import { PluginTaskRequest, PluginTaskResult } from "@penpot/mcp-common"; +import { ContentRollbackEvent, PluginTaskRequest, PluginTaskResult } from "@penpot/mcp-common"; import { randomUUID } from "crypto"; /** @@ -153,3 +153,23 @@ export class PluginTask< this.rejectResult(error); } } + +/** + * Error of a plugin task that failed in the plugin. + * + * Carries the rollbacks the failed task's response reported, which must reach the LLM like those of a + * successful response. + */ +export class PluginTaskError extends Error { + /** + * @param message - the error message reported by the plugin + * @param rollbacks - the rollbacks the response reported + */ + constructor( + message: string, + public readonly rollbacks: ContentRollbackEvent[] = [] + ) { + super(message); + this.name = "PluginTaskError"; + } +} diff --git a/mcp/packages/server/src/RemotePluginTask.ts b/mcp/packages/server/src/RemotePluginTask.ts index 368623a5a0..c4714d021c 100644 --- a/mcp/packages/server/src/RemotePluginTask.ts +++ b/mcp/packages/server/src/RemotePluginTask.ts @@ -1,4 +1,4 @@ -import { AbstractPluginTask } from "./PluginTask"; +import { AbstractPluginTask, PluginTaskError } from "./PluginTask"; import { PluginTaskResult } from "@penpot/mcp-common"; import type { RedisBridge } from "./RedisBridge"; @@ -43,6 +43,7 @@ export class RemotePluginTask extends AbstractPluginTask ({ + fileIds: ["file-1"], + pageIds: [`page-${id}`], + shapeIds: shapes, +}); + +test("collects only the rollbacks recorded while its function runs", async () => { + const notices = new RollbackNotices(); + notices.record([rollback("outside")]); + + const [value, rollbacks] = await notices.collect(async () => { + notices.record([rollback("a")]); + await Promise.resolve(); + notices.record([rollback("b")]); + return 42; + }); + + assert.equal(value, 42); + assert.deepEqual(rollbacks, [rollback("a"), rollback("b")]); +}); + +test("keeps the rollbacks of overlapping executions apart", async () => { + const notices = new RollbackNotices(); + let release!: () => void; + const gate = new Promise((resolve) => (release = resolve)); + + const first = notices.collect(async () => { + await gate; + notices.record([rollback("a")]); + }); + const second = notices.collect(async () => { + notices.record([rollback("b")]); + release(); + }); + + const [[, a], [, b]] = await Promise.all([first, second]); + assert.deepEqual(a, [rollback("a")]); + assert.deepEqual(b, [rollback("b")]); +}); + +test("names each affected file, page and shape once", () => { + const text = RollbackNotices.describe([rollback("a", ["s1", "s2"]), rollback("b", ["s2"])]); + + assert.match(text, /\(2 times\)/); + assert.match(text, /Files: file-1\n/); + assert.match(text, /Pages: page-a, page-b\n/); + assert.match(text, /Shapes: s1, s2$/); +}); + +test("lists at most 50 shapes", () => { + const shapes = Array.from({ length: 53 }, (_, i) => `s${i}`); + + const text = RollbackNotices.describe([rollback("a", shapes)]); + + assert.match(text, /s49 \(and 3 more\)$/); + assert.doesNotMatch(text, /s50/); +}); diff --git a/mcp/packages/server/src/RollbackNotices.ts b/mcp/packages/server/src/RollbackNotices.ts new file mode 100644 index 0000000000..67b746a38c --- /dev/null +++ b/mcp/packages/server/src/RollbackNotices.ts @@ -0,0 +1,88 @@ +import { AsyncLocalStorage } from "async_hooks"; +import type { ContentRollbackEvent } from "@penpot/mcp-common"; +import { TextContent, ToolResponse } from "./ToolResponse"; + +/** + * Collector of the rollbacks of rejected changes that plugin task responses report during one tool execution. + * + * The plugin reports a rollback with the first task response after it, so the rollback reaches the tool + * execution that follows the one which made the changes. {@link withNotice} then puts a warning at the + * top of that tool's response, so the LLM learns that some of its earlier changes are gone. + */ +export class RollbackNotices { + /** + * Maximum number of shape ids listed in a notice. + */ + private static readonly MAX_LISTED_SHAPES = 50; + + private readonly storage = new AsyncLocalStorage(); + + /** + * Runs the given function, collecting the rollbacks recorded while it runs. + * + * @param fn - the function to run (typically a tool's core logic) + * @returns the function's value and the collected rollbacks + */ + async collect(fn: () => Promise): Promise<[T, ContentRollbackEvent[]]> { + const rollbacks: ContentRollbackEvent[] = []; + const value = await this.storage.run(rollbacks, fn); + return [value, rollbacks]; + } + + /** + * Records rollbacks reported by a plugin task response for the current tool execution. + * Rollbacks reported outside of {@link collect} are dropped. + * + * @param rollbacks - the rollbacks the response carried, if any + */ + record(rollbacks: ContentRollbackEvent[] | undefined): void { + if (rollbacks && rollbacks.length > 0) { + this.storage.getStore()?.push(...rollbacks); + } + } + + /** + * Puts a rollback warning at the top of the given response. + * + * @param response - the tool response + * @param rollbacks - the rollbacks collected while the tool ran + * @returns the response, with the warning first if there were rollbacks + */ + static withNotice(response: ToolResponse, rollbacks: ContentRollbackEvent[]): ToolResponse { + if (rollbacks.length === 0) { + return response; + } + // clients may join text items without a separator + const notice = new TextContent(RollbackNotices.describe(rollbacks) + "\n\n"); + response.content = [notice, ...response.content]; + return response; + } + + /** + * Describes the given rollbacks for the LLM. + * + * @param rollbacks - the rollbacks to describe (at least one) + */ + static describe(rollbacks: ContentRollbackEvent[]): string { + const fileIds = new Set(rollbacks.flatMap((r) => r.fileIds)); + const pageIds = new Set(rollbacks.flatMap((r) => r.pageIds)); + const shapeIds = [...new Set(rollbacks.flatMap((r) => r.shapeIds))]; + + const listedShapes = shapeIds.slice(0, RollbackNotices.MAX_LISTED_SHAPES); + const unlistedShapes = shapeIds.length - listedShapes.length; + + return ( + "WARNING: CHANGES WERE ROLLED BACK. Since your previous Penpot tool call, the Penpot backend rejected " + + `some changes ${rollbacks.length > 1 ? `(${rollbacks.length} times) ` : ""}and the workspace undid them, ` + + "together with the rest of their undo steps and any later change to the same file. " + + "Changes you made in earlier calls may be gone, even though those calls succeeded, and the tool call " + + "that returned this warning may have run on the reverted data.\n" + + "Before you continue, read the affected pages and shapes again, check that they hold what you expect, " + + "and redo what is missing. Do not rely on shape references or data you kept from before.\n" + + `Files: ${[...fileIds].join(", ") || "none"}\n` + + `Pages: ${[...pageIds].join(", ") || "none"}\n` + + `Shapes: ${listedShapes.join(", ") || "none"}` + + (unlistedShapes > 0 ? ` (and ${unlistedShapes} more)` : "") + ); + } +} diff --git a/mcp/packages/server/src/Tool.ts b/mcp/packages/server/src/Tool.ts index df4e1f2266..433361b3ad 100644 --- a/mcp/packages/server/src/Tool.ts +++ b/mcp/packages/server/src/Tool.ts @@ -3,6 +3,7 @@ import "reflect-metadata"; import { TextResponse, ToolResponse } from "./ToolResponse"; import type { PenpotMcpServer, SessionContext } from "./PenpotMcpServer"; import { createLogger } from "./logger"; +import { RollbackNotices } from "./RollbackNotices"; /** * An empty arguments class for tools that do not require any parameters. @@ -34,9 +35,22 @@ export abstract class Tool { * Executes the tool with automatic validation and type safety. * * This method handles the unknown args from the MCP protocol, - * delegating to the type-safe implementation. + * delegating to the type-safe implementation. A rollback reported by a + * plugin task during the execution puts a warning first in the response. */ async execute(args: unknown): Promise { + const [response, rollbacks] = await this.mcpServer.pluginBridge.rollbackNotices.collect(() => + this.executeAndReport(args) + ); + return RollbackNotices.withNotice(response, rollbacks); + } + + /** + * Executes the tool logic, turning a failure into an error response. + * + * @param args - the tool arguments + */ + private async executeAndReport(args: unknown): Promise { const executionId = ++Tool.executionCounter; try { let argsInstance: TArgs = args as TArgs; diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index bc46db545d..2c0c3493e7 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -8,6 +8,7 @@ - **plugin-types:** Added `paddingType` (`'simple' | 'multiple'`) to flex and grid layouts and `marginType` (`'simple' | 'multiple'`) to layout children, exposing whether the four padding/margin sides are mirrored or honoured independently. - **plugin-types**: Added `waitForLayoutUpdate` to wait until pending layout updates have finished. It rejects when the optional timeout elapses, defaulting to 30 seconds so a wait never hangs. - **plugin-types**: Added `waitForLayoutUpdate` to the `Shape` interface to wait until the pending layout updates of a shape and its children have finished +- **plugin-types:** Added the `contentrollback` event and the `ContentRollback` type. The event fires after the backend rejected some changes and the workspace undid them, with the ids of the files, pages and shapes affected. - **plugin-types:** `Stroke` exposes per-side widths for boards and rectangles, also supported by `borderWidth` and `dimension` tokens. Read-only `strokeWidthType` reports `simple` when all four effective widths match, otherwise `multiple`. ### 🩹 Fixes diff --git a/plugins/libs/plugin-types/index.d.ts b/plugins/libs/plugin-types/index.d.ts index c756cc50b4..97902fd0a0 100644 --- a/plugins/libs/plugin-types/index.d.ts +++ b/plugins/libs/plugin-types/index.d.ts @@ -110,6 +110,7 @@ export interface Penpot extends Omit< * - themechange: event emitted when the user changes its theme. The callback will receive the new theme (currently: either `dark` or `light`) * - filechange: event emitted when a different file is opened. The callback will receive the new file. * - contentsave: event emitted after the file content is saved in the backend. The callback receives no arguments. + * - contentrollback: event emitted after the backend rejected some changes and they were undone in the workspace. The callback receives a `ContentRollback` with the files, pages and shapes affected. * - finish: event emitted when the current file is closed. The callback will receive the id of the closed file. * * @param type The event type to listen for. @@ -1590,6 +1591,34 @@ export interface EventsMap { * The `contentsave` event will trigger after the file content is saved in the backend. */ readonly contentsave: void; + + /** + * The `contentrollback` event will trigger after the backend rejected some changes + * and the workspace undid them, together with the rest of their undo steps and any + * later change to the same file. Changes made by a plugin can be undone this way + * after the call that made them returned, so a plugin that relies on them should + * read the affected shapes again. + */ + readonly contentrollback: ContentRollback; +} + +/** + * What a rollback of rejected changes undid, as given to the `contentrollback` event. + */ +export interface ContentRollback { + /** + * Ids of the files that lost changes. + */ + readonly fileIds: string[]; + /** + * Ids of the pages the undone changes touched. + */ + readonly pageIds: string[]; + /** + * Ids of the page shapes the undone changes touched. A shape the undone changes + * created no longer exists; one they removed exists again. + */ + readonly shapeIds: string[]; } /** diff --git a/plugins/libs/plugins-runtime/src/lib/api/index.ts b/plugins/libs/plugins-runtime/src/lib/api/index.ts index 8956a5124e..49fd8e55c5 100644 --- a/plugins/libs/plugins-runtime/src/lib/api/index.ts +++ b/plugins/libs/plugins-runtime/src/lib/api/index.ts @@ -42,6 +42,7 @@ export const validEvents = [ 'themechange', 'shapechange', 'contentsave', + 'contentrollback', ] as const; export function createApi(