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.
This commit is contained in:
parent
37bc706a59
commit
1dfd501d83
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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'
|
||||
|
|
|
|||
|
|
@ -65,10 +65,25 @@ export function defineTool<P extends v.ObjectEntries, R>(
|
|||
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<S extends v.GenericSchema>(
|
||||
name: string,
|
||||
schema: S,
|
||||
args: unknown
|
||||
): v.InferOutput<S> {
|
||||
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<ToolDef, 'execution'>): boolean {
|
||||
return def.execution.mutation === 'properties' || def.execution.mutation === 'document'
|
||||
}
|
||||
|
|
|
|||
38
packages/core/tests/tools/schema.test.ts
Normal file
38
packages/core/tests/tools/schema.test.ts
Normal file
|
|
@ -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 })
|
||||
})
|
||||
})
|
||||
|
|
@ -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 <Instance>',
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
15
packages/design-jsx/src/validation.ts
Normal file
15
packages/design-jsx/src/validation.ts
Normal file
|
|
@ -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<S extends v.GenericSchema>(
|
||||
heading: string,
|
||||
schema: S,
|
||||
value: unknown
|
||||
): v.InferOutput<S> {
|
||||
const result = v.safeParse(schema, value)
|
||||
if (result.success) return result.output
|
||||
throw new Error(`${heading}:\n${v.summarize(result.issues)}`)
|
||||
}
|
||||
|
|
@ -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"
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<unknown> {
|
||||
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<void> {
|
|||
}
|
||||
|
||||
export async function handleCloseFile(target: AutomationTarget, args: unknown): Promise<unknown> {
|
||||
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<unknown> {
|
||||
const { path } = v.parse(saveArgsSchema, args)
|
||||
const { path } = parseToolArgs('new_document', saveArgsSchema, args)
|
||||
const tab = createTab()
|
||||
if (path) {
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 <Instance>:\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
|
||||
|
|
|
|||
Loading…
Reference in a new issue