🐛 Fix ChatGPT tool discovery with the submit_feedback tool (#12845)

* 🐛 Fix ChatGPT tool discovery with the submit_feedback tool

ChatGPT compiles the regex patterns advertised in tool input schemas
with Python, which rejects Unicode property escapes such as \P{Cc}. The
submit_feedback subject pattern used one, so ChatGPT dropped the whole
tool list ("action discovery failed") on deployments that register the
tool.

The subject pattern now spells out the Cc range as a character class,
which accepts the same input. A test asserts that no advertised pattern
uses \p{...} or \P{...}.

AI-assisted-by: claude-opus-5-5

* 🐛 Fix MCP plugin test stub for the version check

plugin.ts checks version compatibility with extractMinorVersion, but the
tests loaded it with a require stub that returned the same dummy object
for every import, so the check threw once the UI initialized.

The plugin.ts tests now share a stub that supplies the real
VersionCompatibility, ErrorUtils and TaskHandler modules, stubs only the
code execution handler and rejects unexpected imports. TaskHandler is
transpiled for the sandbox because Node's type stripping cannot load its
parameter properties.

AI-assisted-by: claude-opus-5-5

* 🐛 Fix Management tests for backend-confirmed setters

Management setters save to the backend first and update the object only
when the save succeeds, so seven live tests failed:

- setter tests now wait for the new value before reading it
- createProject checks fileCount by listing the project again, as
  fileCount is the count from when the project was listed
- project moveTo no longer calls remove on the target team, which teams
  do not have; the live CI driver deletes temporary teams
- renaming the open file waits for its restore, which made the next
  navigation test fail

The README notes on setters, fileCount and team cleanup match this.

AI-assisted-by: claude-opus-5-5

mcp/packages/plugin/src/ConnectionMetadata.test.ts

@@ -4,9 +4,11 @@ import { readFileSync } from "node:fs";
 import test from "node:test";
 import { runInNewContext } from "node:vm";
 import ts from "typescript";
+import * as ErrorUtils from "./ErrorUtils.ts";
 import { shouldReconnectAfterClose } from "./ReconnectPolicy.ts";
 import { SessionId } from "./SessionId.ts";
 import { SessionIdDisplay } from "./SessionIdDisplay.ts";
+import * as VersionCompatibility from "./VersionCompatibility.ts";
 
 /** Compiles the actual plugin entry point for a simulated browser boundary. */
 function compile(name: string) {
@@ -15,6 +17,22 @@ function compile(name: string) {
     }).outputText;
 }
 
+/** Exports of a compiled module that Node's type stripping cannot load directly. */
+function compiledModule(name: string) {
+    const exports = {};
+    runInNewContext(compile(name), { exports });
+    return exports;
+}
+
+/** Resolves the imports of `plugin.ts`, stubbing only the code execution handler. */
+function pluginRequire(name: string) {
+    if (name === "./task-handlers/ExecuteCodeTaskHandler") return { ExecuteCodeTaskHandler: class {} };
+    if (name === "./TaskHandler") return compiledModule("TaskHandler.ts");
+    if (name === "./ErrorUtils") return ErrorUtils;
+    if (name === "./VersionCompatibility") return VersionCompatibility;
+    throw new Error(`Unexpected import: ${name}`);
+}
+
 function pluginUi() {
     const messages: any[] = [];
     const sockets: FakeSocket[] = [];
@@ -184,7 +202,7 @@ test("plugin reads the current file for each connection metadata request", () =>
             on() {},
         },
         mcp: undefined,
-        require: () => ({ ExecuteCodeTaskHandler: class {} }),
+        require: pluginRequire,
     });
     onMessage({ type: "connection-metadata-request", sessionId: "first" });
     penpot.currentFile = { id: "file-2", name: "Second design" };
