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(