From 589d9b1e304cdefd84012e1b58f2c10adf3ca132 Mon Sep 17 00:00:00 2001 From: u8array Date: Tue, 22 Sep 2026 10:34:10 +0200 Subject: [PATCH] fix(mcp): read a design file handed over as a JSON string A partial variable in an envelope is completed by create_draft's builder. --- packages/core/src/lib/designInput.ts | 38 ++++++--- packages/core/src/types/Variable.ts | 2 + packages/mcp-server/src/boundary.ts | 75 +++++++++++++----- packages/mcp-server/src/patchOps.ts | 4 +- packages/mcp-server/src/server.test.ts | 27 +++++++ packages/mcp-server/src/testFixtures.ts | 21 +++++ packages/mcp-server/src/tools.test.ts | 100 +++++++++++++++++++++++- 7 files changed, 230 insertions(+), 37 deletions(-) diff --git a/packages/core/src/lib/designInput.ts b/packages/core/src/lib/designInput.ts index cf172368..a0cacd10 100644 --- a/packages/core/src/lib/designInput.ts +++ b/packages/core/src/lib/designInput.ts @@ -213,17 +213,16 @@ export function duplicateVariableIssue(variables: readonly Variable[]): string | return null; } -/** Ids and free ^FN slots for named inputs. Duplicates would merge fields silently, so they are refused. */ -export function buildVariables( +/** Ids, free ^FN slots and stripped defaults for named inputs. Duplicates are the caller's policy. */ +export function completeVariables( inputs: readonly VariableInput[], existing: readonly Variable[] = [], ): { value: Variable[] } | { error: string } { - const names = new Set(existing.map((v) => v.name)); const taken: number[] = [ ...existing.map((v) => v.fnNumber), ...inputs.flatMap((v) => (v.fnNumber === undefined ? [] : [v.fnNumber])), ]; - const usedIds = new Set(existing.map((v) => v.id)); + const usedIds = new Set([...existing.map((v) => v.id), ...inputs.flatMap((v) => (v.id === undefined ? [] : [v.id]))]); const out: Variable[] = []; for (const input of inputs) { // Trimmed like every reader trims, or the stored name could never be addressed again. @@ -231,8 +230,6 @@ export function buildVariables( if (!isValidVariableName(name)) { return { error: `Invalid variable name: ${JSON.stringify(input.name)}` }; } - if (names.has(name)) return { error: `Duplicate variable name: ${name}` }; - names.add(name); if (input.fnNumber !== undefined && (input.fnNumber < FN_NUMBER_MIN || input.fnNumber > FN_NUMBER_MAX)) { return { error: `^FN slot must be ${FN_NUMBER_MIN}-${FN_NUMBER_MAX} (got ${input.fnNumber})` }; } @@ -243,11 +240,15 @@ export function buildVariables( fnNumber = free; taken.push(free); } - let idIndex = existing.length + out.length + 1; - while (usedIds.has(`var-${idIndex}`)) idIndex++; - usedIds.add(`var-${idIndex}`); + let id = input.id; + if (id === undefined) { + let idIndex = existing.length + out.length + 1; + while (usedIds.has(`var-${idIndex}`)) idIndex++; + id = `var-${idIndex}`; + usedIds.add(id); + } out.push({ - id: `var-${idIndex}`, + id, name, fnNumber, // A default carrying its own «…» would resolve in preview and emit verbatim, see stripMarkerDelimiters. @@ -255,6 +256,19 @@ export function buildVariables( ...(input.comment !== undefined ? { comment: input.comment } : {}), }); } - const dup = duplicateVariableIssue([...existing, ...out]); - return dup === null ? { value: out } : { error: dup }; + return { value: out }; +} + +/** create_draft's variables: completed, then duplicates refused, since they would merge fields silently. */ +export function buildVariables( + inputs: readonly VariableInput[], + existing: readonly Variable[] = [], +): { value: Variable[] } | { error: string } { + const built = completeVariables(inputs, existing); + if ("error" in built) return built; + const all = [...existing, ...built.value]; + const name = all.map((v) => v.name).find((n, i, names) => names.indexOf(n) !== i); + if (name !== undefined) return { error: `Duplicate variable name: ${name}` }; + const dup = duplicateVariableIssue(all); + return dup === null ? built : { error: dup }; } diff --git a/packages/core/src/types/Variable.ts b/packages/core/src/types/Variable.ts index 6a07c464..80cfe6de 100644 --- a/packages/core/src/types/Variable.ts +++ b/packages/core/src/types/Variable.ts @@ -17,6 +17,8 @@ export const variableSchema = z.object({ export type Variable = z.infer; export interface VariableInput { + /** Kept when given: a file's column mapping addresses the variable by it. */ + id?: string; name: string; defaultValue?: string; /** Explicit slot. When omitted, the store assigns the next free number. */ diff --git a/packages/mcp-server/src/boundary.ts b/packages/mcp-server/src/boundary.ts index d16bb6ab..1e17b0af 100644 --- a/packages/mcp-server/src/boundary.ts +++ b/packages/mcp-server/src/boundary.ts @@ -10,8 +10,8 @@ import { } from "@zplab/core/lib/designFile"; import { buildVariables, + completeVariables, duplicateVariableIssue, - freeId, normalizeLeaves, propIssues, RAW_GRAPHIC_PROPS, @@ -25,7 +25,7 @@ import { designSizeIssue } from "@zplab/core/lib/designLimits"; import { getAllLeaves, walkObjects, type LabelObject } from "@zplab/core/types/Group"; import { errorMessage } from "@zplab/core/lib/errorMessage"; import { DPMM_VALUES, isDpmm, type DeviceFontLabel, type Dpmm, type LabelConfig } from "@zplab/core/types/LabelConfig"; -import type { Variable, VariableInput } from "@zplab/core/types/Variable"; +import { variableSchema, type Variable, type VariableInput } from "@zplab/core/types/Variable"; // Re-exported so the tools keep their names. export { buildVariables, propIssues, typeIssues }; @@ -52,7 +52,7 @@ export const variableInputSchema: z.ZodType = z.object({ fnNumber: z.number().int().optional(), comment: z.string().optional(), }); -export type VariableInputJson = VariableInput; +export type VariableInputJson = Omit; export const createDraftShape = { widthMm: z.number().positive(), @@ -80,7 +80,12 @@ export interface DesignFileJson { variables?: Variable[]; } -export const designFileEnvelopeSchema = z.object({ designFile: z.record(z.string(), z.unknown()) }); +/** Smaller models tend to hand nested JSON over as a string, so both forms are the same design. */ +export const designFileInputSchema = z + .union([z.record(z.string(), z.unknown()), z.string()], { error: "designFile must be the design object or its JSON as a string" }) + .describe("The design file as an object, or the same JSON as a string."); + +export const designFileEnvelopeSchema = z.object({ designFile: designFileInputSchema }); /** export_zpl: `metadata` keeps ZPLab's ^FX comments for a lossless re-import. */ export const exportZplInputSchema = designFileEnvelopeSchema.extend({ metadata: z.boolean().optional() }); @@ -130,28 +135,56 @@ export function duplicateVariableError(variables: readonly Variable[]): ToolErro return issue === null ? null : { ok: false, errors: [issue] }; } -/** The model addresses variables by name, so a missing id is filled in. Object ids stay - * required: patch_design addresses objects by id. */ -function withVariableIds(designFile: unknown): unknown { - if (!designFile || typeof designFile !== "object") return designFile; - const { variables } = designFile as { variables?: unknown }; - if (!Array.isArray(variables)) return designFile; - const taken = new Set(); - for (const v of variables) { - if (v && typeof v === "object" && typeof (v as { id?: unknown }).id === "string") taken.add((v as { id: string }).id); - } - const filled = variables.map((v) => { - if (!v || typeof v !== "object" || (v as { id?: unknown }).id !== undefined) return v; - const id = freeId("var", taken); - taken.add(id); - return { ...(v as object), id }; +const isRecord = (v: unknown): v is Record => typeof v === "object" && v !== null; +const isComplete = (v: unknown): v is Variable => variableSchema.safeParse(v).success; +const isPartial = (v: unknown): v is Record & { name: string } => + isRecord(v) && typeof v.name === "string" && !isComplete(v); +const typed = (v: unknown, type: "string" | "number"): T | undefined => (typeof v === type ? (v as T) : undefined); + +/** A partial variable is completed like create_draft's. A mistyped field comes back as sent, so the + * schema names it alone. Duplicates go to the loader's repair. */ +function withCompleteVariables(designFile: unknown): { ok: true; value: unknown } | ToolError { + if (!isRecord(designFile) || !Array.isArray(designFile.variables)) return { ok: true, value: designFile }; + const { variables } = designFile; + const partial = variables.filter(isPartial); + if (partial.length === 0) return { ok: true, value: designFile }; + const inputs = partial.map((v) => ({ + name: v.name, + id: typed(v.id, "string"), + fnNumber: typed(v.fnNumber, "number"), + defaultValue: typed(v.defaultValue, "string"), + comment: typed(v.comment, "string"), + })); + const built = completeVariables(inputs, variables.filter(isComplete)); + if ("error" in built) return { ok: false, errors: [built.error] }; + let next = 0; + const completed = variables.map((v) => { + if (!isPartial(v)) return v; + const full = built.value[next++] as unknown as Record; + const mistyped = Object.entries(v).filter(([k, x]) => x !== undefined && typeof x !== typeof full[k]); + return { ...full, ...Object.fromEntries(mistyped) }; }); - return { ...(designFile as object), variables: filled }; + return { ok: true, value: { ...designFile, variables: completed } }; +} + +function decodeDesignFile(designFile: unknown): { ok: true; value: unknown } | ToolError { + if (typeof designFile !== "string") return { ok: true, value: designFile }; + try { + const value: unknown = JSON.parse(designFile); + if (typeof value === "string") return { ok: false, errors: ["designFile: the JSON decodes to another string, send the design once"] }; + return { ok: true, value }; + } catch (e) { + return { ok: false, errors: [`designFile: not valid JSON, ${errorMessage(e)}`] }; + } } export function parseEnvelope(designFile: unknown): { ok: true; value: DesignFile } | ToolError { + const input = decodeDesignFile(designFile); + if (!input.ok) return input; + const completed = withCompleteVariables(input.value); + if (!completed.ok) return completed; try { - const parsed = parseDesignFile(JSON.stringify(withVariableIds(designFile))); + const parsed = parseDesignFile(JSON.stringify(completed.value)); if (!parsed.ok) return { ok: false, errors: [designFileErrors[parsed.error], ...(parsed.issues ?? [])] }; const issues = labelConfigIssues(parsed.value.label); if (issues.length > 0) return { ok: false, errors: issues }; diff --git a/packages/mcp-server/src/patchOps.ts b/packages/mcp-server/src/patchOps.ts index 0c6c0f87..397b3290 100644 --- a/packages/mcp-server/src/patchOps.ts +++ b/packages/mcp-server/src/patchOps.ts @@ -1,6 +1,6 @@ // patch_design: the envelope around the core op reducer, plus the report the agent reads. -import { objectInputSchema, pagesSizeError, parseEnvelope, unknownPropNotes, variableInputSchema, type DesignFileJson, type ToolError } from "./boundary.js"; +import { designFileInputSchema, objectInputSchema, pagesSizeError, parseEnvelope, unknownPropNotes, variableInputSchema, type DesignFileJson, type ToolError } from "./boundary.js"; import { reportFor, type ObjectBounds, type ObjectOverlap, type PreflightWarning } from "./report.js"; import { z } from "zod"; @@ -58,7 +58,7 @@ export const MAX_PATCH_OPS = 1000; export const patchOperationsSchema = z.array(patchOpSchema).min(1).max(MAX_PATCH_OPS); export const patchDesignShape = { - designFile: z.record(z.string(), z.unknown()), + designFile: designFileInputSchema, operations: patchOperationsSchema, }; diff --git a/packages/mcp-server/src/server.test.ts b/packages/mcp-server/src/server.test.ts index a3380c4a..317d7c84 100644 --- a/packages/mcp-server/src/server.test.ts +++ b/packages/mcp-server/src/server.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { buildServer, SERVER_INSTRUCTIONS } from "./server"; +import { demoLabel } from "./testFixtures"; async function connect(hosted = false): Promise { const [clientT, serverT] = InMemoryTransport.createLinkedPair(); @@ -29,6 +30,32 @@ describe("server handshake", () => { }); }); +describe("the designFile argument", () => { + const envelopeTools = ["validate_draft", "patch_design", "export_zpl", "open_in_app"]; + + it("is listed as object or string on every envelope tool", async () => { + const { tools } = await (await connect(true)).listTools(); + for (const name of envelopeTools) { + const tool = tools.find((t) => t.name === name); + expect(tool, name).toBeDefined(); + const schema = tool?.inputSchema as { properties: Record }; + expect(schema.properties.designFile?.anyOf?.map((v) => v.type), name).toEqual(["object", "string"]); + } + }); + + it("passes the MCP input check as a string and reads the design", async () => { + const client = await connect(); + const res = (await client.callTool({ + name: "validate_draft", + arguments: { designFile: JSON.stringify(demoLabel) }, + })) as { isError?: boolean; content: { text: string }[] }; + expect(res.isError).toBeFalsy(); + const body = JSON.parse(res.content[0]?.text ?? "{}") as { ok: boolean; bounds: { objectId: string }[] }; + expect(body.ok).toBe(true); + expect(body.bounds.map((b) => b.objectId)).toEqual(["titel", "preis", "hinweis", "ean", "rueck"]); + }); +}); + describe("prepared workflows", () => { const names = async (hosted: boolean) => (await (await connect(hosted)).listPrompts()).prompts.map((p) => p.name); diff --git a/packages/mcp-server/src/testFixtures.ts b/packages/mcp-server/src/testFixtures.ts index 5069af8d..23930e22 100644 --- a/packages/mcp-server/src/testFixtures.ts +++ b/packages/mcp-server/src/testFixtures.ts @@ -33,6 +33,27 @@ export function textObject(id: string, content: string) { }; } +/** A label the way a model hand-builds it: bytes a JSON round-trip must carry unchanged. */ +export const demoLabel = { + schemaVersion: 6, + label: { widthMm: 100, heightMm: 60, dpmm: 8 }, + variables: [ + { name: "bezeichnung", defaultValue: "Bio-Müsli • Früchte" }, + { name: "preis", defaultValue: "4,99 €" }, + ], + pages: [ + { + objects: [ + textObject("titel", "«bezeichnung»"), + { ...textObject("preis", "Preis: «preis»"), y: 60 }, + { ...textObject("hinweis", "• Ohne Zusätze • Äpfel aus der Region"), y: 110 }, + { id: "ean", type: "ean13", x: 10, y: 200, rotation: 0, props: { content: "400638133393", height: 80 } }, + ], + }, + { objects: [textObject("rueck", "Rückseite: Zutaten • Nährwerte")] }, + ], +}; + /** Minimal valid single-page design file. */ export const designFile = { schemaVersion: 3, diff --git a/packages/mcp-server/src/tools.test.ts b/packages/mcp-server/src/tools.test.ts index 41515c31..e2a151ec 100644 --- a/packages/mcp-server/src/tools.test.ts +++ b/packages/mcp-server/src/tools.test.ts @@ -4,12 +4,12 @@ // who opens only this file sees why the values look the way they do. import { describe, it, expect } from "vitest"; import { z } from "zod"; -import { rasterImageShape, buildCurrentDesignResult, createDraft, patchDesign, patchDesignShape, rasterImageResult, createDraftShape, validateDraft, exportZpl, getSchema, importZpl, validateZpl } from "./tools"; +import { rasterImageShape, buildCurrentDesignResult, createDraft, patchDesign, patchDesignShape, rasterImageResult, createDraftShape, validateDraft, exportZpl, getSchema, importZpl, validateZpl, openInApp } from "./tools"; import { parseEnvelope } from "./boundary.js"; import { wireBounds } from "@zplab/core/types/propSpec"; import { IMAGE_PROP_SPECS } from "@zplab/core/registry/image"; import { ObjectRegistry } from "@zplab/core/registry"; -import { textObject } from "./testFixtures"; +import { demoLabel, textObject } from "./testFixtures"; import { serializeDesign } from "@zplab/core/lib/designFile"; /** Assert a tool succeeded and narrow away the ToolError branch. */ @@ -839,6 +839,66 @@ describe("agent-facing reporting", () => { expect(parsed.value.variables.map((v) => v.id)).toEqual(["var-1", "var-2"]); }); + it("completes a partial variable like create_draft, around the complete ones", () => { + const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); + const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[] }; + file.variables = [ + { name: "LOT", defaultValue: "\u00abx\u00bb" }, + { id: "var-9", name: "QTY", fnNumber: 1, defaultValue: "1" }, + { id: "keep", name: "BATCH" }, + ]; + const parsed = parseEnvelope(file); + expect(parsed.ok).toBe(true); + if (!parsed.ok) return; + expect(parsed.value.variables.map((v) => [v.id, v.fnNumber, v.defaultValue])).toEqual([["var-2", 2, "x"], ["var-9", 1, "1"], ["keep", 3, ""]]); + }); + + it("keeps a partial variable's id, so its column binding stays attached", () => { + const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); + const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[]; csvMapping?: unknown }; + file.variables = [{ id: "mine", name: "LOT" }]; + file.csvMapping = { bindings: { mine: "Lot" }, headerSnapshot: ["Lot"] }; + const parsed = parseEnvelope(file); + expect(parsed.ok).toBe(true); + if (!parsed.ok) return; + expect(parsed.value.variables[0]?.id).toBe("mine"); + expect(parsed.value.columnMapping).toMatchObject({ bindings: { mine: "Lot" } }); + }); + + it("repairs duplicate slots among partial variables like the loader does for complete ones", () => { + const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); + const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[] }; + file.variables = [{ name: "A", fnNumber: 1 }, { name: "B", fnNumber: 1 }]; + const parsed = parseEnvelope(file); + expect(parsed.ok).toBe(true); + if (!parsed.ok) return; + expect(parsed.value.variables.map((v) => v.fnNumber)).toEqual([1, 2]); + }); + + it("lets the schema name a mistyped field, and nothing else", () => { + const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); + const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[] }; + file.variables = [{ name: "P", fnNumber: "1" }]; + const result = parseEnvelope(file); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.errors.slice(1)).toEqual([expect.stringMatching(/^variables\.0\.fnNumber: /)]); + file.variables = [{ name: "P", comment: null }]; + const nulled = parseEnvelope(file); + expect(nulled.ok).toBe(false); + if (nulled.ok) return; + expect(nulled.errors.slice(1)).toEqual([expect.stringMatching(/^variables\.0\.comment: /)]); + }); + + it("refuses a partial variable with create_draft's words", () => { + const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); + const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[] }; + file.variables = [{ name: "" }]; + expect(parseEnvelope(file)).toEqual({ ok: false, errors: ['Invalid variable name: ""'] }); + file.variables = [...Array.from({ length: 99 }, (_, i) => ({ id: `v${i}`, name: `v${i}`, fnNumber: i + 1, defaultValue: "" })), { name: "more" }]; + expect(parseEnvelope(file)).toEqual({ ok: false, errors: ["No free ^FN slot left (1-99)"] }); + }); + it("reports a wrongly typed variable id instead of replacing it", () => { const base = ok(createDraft({ widthMm: 60, heightMm: 40, dpmm: 8, objects: [] })); const file = JSON.parse(JSON.stringify(base.designFile)) as { variables?: unknown[] }; @@ -1843,6 +1903,42 @@ describe("a graphic header past the shared bytes-per-row cap", () => { }); }); +describe("a design file handed over as a JSON string", () => { + const text = JSON.stringify(demoLabel); + + it("reads the same as the object on every envelope tool", () => { + expect(validateDraft(text)).toEqual(validateDraft(demoLabel)); + expect(exportZpl(text)).toEqual(exportZpl(demoLabel)); + expect(openInApp(text)).toEqual(openInApp(demoLabel)); + const move = [{ op: "update", id: "preis", y: 70 }] as const; + expect(patchDesign(text, move)).toEqual(patchDesign(demoLabel, move)); + expect(ok(exportZpl(text)).zpl).toContain("Äpfel"); + expect(ok(exportZpl(text)).zpl).toContain("4,99 €"); + }); + + it("names a design encoded as a string twice", () => { + const result = validateDraft(JSON.stringify(text)); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.errors).toEqual(["designFile: the JSON decodes to another string, send the design once"]); + }); + + it("names broken JSON instead of a schema failure", () => { + const result = validateDraft(text.slice(0, -1)); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.errors).toHaveLength(1); + expect(result.errors[0]).toMatch(/^designFile: not valid JSON, /); + }); + + it("refuses any other shape at the input schema with both accepted forms named", () => { + const parsed = z.object(patchDesignShape).safeParse({ designFile: 5, operations: [{ op: "remove", id: "x" }] }); + expect(parsed.success).toBe(false); + if (parsed.success) return; + expect(parsed.error.issues.map((i) => i.message)).toEqual(["designFile must be the design object or its JSON as a string"]); + }); +}); + describe("the patch operations array", () => { it("is capped by the input schema", () => { const schema = z.object(patchDesignShape);