From 1dfd501d83a2ccccc76fed2b69ef3d769eb3d9e9 Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Mon, 5 Oct 2026 14:06:26 +0000 Subject: [PATCH] feat: name the call and list every problem in validation errors (#911) * feat: name the call and list every problem in validation errors Tool arguments were checked with v.parse, whose error names only the first problem and neither the tool nor the argument, so a model that sent a wrong create_shape call read 'Expected string but received 42'. Tool arguments in Core, which AI chat, MCP, the CLI, and WebMCP all reach, the automation bridge's file commands, design JSX component properties, and gradient stops now throw a heading that names the call followed by v.summarize, which lists each issue with its path. parseToolArgs is exported for the app's own parses of tool arguments. * test(design-jsx): pass a deliberately wrong property value through the types The test checks what a script sees for a non-string instance property, which the Instance types reject at compile time. * docs: say that MCP clients get the MCP SDK's validation report The MCP SDK validates tool arguments against the registered schema before OpenPencil's handler runs, so an MCP client already received every problem with its path and does not see parseToolArgs' message. --- CHANGELOG.md | 1 + packages/core/src/tools/index.ts | 8 +++- packages/core/src/tools/schema.ts | 17 ++++++++- packages/core/tests/tools/schema.test.ts | 38 +++++++++++++++++++ .../design-jsx/src/component-properties.ts | 20 ++++++++-- packages/design-jsx/src/paints.ts | 13 +++---- packages/design-jsx/src/validation.ts | 15 ++++++++ packages/design-jsx/tests/paints.test.ts | 4 +- src/app/automation/bridge/file-handlers.ts | 8 ++-- src/app/automation/webmcp/registration.ts | 5 +-- .../render/jsx/component/properties.test.ts | 10 +++++ 11 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 packages/core/tests/tools/schema.test.ts create mode 100644 packages/design-jsx/src/validation.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 91c26b272..9d82626f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ ### Changed +- Name the tool and list every invalid argument with where it is when AI chat, the CLI, or WebMCP calls a tool wrongly, as MCP clients already saw, as in `Invalid arguments for create_shape:` followed by `× Invalid type: Expected ("FRAME" | …) but received "CIRCLE"` and `→ at type`. Design JSX component properties and gradient stops report their problems the same way. Previously only the first problem was named, without the tool or the argument. - HTML and Tailwind JSX export write variable-bound colors, spacing, radii, borders, sizes, type sizes, and opacity as the tokens they come from, such as `var(--color-primary)` or `bg-primary`, and put layers set to another mode in it with an attribute such as `data-theme="dark"`. Values CSS would not resolve as the canvas draws them stay literal, and standalone HTML includes the stylesheet for the tokens it uses. - Clicking selects layers as in Figma: inside a top-level frame or section, a click selects the frame's direct child rather than the deepest layer, the empty part of a top-level frame or section that holds layers selects nothing, and once a layer is selected, clicks reach its siblings and cousins. Double-click goes one level deeper and ⌘-click (Ctrl-click on Windows and Linux) selects the deepest layer. A marquee started inside a top-level frame or section selects that container's layers, and from the page it selects a frame or section that holds layers only when fully enclosed. - Drag, draw, duplicate, and paste follow Figma. A dragged layer lands in the frame under the cursor and leaves its frame as soon as the cursor does; groups and locked frames never take a drop, a component set takes back only its own variants, and a layer inside a group stays in it unless dropped on another frame. Groups and booleans fit their children after a move or nudge, and a group whose last layer leaves is removed. Pressing inside a selected frame, group, or component set drags it rather than the layer under the cursor, and an auto layout child dragged out lands where you drop it. Hold Space while dragging to keep layers in their parents, Shift to move along one axis, or Control to drop into auto layout as an absolute-positioned layer. Locked layers stay put when the rest of the selection moves. Shapes drawn inside a frame, auto layout, or slot go into it, a frame drawn over layers takes in the ones it fully covers, and a section does so when moved or resized too. Wrapped auto layout inserts on the line under the cursor. Duplicates keep their names, and duplicating a main component with ⌘D or Alt-drag creates an instance. ⌘D duplicates in place, placing a lone top-level frame's copy to its right, and ⌘V keeps the copied position, centering an axis that doesn't fit the selected frame; **Paste here** still pastes at the cursor. diff --git a/packages/core/src/tools/index.ts b/packages/core/src/tools/index.ts index e373b87b1..53bb8a3d2 100644 --- a/packages/core/src/tools/index.ts +++ b/packages/core/src/tools/index.ts @@ -17,7 +17,13 @@ export { toolChangesDocument } from './schema' export type { ToolDef, ToolExecution, ToolCapability } from './schema' -export { isAtomicTool, isToolExposed, type ToolInterface, type ToolExposure } from './schema' +export { + isAtomicTool, + isToolExposed, + parseToolArgs, + type ToolInterface, + type ToolExposure +} from './schema' export { toolNumber } from './input' export { toolsToAI, buildDebugLog } from './ai-adapter' export type { ToolLogEntry, ToolDebugLog, AIAdapterOptions, StepBudget } from './ai-adapter' diff --git a/packages/core/src/tools/schema.ts b/packages/core/src/tools/schema.ts index c82ac379a..b86310823 100644 --- a/packages/core/src/tools/schema.ts +++ b/packages/core/src/tools/schema.ts @@ -65,10 +65,25 @@ export function defineTool

( get mutates() { return def.execution.mutation !== 'none' }, - execute: (figma, args) => def.execute(figma, v.parse(def.input, args)) + execute: (figma, args) => def.execute(figma, parseToolArgs(def.name, def.input, args)) } } +/** + * Every tool run goes through here, so a wrong call from AI chat, the CLI, or WebMCP names the + * tool and lists each problem with its argument, as `v.summarize` formats them. MCP clients get + * the MCP SDK's own report, which validates the same schema before the handler runs. + */ +export function parseToolArgs( + name: string, + schema: S, + args: unknown +): v.InferOutput { + const result = v.safeParse(schema, args) + if (result.success) return result.output + throw new Error(`Invalid arguments for ${name}:\n${v.summarize(result.issues)}`) +} + export function toolChangesDocument(def: Pick): boolean { return def.execution.mutation === 'properties' || def.execution.mutation === 'document' } diff --git a/packages/core/tests/tools/schema.test.ts b/packages/core/tests/tools/schema.test.ts new file mode 100644 index 000000000..a4812c70e --- /dev/null +++ b/packages/core/tests/tools/schema.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, test } from 'bun:test' + +import * as v from 'valibot' + +import { FigmaAPI } from '@open-pencil/core/figma-api' +import { defineTool } from '@open-pencil/core/tools' +import { SceneGraph } from '@open-pencil/scene-graph' + +const tool = defineTool({ + name: 'place_box', + description: 'Places a box', + execution: { kind: 'sync', mutation: 'none' }, + input: v.object({ + kind: v.picklist(['FRAME', 'RECTANGLE']), + width: v.pipe(v.number(), v.minValue(1)) + }), + execute: (_figma, args) => args +}) + +describe('tool arguments', () => { + test('name the tool and list every problem with its argument', () => { + const figma = new FigmaAPI(new SceneGraph()) + expect(() => tool.execute(figma, { kind: 'CIRCLE', width: 0 })).toThrow( + [ + 'Invalid arguments for place_box:', + '× Invalid type: Expected ("FRAME" | "RECTANGLE") but received "CIRCLE"', + ' → at kind', + '× Invalid value: Expected >=1 but received 0', + ' → at width' + ].join('\n') + ) + }) + + test('pass valid arguments through', () => { + const figma = new FigmaAPI(new SceneGraph()) + expect(tool.execute(figma, { kind: 'FRAME', width: 10 })).toEqual({ kind: 'FRAME', width: 10 }) + }) +}) diff --git a/packages/design-jsx/src/component-properties.ts b/packages/design-jsx/src/component-properties.ts index 779f7a3db..f78cd7524 100644 --- a/packages/design-jsx/src/component-properties.ts +++ b/packages/design-jsx/src/component-properties.ts @@ -8,6 +8,8 @@ import type { SceneNode } from '@open-pencil/scene-graph' +import { parseScriptInput } from './validation' + const definitionSchema = v.object({ id: v.string(), name: v.string(), @@ -31,7 +33,11 @@ export function componentMetadata( if (props.properties !== undefined && type !== 'INSTANCE') { if (type !== 'COMPONENT' && type !== 'COMPONENT_SET') throw new Error('Only components, component sets, and instances accept properties') - const definitions = v.parse(v.array(definitionSchema), props.properties) + const definitions = parseScriptInput( + 'Invalid properties', + v.array(definitionSchema), + props.properties + ) if (new Set(definitions.map((definition) => definition.id)).size !== definitions.length) throw new Error('Duplicate component property IDs') const variantNames = definitions @@ -42,7 +48,11 @@ export function componentMetadata( result.componentPropertyDefinitions = definitions } if (props.propertyRefs !== undefined) { - const references = v.parse(v.array(referenceSchema), props.propertyRefs) + const references = parseScriptInput( + 'Invalid propertyRefs', + v.array(referenceSchema), + props.propertyRefs + ) for (const reference of references) { if (reference.field === 'TEXT' && type !== 'TEXT') throw new Error('TEXT properties require a text node') @@ -88,7 +98,11 @@ export function assignComponentProperties( input: unknown ): void { if (input === undefined) return - const assignments = v.parse(v.record(v.string(), v.string()), input) + const assignments = parseScriptInput( + 'Invalid properties on ', + v.record(v.string(), v.string()), + input + ) const definitions = componentPropertyDefinitions(graph, instance) for (const [id, value] of Object.entries(assignments)) { const definition = definitions.find((item) => item.id === id) diff --git a/packages/design-jsx/src/paints.ts b/packages/design-jsx/src/paints.ts index 10830f025..d7029f0b0 100644 --- a/packages/design-jsx/src/paints.ts +++ b/packages/design-jsx/src/paints.ts @@ -11,6 +11,8 @@ import { colorToFill, parseColor } from '@open-pencil/scene-graph/color' import { TRANSPARENT } from '@open-pencil/scene-graph/constants' import type { Color } from '@open-pencil/scene-graph/primitives' +import { parseScriptInput } from './validation' + export type PaintColor = string | Color export type PaintStop = readonly [PaintColor, number] | { color: PaintColor; position: number } @@ -58,13 +60,10 @@ type GradientType = keyof typeof GRADIENT_HELPERS * and what was wrong, as Valibot describes it. */ function parseStops(type: GradientType, stops: unknown): PaintStop[] { - const parsed = v.safeParse(stopsSchema, stops) - if (parsed.success) return parsed.output - const [issue] = parsed.issues - // Only the array's items are checked one level down, so a path is a stop's index. - const stop = v.getDotPath(issue) - throw new Error( - `${GRADIENT_HELPERS[type]}() expects an array of stops, such as [['#3b82f6', 0], ['#8b5cf6', 1]]: ${issue.message}${stop === null ? '' : ` (stop ${stop})`}` + return parseScriptInput( + `${GRADIENT_HELPERS[type]}() expects an array of stops, such as [['#3b82f6', 0], ['#8b5cf6', 1]]`, + stopsSchema, + stops ) } diff --git a/packages/design-jsx/src/validation.ts b/packages/design-jsx/src/validation.ts new file mode 100644 index 000000000..8a2f7f5bf --- /dev/null +++ b/packages/design-jsx/src/validation.ts @@ -0,0 +1,15 @@ +import * as v from 'valibot' + +/** + * Parse a value a design JSX script passed, or throw `heading` followed by every problem and + * where it is, as `v.summarize` lists them. Core words tool arguments the same way. + */ +export function parseScriptInput( + heading: string, + schema: S, + value: unknown +): v.InferOutput { + const result = v.safeParse(schema, value) + if (result.success) return result.output + throw new Error(`${heading}:\n${v.summarize(result.issues)}`) +} diff --git a/packages/design-jsx/tests/paints.test.ts b/packages/design-jsx/tests/paints.test.ts index db6070205..0133ca0c9 100644 --- a/packages/design-jsx/tests/paints.test.ts +++ b/packages/design-jsx/tests/paints.test.ts @@ -22,13 +22,13 @@ describe('gradient helpers', () => { [circular, 'Object'] ])('say what they expect when the stops are not an array (%p)', (stops, received) => { expect(() => linearGradient(stops as never)).toThrow( - `${expected}: Invalid type: Expected Array but received ${received}` + `${expected}:\n× Invalid type: Expected Array but received ${received}` ) }) test('name the stop that is wrong and what it was', () => { expect(() => radialGradient([['#000000', 0], ['#ffffff']] as never)).toThrow( - "radialGradient() expects an array of stops, such as [['#3b82f6', 0], ['#8b5cf6', 1]]: Expected [color, position] or { color, position } but received Array (stop 1)" + "radialGradient() expects an array of stops, such as [['#3b82f6', 0], ['#8b5cf6', 1]]:\n× Expected [color, position] or { color, position } but received Array\n → at 1" ) }) }) diff --git a/src/app/automation/bridge/file-handlers.ts b/src/app/automation/bridge/file-handlers.ts index d285e8c51..eba2951a3 100644 --- a/src/app/automation/bridge/file-handlers.ts +++ b/src/app/automation/bridge/file-handlers.ts @@ -1,5 +1,7 @@ import * as v from 'valibot' +import { parseToolArgs } from '@open-pencil/core/tools' + import { resolveAutomationTarget, responseWithTarget, @@ -31,7 +33,7 @@ async function saveWithoutPrompt(store: AutomationTarget['store'], path?: string } export async function handleSaveFile(target: AutomationTarget, args: unknown): Promise { - const { path } = v.parse(saveArgsSchema, args) + const { path } = parseToolArgs('save_file', saveArgsSchema, args) await saveWithoutPrompt(target.store, path) return { ok: true, result: { saved: true } } } @@ -48,7 +50,7 @@ export async function ensureTauriParentDirectory(path: string): Promise { } export async function handleCloseFile(target: AutomationTarget, args: unknown): Promise { - const { path, unsaved } = v.parse(closeArgsSchema, args) + const { path, unsaved } = parseToolArgs('close_file', closeArgsSchema, args) const store = target.store if (store.hasUnsavedChanges()) { if (unsaved === 'error') { @@ -67,7 +69,7 @@ export async function handleNewDocument( _target: AutomationTarget, args: unknown ): Promise { - const { path } = v.parse(saveArgsSchema, args) + const { path } = parseToolArgs('new_document', saveArgsSchema, args) const tab = createTab() if (path) { try { diff --git a/src/app/automation/webmcp/registration.ts b/src/app/automation/webmcp/registration.ts index 946dd4d25..aa73d61ca 100644 --- a/src/app/automation/webmcp/registration.ts +++ b/src/app/automation/webmcp/registration.ts @@ -1,9 +1,8 @@ // eslint-disable-next-line open-pencil/no-mixed-case-acronym-identifiers -- Upstream export spelling. import { toJsonSchema as toJSONSchema } from '@valibot/to-json-schema' -import * as v from 'valibot' import type { WebMCP } from 'webmcp-types' -import type { ToolDef } from '@open-pencil/core/tools' +import { parseToolArgs, type ToolDef } from '@open-pencil/core/tools' import { getWebMCPTools, type WebMCPMode } from './policy' @@ -45,7 +44,7 @@ export function registerWebMCPTools( : lifetime.signal lifetime.signal.throwIfAborted() signal.throwIfAborted() - const args = v.parse(schema, input) + const args = parseToolArgs(def.name, schema, input) const target = getTarget() const result = await target.execute(def, args, signal) // A synchronous mutation has committed; a later abort cannot roll it back. diff --git a/tests/engine/render/jsx/component/properties.test.ts b/tests/engine/render/jsx/component/properties.test.ts index 189271aee..4fb6e84c7 100644 --- a/tests/engine/render/jsx/component/properties.test.ts +++ b/tests/engine/render/jsx/component/properties.test.ts @@ -109,6 +109,16 @@ test('variant selection ignores non-variant instance props', async () => { expect(graph.getChildren(instance.id)[0].text).toBe('Changed') }) +test('an assignment of the wrong type names the prop and the property', async () => { + const { graph, component } = await setup() + await expect( + // A script can pass any value; the types only describe valid calls. + renderTree(graph, Instance({ of: component.id, properties: { message: 42 } as never })) + ).rejects.toThrow( + 'Invalid properties on :\n× Invalid type: Expected string but received 42\n → at message' + ) +}) + test('invalid assignments remove the new instance and do not edit its source', async () => { const { graph, component } = await setup() const count = graph.nodes.size