diff --git a/.changeset/typed-mermaid-validation-errors.md b/.changeset/typed-mermaid-validation-errors.md new file mode 100644 index 0000000..1b8e9d0 --- /dev/null +++ b/.changeset/typed-mermaid-validation-errors.md @@ -0,0 +1,8 @@ +--- +"sideshow": patch +--- + +Invalid Mermaid submitted through the posts API now returns a typed validation +error with the failing request field, diagram type, complete parser diagnostic, +and concrete retry steps. The existing human-readable `error` string remains +for compatibility, and rejected diagrams are not persisted. diff --git a/e2e/mermaid.spec.ts b/e2e/mermaid.spec.ts index 1dd9a5a..ab5965e 100644 --- a/e2e/mermaid.spec.ts +++ b/e2e/mermaid.spec.ts @@ -50,6 +50,39 @@ test("a mermaid part renders inside an opaque-origin frame served from /s", asyn await expect(frame.locator("body")).toContainText("Choice"); }); +test("the native posts endpoint rejects invalid mermaid with actionable details", async ({ + page, + request, + server, +}) => { + const response = await request.post(`${server.url}/api/posts`, { + data: { + title: "Broken diagram", + surfaces: [{ kind: "mermaid", mermaid: 'pie title Pets\n "Dogs" : broken !!@@' }], + }, + }); + + expect(response.status()).toBe(400); + const body = await response.json(); + expect(body).toMatchObject({ + code: "surface_validation_failed", + issues: [ + { + code: "invalid_mermaid_syntax", + requestPath: "surfaces[0].mermaid", + diagramType: "pie", + }, + ], + }); + expect(body.issues[0].parserMessage).toContain("Parsing failed"); + expect(body.issues[0].nextSteps).toContain( + "Correct the Mermaid syntax described by parserMessage, then retry the request.", + ); + + await page.goto(server.url); + await expect(page.locator(".card:not(#whatsNew)")).toHaveCount(0); +}); + test("an invalid mermaid part shows the source in an error fallback, not a crash", async ({ page, server, diff --git a/server/app.ts b/server/app.ts index c1b0260..530c4ff 100644 --- a/server/app.ts +++ b/server/app.ts @@ -46,7 +46,7 @@ import { type TerminalSurface, type TraceStep, } from "./types.ts"; -import { validateSurfaces } from "./postSurfaces.ts"; +import { type SurfaceValidationFailure, validateSurfaces } from "./postSurfaces.ts"; import { findWelcomePost, WELCOME_POST_TITLE, @@ -77,6 +77,12 @@ const MAX_STEP_LABEL = 500; // edge to keep one oversize value from bloating the agent's context forever. const MAX_COMMENT_TEXT = 8000; const MAX_TITLE = 500; + +const surfaceValidationErrorBody = (failure: SurfaceValidationFailure) => ({ + error: failure.error, + code: failure.code, + issues: failure.issues, +}); // Ceiling on concurrently-held SSE + long-poll connections. Both are GETs that // pin a connection open (the event stream indefinitely, /api/comments?wait up // to MAX_WAIT_SECONDS); on a publicRead workspace they're reachable unauthenticated, @@ -1198,7 +1204,7 @@ export function createApp({ return c.json({ error: 'body must include a "surfaces" (or legacy "parts") array' }, 400); } const parsed = await validateSurfaces(blocks); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); return publish(c, body, parsed.surfaces); }; app.post("/api/posts", publishPost); // canonical @@ -1213,7 +1219,7 @@ export function createApp({ return c.json({ error: 'body must include non-empty "html" string' }, 400); } const parsed = await validateSurfaces([htmlSurface(body.html, body.kits)]); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); return publish(c, body, parsed.surfaces); }); @@ -1278,11 +1284,11 @@ export function createApp({ return c.json({ error: '"surfaces" (or legacy "parts") must be an array' }, 400); } const parsed = await validateSurfaces(blocks); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); surfaces = parsed.surfaces; } else if (typeof body.html === "string") { const parsed = await validateSurfaces([htmlSurface(body.html, body.kits)]); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); surfaces = parsed.surfaces; } const result = await revisePost(c.req.param("id"), { @@ -1346,7 +1352,7 @@ export function createApp({ ); } const parsed = await validateSurfaces([updated]); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); surfaces = [...existing.surfaces]; surfaces[targetIdx] = { ...parsed.surfaces[0], id: existing.surfaces[targetIdx].id }; } @@ -1371,7 +1377,7 @@ export function createApp({ return c.json({ error: 'body must include a "surface" object' }, 400); } const parsed = await validateSurfaces([body.surface]); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); const result = await appendPostSurface(c.req.param("id"), parsed.surfaces[0], { before: body.before, after: body.after, @@ -1394,7 +1400,7 @@ export function createApp({ let surface: Surface | undefined; if (body.surface !== undefined) { const parsed = await validateSurfaces([body.surface]); - if (!parsed.ok) return c.json({ error: parsed.error }, 400); + if (!parsed.ok) return c.json(surfaceValidationErrorBody(parsed), 400); surface = parsed.surfaces[0]; } const result = await replacePostSurface(c.req.param("id"), c.req.param("target"), { diff --git a/server/postSurfaces.ts b/server/postSurfaces.ts index 6bd3d85..3d5e157 100644 --- a/server/postSurfaces.ts +++ b/server/postSurfaces.ts @@ -2,13 +2,49 @@ import { z } from "zod"; import { isKnownKit, KIT_IDS } from "./kits.ts"; import { isSurfaceKind, type Surface, type SurfaceKind } from "./types.ts"; +/** Holds parsed surfaces and any validation issues found at the HTTP/MCP boundary. */ export interface SurfaceParseResult { surfaces: Surface[]; // Deprecated compatibility alias for older deep imports. parts: Surface[]; - errors: string[]; + errors: SurfaceValidationIssue[]; } +/** Identifies a rejected surface field and, for Mermaid syntax, how the agent can recover. */ +export type SurfaceValidationIssue = + | { + code: "invalid_surface"; + requestPath: string; + message: string; + } + | { + code: "invalid_mermaid_syntax"; + requestPath: string; + message: string; + diagramType: string | null; + parserMessage: string; + nextSteps: string[]; + }; + +/** Describes a failed surface validation without losing machine-readable issue details. */ +export interface SurfaceValidationFailure { + ok: false; + error: string; + code: "surface_validation_failed"; + issues: SurfaceValidationIssue[]; +} + +/** Returns either validated surfaces or a typed surface validation failure. */ +export type SurfaceValidationResult = + | { ok: true; surfaces: Surface[]; parts: Surface[] } + | SurfaceValidationFailure; + +const MERMAID_PARSE_NEXT_STEPS = [ + "Correct the Mermaid syntax described by parserMessage, then retry the request.", + 'Keep the diagram declaration on the first non-comment line (for example, "flowchart TD").', + "Reduce the diagram to a minimal valid form, then add statements back until the failing line is isolated.", +]; + const requiredString = (name: string) => z.string({ required_error: `requires string "${name}"`, @@ -261,7 +297,12 @@ async function parseSurfaceList( raw: unknown, opts: { strict?: boolean } = {}, ): Promise { - if (!Array.isArray(raw)) return surfaceResult([], ["surfaces must be an array"]); + if (!Array.isArray(raw)) { + return surfaceResult( + [], + [{ code: "invalid_surface", requestPath: "surfaces", message: "must be an array" }], + ); + } if (opts.strict === true) { const results = await Promise.all(raw.map((surface, i) => parseStrictSurface(surface, i))); @@ -272,28 +313,32 @@ async function parseSurfaceList( } const surfaces: Surface[] = []; - for (const surface of raw) { + for (const [index, surface] of raw.entries()) { const parsed = looseSurfaceSchema.safeParse(surface); if (!parsed.success) continue; - if ((await validateSemantics(parsed.data as Surface)).length === 0) + if ((await validateSemantics(parsed.data as Surface, `surfaces[${index}]`)).length === 0) surfaces.push(parsed.data as Surface); } return surfaceResult(surfaces, []); } -function surfaceResult(surfaces: Surface[], errors: string[]): SurfaceParseResult { +function surfaceResult(surfaces: Surface[], errors: SurfaceValidationIssue[]): SurfaceParseResult { return { surfaces, parts: surfaces, errors }; } export const coerceSurfaces = (raw: unknown): Promise => parseSurfaceList(raw).then((r) => r.surfaces); -export async function validateSurfaces( - raw: unknown, -): Promise<{ ok: true; surfaces: Surface[]; parts: Surface[] } | { ok: false; error: string }> { +/** Strictly validates surfaces before a native API write reaches persistence. */ +export async function validateSurfaces(raw: unknown): Promise { const result = await parseSurfaceList(raw, { strict: true }); return result.errors.length > 0 - ? { ok: false, error: result.errors.join("; ") } + ? { + ok: false, + error: result.errors.map((issue) => `${issue.requestPath}: ${issue.message}`).join("; "), + code: "surface_validation_failed", + issues: result.errors, + } : { ok: true, surfaces: result.surfaces, parts: result.surfaces }; } @@ -345,24 +390,46 @@ function mermaidDiagramType(src: string): string | null { // // This is the other half of app.ts's lazy richRender import: leaving either half // static keeps the whole dependency graph resident and neither one saves anything. -async function validateSemantics(surface: Surface): Promise { +async function validateSemantics( + surface: Surface, + surfaceRequestPath: string, +): Promise { if (surface.kind === "diff" && surface.patch) { const diffParsers = await import("@pierre/diffs"); try { if (!diffPatchHasContent(surface.patch, diffParsers)) return [ - 'diff surface "patch" did not parse to any file — expected a unified/git patch with --- /+++ headers and @@ hunks', + { + code: "invalid_surface", + requestPath: `${surfaceRequestPath}.patch`, + message: + "did not parse to any file — expected a unified/git patch with --- /+++ headers and @@ hunks", + }, ]; } catch (e) { return [ - 'diff surface "patch" failed to parse: ' + (e instanceof Error ? e.message : "error"), + { + code: "invalid_surface", + requestPath: `${surfaceRequestPath}.patch`, + message: "failed to parse: " + (e instanceof Error ? e.message : "error"), + }, ]; } } if (surface.kind === "mermaid") { const diagramType = mermaidDiagramType(surface.mermaid); - if (!diagramType) - return ['mermaid surface has no diagram type (first line should be e.g. "flowchart TD")']; + if (!diagramType) { + return [ + { + code: "invalid_mermaid_syntax", + requestPath: `${surfaceRequestPath}.mermaid`, + message: 'has no diagram type (first line should be e.g. "flowchart TD")', + diagramType: null, + parserMessage: "No Mermaid diagram type was found.", + nextSteps: MERMAID_PARSE_NEXT_STEPS, + }, + ]; + } // Outside the try: the catch below turns a throw into "your mermaid is // invalid", which would be a wrong answer (and a 400 instead of a 500) if what // actually failed was loading the parser. @@ -373,8 +440,18 @@ async function validateSemantics(surface: Surface): Promise { // Unsupported diagram types (flowchart, sequence, etc. — still on Jison) // skip validation; the viewer's graceful fallback handles render failures. if (e instanceof Error && /unknown diagram type/i.test(e.message)) return []; - const msg = e instanceof Error ? (e.message.split("\n")[0] ?? "parse error") : "parse error"; - return [`mermaid surface failed to parse: ${msg}`]; + const parserMessage = e instanceof Error ? e.message : "parse error"; + const summary = parserMessage.split("\n")[0] ?? "parse error"; + return [ + { + code: "invalid_mermaid_syntax", + requestPath: `${surfaceRequestPath}.mermaid`, + message: `failed to parse: ${summary}`, + diagramType, + parserMessage, + nextSteps: MERMAID_PARSE_NEXT_STEPS, + }, + ]; } } return []; @@ -383,20 +460,38 @@ async function validateSemantics(surface: Surface): Promise { async function parseStrictSurface( raw: unknown, index: number, -): Promise<{ surface: Surface | null; errors: string[] }> { - const path = `surfaces[${index}]`; +): Promise<{ surface: Surface | null; errors: SurfaceValidationIssue[] }> { + const surfaceRequestPath = `surfaces[${index}]`; if (!raw || typeof raw !== "object") - return { surface: null, errors: [`${path}: must be an object`] }; + return { + surface: null, + errors: [ + { code: "invalid_surface", requestPath: surfaceRequestPath, message: "must be an object" }, + ], + }; const kind = (raw as { kind?: unknown }).kind; const schema = schemaForKind(kind); - if (!schema) return { surface: null, errors: [`${path}: unknown surface kind`] }; + if (!schema) { + return { + surface: null, + errors: [ + { + code: "invalid_surface", + requestPath: surfaceRequestPath, + message: "unknown surface kind", + }, + ], + }; + } const parsed = schema.safeParse(raw); - if (!parsed.success) return { surface: null, errors: formatZodErrors(parsed.error, path) }; - const semantic = await validateSemantics(parsed.data); + if (!parsed.success) { + return { surface: null, errors: formatZodErrors(parsed.error, surfaceRequestPath) }; + } + const semantic = await validateSemantics(parsed.data, surfaceRequestPath); return semantic.length > 0 - ? { surface: null, errors: semantic.map((m) => `${path}: ${m}`) } + ? { surface: null, errors: semantic } : { surface: parsed.data, errors: [] }; } @@ -404,9 +499,16 @@ function schemaForKind(kind: unknown): z.ZodType | n return isSurfaceKind(kind) ? strictSurfaceSchemas[kind] : null; } -function formatZodErrors(error: z.ZodError, prefix = "surfaces"): string[] { +function formatZodErrors( + error: z.ZodError, + requestPathPrefix = "surfaces", +): SurfaceValidationIssue[] { return error.issues.map((issue) => { const suffix = issue.path.length > 0 ? `.${issue.path.join(".")}` : ""; - return `${prefix}${suffix}: ${issue.message}`; + return { + code: "invalid_surface", + requestPath: `${requestPathPrefix}${suffix}`, + message: issue.message, + }; }); } diff --git a/server/public.ts b/server/public.ts index 7fd2084..c149e3b 100644 --- a/server/public.ts +++ b/server/public.ts @@ -5,4 +5,9 @@ export { createApp, type AppOptions, type AuthenticateHook, type FeedEvent } fro export { SqlStore } from "./sqlStore.js"; export { createSqliteStorage, migrateJsonToSqlite } from "./sqliteStorage.js"; export { JsonFileStore } from "./storage.js"; +export type { + SurfaceValidationFailure, + SurfaceValidationIssue, + SurfaceValidationResult, +} from "./postSurfaces.js"; export type * from "./types.js"; diff --git a/test/api.test.ts b/test/api.test.ts index f9bb560..1cefc4d 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -590,6 +590,29 @@ test("REST surface routes reject malformed surfaces before storage", async () => assert.equal(unchanged.surfaces[0].html, "

x

"); }); +test("POST /api/posts returns a typed actionable error for invalid mermaid", async () => { + const app = makeApp(); + const response = await app.request( + "/api/posts", + json({ + title: "Broken diagram", + surfaces: [{ kind: "mermaid", mermaid: 'pie title Pets\n "Dogs" : broken !!@@' }], + }), + ); + + assert.equal(response.status, 400); + const body = (await response.json()) as any; + assert.equal(body.code, "surface_validation_failed"); + assert.match(body.error, /surfaces\[0\]\.mermaid: failed to parse/); + assert.equal(body.issues.length, 1); + assert.equal(body.issues[0].code, "invalid_mermaid_syntax"); + assert.equal(body.issues[0].requestPath, "surfaces[0].mermaid"); + assert.equal(body.issues[0].diagramType, "pie"); + assert.match(body.issues[0].parserMessage, /Parsing failed/); + assert.match(body.issues[0].nextSteps[0], /retry the request/); + assert.deepEqual(await (await app.request("/api/sessions")).json(), []); +}); + test("publish_surface MCP tool round-trips a diff surface", async () => { const app = makeApp(); const list = (await (await app.request("/mcp", mcpCall(1, "tools/list"))).json()) as any; diff --git a/test/assets.test.ts b/test/assets.test.ts index 078916d..359a566 100644 --- a/test/assets.test.ts +++ b/test/assets.test.ts @@ -205,7 +205,22 @@ test("validateSurfaces rejects invalid mermaid with a parse error (supported typ false, `mermaid ${JSON.stringify(mermaid).slice(0, 40)} should be rejected`, ); - if (!result.ok) assert.match(result.error, /mermaid surface failed to parse/); + if (!result.ok) { + assert.match(result.error, /failed to parse/); + assert.equal(result.code, "surface_validation_failed"); + const issue = result.issues[0]; + assert.ok(issue); + assert.equal(issue.code, "invalid_mermaid_syntax"); + if (issue.code !== "invalid_mermaid_syntax") continue; + assert.equal(issue.requestPath, "surfaces[0].mermaid"); + assert.equal(issue.diagramType, mermaid.split(/\s+/)[0]); + assert.match(issue.parserMessage, /Parsing failed/); + assert.deepEqual(issue.nextSteps, [ + "Correct the Mermaid syntax described by parserMessage, then retry the request.", + 'Keep the diagram declaration on the first non-comment line (for example, "flowchart TD").', + "Reduce the diagram to a minimal valid form, then add statements back until the failing line is isolated.", + ]); + } } });