@@ -223,7 +241,7 @@ test("plugin without penpotMgmt derives metadata from the open file", () => {
         exports: {},
         penpot,
         mcp: undefined,
-        require: () => ({ ExecuteCodeTaskHandler: class {} }),
+        require: pluginRequire,
     });
     onMessage({ type: "connection-metadata-request", sessionId: "first" });
     penpot.currentFile = null;
@@ -313,7 +331,7 @@ test("integrated plugin stays idle until explicitly connected, including while t
             setMcpStatus: (status: string) => statuses.push(status),
             on: (event: string, callback: () => void) => events.set(event, callback),
         },
-        require: () => ({ ExecuteCodeTaskHandler: class {} }),
+        require: pluginRequire,
     });
     assert.ok(!statuses.includes("connecting"));
     events.get("connect")!();

mcp/packages/server/src/Feedback.test.ts

@@ -70,6 +70,21 @@ async function connect(multiUser: boolean, token?: string) {
 
 const feedback = { subject: "Export fails", feedback: "Expected a PNG, received an error." };
 
+/**
+ * Collects all `pattern` values in a JSON schema.
+ */
+function schemaPatterns(schema: unknown): string[] {
+    if (Array.isArray(schema)) {
+        return schema.flatMap(schemaPatterns);
+    }
+    if (schema === null || typeof schema !== "object") {
+        return [];
+    }
+    return Object.entries(schema).flatMap(([key, value]) =>
+        key === "pattern" && typeof value === "string" ? [value] : schemaPatterns(value)
+    );
+}
+
 for (const multiUser of [false, true]) {
     test(`feedback is absent without a backend URI (multi-user=${multiUser})`, async () => {
         const connected = await connect(multiUser, "alice");
@@ -82,6 +97,10 @@ for (const multiUser of [false, true]) {
         const tool = (await connected.listTools()).tools.find((tool) => tool.name === "submit_feedback");
         assert.ok(tool);
         assert.deepEqual(tool.inputSchema.required, ["subject", "feedback"]);
+        // advertised patterns must also compile outside ECMAScript (ChatGPT uses Python)
+        const patterns = schemaPatterns(tool.inputSchema);
+        assert.ok(patterns.length > 0);
+        assert.ok(!patterns.some((pattern) => /\\[pP]\{/.test(pattern)), patterns.join(", "));
         const result = await connected.callTool({ name: "submit_feedback", arguments: feedback });
         assert.deepEqual(result.content, [
             { type: "text", text: "Feedback queued. It is emailed in the background, so delivery is not confirmed." },

mcp/packages/server/src/tools/SubmitFeedbackTool.ts

@@ -12,7 +12,9 @@ export class SubmitFeedbackArgs implements Feedback {
             .min(1)
             .max(120)
             .regex(/\S/)
-            .regex(/^\P{Cc}*$/u)
+            // the Unicode Cc category; `\P{Cc}` is avoided because clients that compile
+            // advertised patterns with other regex engines (e.g. Python) reject it
+            .regex(/^[^\x00-\x1F\x7F-\x9F]*$/)
             .describe("Short, specific summary without line breaks or other control characters (max 120 characters)."),
         feedback: z
             .string()

plugins/apps/plugin-api-test-suite/README.md

@@ -247,13 +247,17 @@ with `name`/`pinned`/`shared` properties and `open`, `moveTo`, `duplicate` and
 `remove` methods. `management.test.ts` shows the patterns:
 
 - Put every team and project a test creates under a name from
-  `temporaryName()` (`src/framework/temporary.ts`). Tests delete the teams they
-  create; the live CI driver deletes the teams and projects with that prefix
-  after the run. Runs from the plugin UI leave the projects behind. The tests
-  need nothing set up beforehand.
-- Assigning `name`, `pinned` or `shared` updates the object at once and saves
-  in the background. To check the saved result, list again inside
-  `waitForAsync()` (`src/tests/wait.ts`).
+  `temporaryName()` (`src/framework/temporary.ts`). The API cannot delete
+  teams, so the live CI driver deletes the teams and projects with that prefix
+  after the run. Runs from the plugin UI leave them behind. The tests need
+  nothing set up beforehand.
+- Assigning `name`, `pinned` or `shared` saves to the backend first and updates
+  the object only when the save succeeds. Wait for the new value with
+  `waitFor()` (`src/tests/wait.ts`) before reading it; to check what the
+  backend stored, list again inside `waitForAsync()`.
+- `Project.fileCount` is the count when the project was listed; creating or
+  moving files does not change it. List the project again to see the new
+  count.
 - Shape, page, file and library objects taken before `file.open()` (or
   `openFile`) are stale afterwards; look them up again in the new file.
   `ctx.board` belongs to the file the test started in, so only use it there.

plugins/apps/plugin-api-test-suite/src/tests/management.test.ts

@@ -193,6 +193,7 @@ describe('Management', () => {
 
       try {
         team.name = renamed;
+        await waitFor(() => team.name === renamed, { timeout: 5000 });
         expect(team.name).toBe(renamed);
         await waitForAsync(
           async () => (await currentTeam(ctx)).name === renamed,
@@ -225,7 +226,9 @@ describe('Management', () => {
       expect(file.projectId).toBe(created.id);
       expect(file.teamId).toBe(team.id);
       expect(file.shared).toBe(false);
-      expect(created.fileCount).toBe(1);
+      // fileCount is a listing snapshot, so list the project again.
+      expect(created.fileCount).toBe(0);
+      expect((await listedProject(ctx, created.id))?.fileCount).toBe(1);
       expect((await created.listFiles()).map((f) => f.id)).toEqual([file.id]);
       // Creating does not navigate.
       expect(ctx.penpotMgmt.workspace).toEqual(home);
@@ -240,6 +243,9 @@ describe('Management', () => {
 
       created.name = renamed;
       created.pinned = true;
+      await waitFor(() => created.name === renamed && created.pinned, {
+        timeout: 5000,
+      });
       expect(created.name).toBe(renamed);
       expect(created.pinned).toBe(true);
 
@@ -282,20 +288,18 @@ describe('Management', () => {
         name: temporaryName('team'),
       });
 
-      try {
-        const moved = await team.createProject({
-          name: temporaryName('moved'),
-        });
-        await moved.moveTo(target);
+      // Teams cannot be deleted through the API; the live CI driver deletes
+      // the temporary team after the run.
+      const moved = await team.createProject({
+        name: temporaryName('moved'),
+      });
+      await moved.moveTo(target);
 
-        expect(moved.teamId).toBe(target.id);
-        expect((await target.listProjects()).map((p) => p.id)).toContain(
-          moved.id,
-        );
-        expect(await listedProject(ctx, moved.id)).toBeUndefined();
-      } finally {
-        await target.remove();
-      }
+      expect(moved.teamId).toBe(target.id);
+      expect((await target.listProjects()).map((p) => p.id)).toContain(
+        moved.id,
+      );
+      expect(await listedProject(ctx, moved.id)).toBeUndefined();
     });
   });
 
@@ -305,6 +309,7 @@ describe('Management', () => {
       const parent = await temporaryProject(ctx);
 
       file.name = '  Renamed  ';
+      await waitFor(() => file.name === 'Renamed', { timeout: 5000 });
       expect(file.name).toBe('Renamed');
       await waitForAsync(
         async () => (await listedFile(parent, file.id))?.name === 'Renamed',
@@ -361,6 +366,9 @@ describe('Management', () => {
         expect(ctx.penpotMgmt.workspace.fileName).toBe(`${original} (renamed)`);
       } finally {
         file.name = original;
+        await waitFor(() => ctx.penpotMgmt.workspace.fileName === original, {
+          timeout: 5000,
+        });
       }
     });
   });
@@ -460,6 +468,7 @@ describe('Management', () => {
         );
 
       file.shared = true;
+      await waitFor(() => file.shared, { timeout: 5000 });
       expect(file.shared).toBe(true);
       await waitForAsync(isListed);
       expect(await isListed()).toBe(true);