From 6e70ea687a2cdc2b2b98fc2ebd1837bbd8505942 Mon Sep 17 00:00:00 2001 From: Chris Strahl <3299153+chrisstrahl@users.noreply.github.com> Date: Wed, 22 Jul 2026 23:34:32 -0700 Subject: [PATCH] fix: surface validation issues in update and delete error output When collection.update() rejects a write, the library returns both result.error and result.issues (an array of {code, message, field, severity}), but the update command printed only error.message ("Validation failed on update") and discarded the issues in both text and --format json output. That leaves the user with no way to see *which* field failed or why. The create command already surfaced issues; update and delete did not. - Move formatIssue from create.ts into utils.ts and share it. - update: include `issues` in JSON error output and print them as indented lines in text output, matching create's behavior. - delete: same defensive passthrough (no library code path returns issues on delete today, but the output contract now matches). - Add a strict-validation test fixture (default_validation: error), since the default "warn" level never rejects updates. - Add tests for update issue output, plus regression tests for create's existing issue output. Co-Authored-By: Claude Fable 5 --- .gitignore | 1 + src/commands/create.ts | 15 +--------- src/commands/delete.ts | 15 ++++++++-- src/commands/update.ts | 15 ++++++++-- src/utils.ts | 16 ++++++++++ test/create.test.ts | 29 +++++++++++++++++++ .../fixtures/strict-collection/_types/note.md | 11 +++++++ test/fixtures/strict-collection/hello.md | 7 +++++ test/fixtures/strict-collection/mdbase.yaml | 4 +++ test/update.test.ts | 28 ++++++++++++++++++ 10 files changed, 123 insertions(+), 18 deletions(-) create mode 100644 test/fixtures/strict-collection/_types/note.md create mode 100644 test/fixtures/strict-collection/hello.md create mode 100644 test/fixtures/strict-collection/mdbase.yaml diff --git a/.gitignore b/.gitignore index 6b8cace..c4a4429 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ obsidian-help/ # Test artifacts .mdbase-test-*/ +test/fixtures/strict-collection/.mdbase/ diff --git a/src/commands/create.ts b/src/commands/create.ts index 1caf5bd..9dbe895 100644 --- a/src/commands/create.ts +++ b/src/commands/create.ts @@ -4,7 +4,7 @@ import chalk from "chalk"; import { Collection } from "@callumalpass/mdbase"; import type { MdbaseError } from "@callumalpass/mdbase"; import yaml from "js-yaml"; -import { finishCommand, parseFieldValue } from "../utils.js"; +import { finishCommand, formatIssue, parseFieldValue } from "../utils.js"; function parseFields(fieldArgs: string[]): Record | null { const frontmatter: Record = {}; @@ -95,19 +95,6 @@ export function registerCreate(program: Command): void { }); } -function formatIssue(issue: MdbaseError): string { - const severity = issue.severity ?? "error"; - const tag = - severity === "error" - ? chalk.red("error") - : severity === "warning" - ? chalk.yellow("warn") - : chalk.blue("info"); - - const field = issue.field ? ` field ${chalk.bold(issue.field)}` : ""; - return ` ${tag}${field}: ${issue.message} ${chalk.dim(`[${issue.code}]`)}`; -} - function outputResult( result: { valid?: boolean; frontmatter?: Record; body?: string; path?: string; error?: { code: string; message: string }; issues?: MdbaseError[] }, requestedPath: string | undefined, diff --git a/src/commands/delete.ts b/src/commands/delete.ts index 073a397..9a3c9a5 100644 --- a/src/commands/delete.ts +++ b/src/commands/delete.ts @@ -2,8 +2,9 @@ import { Command } from "commander"; import path from "node:path"; import chalk from "chalk"; import { Collection } from "@callumalpass/mdbase"; +import type { MdbaseError } from "@callumalpass/mdbase"; import yaml from "js-yaml"; -import { finishCommand } from "../utils.js"; +import { finishCommand, formatIssue } from "../utils.js"; export function registerDelete(program: Command): void { program @@ -38,10 +39,20 @@ export function registerDelete(program: Command): void { : result.error.code === "permission_denied" ? 5 : 1; + const issues = (result as { issues?: MdbaseError[] }).issues; if (opts.format === "json") { - console.log(JSON.stringify({ error: result.error }, null, 2)); + const output: Record = { error: result.error }; + if (issues) { + output.issues = issues; + } + console.log(JSON.stringify(output, null, 2)); } else { console.error(chalk.red(`error: ${result.error.message}`)); + if (issues) { + for (const issue of issues) { + console.error(formatIssue(issue)); + } + } } await finishCommand(collection, exitCode); return; diff --git a/src/commands/update.ts b/src/commands/update.ts index 64ed9ee..adcc9fa 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -2,8 +2,9 @@ import { Command } from "commander"; import path from "node:path"; import chalk from "chalk"; import { Collection } from "@callumalpass/mdbase"; +import type { MdbaseError } from "@callumalpass/mdbase"; import yaml from "js-yaml"; -import { finishCommand, parseFieldValue } from "../utils.js"; +import { finishCommand, formatIssue, parseFieldValue } from "../utils.js"; function parseFields(fieldArgs: string[]): Record | null { const fields: Record = {}; @@ -97,10 +98,20 @@ export function registerUpdate(program: Command): void { : result.error.code === "permission_denied" ? 5 : 1; + const issues = (result as { issues?: MdbaseError[] }).issues; if (opts.format === "json") { - console.log(JSON.stringify({ error: result.error }, null, 2)); + const output: Record = { error: result.error }; + if (issues) { + output.issues = issues; + } + console.log(JSON.stringify(output, null, 2)); } else { console.error(chalk.red(`error: ${result.error.message}`)); + if (issues) { + for (const issue of issues) { + console.error(formatIssue(issue)); + } + } } await finishCommand(collection, exitCode); return; diff --git a/src/utils.ts b/src/utils.ts index e4c2119..e79666f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,7 +1,23 @@ +import chalk from "chalk"; +import type { MdbaseError } from "@callumalpass/mdbase"; + type ClosableCollection = { close(): Promise; }; +export function formatIssue(issue: MdbaseError): string { + const severity = issue.severity ?? "error"; + const tag = + severity === "error" + ? chalk.red("error") + : severity === "warning" + ? chalk.yellow("warn") + : chalk.blue("info"); + + const field = issue.field ? ` field ${chalk.bold(issue.field)}` : ""; + return ` ${tag}${field}: ${issue.message} ${chalk.dim(`[${issue.code}]`)}`; +} + export async function finishCommand( collection: ClosableCollection | null | undefined, code: number, diff --git a/test/create.test.ts b/test/create.test.ts index 6fe4313..7d8b9ba 100644 --- a/test/create.test.ts +++ b/test/create.test.ts @@ -142,6 +142,35 @@ describe("create command", () => { }); }); + describe("validation issues", () => { + it("includes issues in JSON output when validation fails", () => { + const file = trackFile("create-invalid.md"); + const { stdout, exitCode } = run( + ["create", file, "-t", "note", "-f", "rating=10", "--format", "json"], + VALID, + ); + expect(exitCode).toBe(2); + const parsed = JSON.parse(stdout); + expect(parsed.error.code).toBe("validation_failed"); + expect(parsed.issues).toBeInstanceOf(Array); + const codes = parsed.issues.map((i: { code: string }) => i.code); + expect(codes).toContain("missing_required"); + expect(codes).toContain("number_too_large"); + }); + + it("prints issues in text output when validation fails", () => { + const file = trackFile("create-invalid.md"); + const { exitCode, stderr } = run( + ["create", file, "-t", "note", "-f", "rating=10"], + VALID, + ); + expect(exitCode).toBe(2); + expect(stderr).toContain("Validation failed on create"); + expect(stderr).toContain("missing_required"); + expect(stderr).toContain("number_too_large"); + }); + }); + describe("error handling", () => { it("exits 1 for path conflict", () => { const { exitCode, stderr } = run( diff --git a/test/fixtures/strict-collection/_types/note.md b/test/fixtures/strict-collection/_types/note.md new file mode 100644 index 0000000..9f48861 --- /dev/null +++ b/test/fixtures/strict-collection/_types/note.md @@ -0,0 +1,11 @@ +--- +name: note +fields: + title: + type: string + required: true + rating: + type: integer + min: 1 + max: 5 +--- diff --git a/test/fixtures/strict-collection/hello.md b/test/fixtures/strict-collection/hello.md new file mode 100644 index 0000000..24ffc98 --- /dev/null +++ b/test/fixtures/strict-collection/hello.md @@ -0,0 +1,7 @@ +--- +type: note +title: Hello World +rating: 5 +--- + +This is a valid note. diff --git a/test/fixtures/strict-collection/mdbase.yaml b/test/fixtures/strict-collection/mdbase.yaml new file mode 100644 index 0000000..fddcbad --- /dev/null +++ b/test/fixtures/strict-collection/mdbase.yaml @@ -0,0 +1,4 @@ +spec_version: "0.2.0" +name: strict-collection +settings: + default_validation: error diff --git a/test/update.test.ts b/test/update.test.ts index 50a1108..59f6fdb 100644 --- a/test/update.test.ts +++ b/test/update.test.ts @@ -6,6 +6,7 @@ import path from "node:path"; const CLI = path.resolve(__dirname, "../src/cli.ts"); const TSX_CLI = path.resolve(__dirname, "../node_modules/tsx/dist/cli.mjs"); const VALID = path.resolve(__dirname, "fixtures/valid-collection"); +const STRICT = path.resolve(__dirname, "fixtures/strict-collection"); function run(args: string[], cwd: string): { stdout: string; stderr: string; exitCode: number } { try { @@ -196,6 +197,33 @@ describe("update command", () => { }); }); + describe("validation issues", () => { + it("includes issues in JSON output when validation fails", () => { + const { stdout, exitCode } = run( + ["update", "hello.md", "-f", "rating=10", "--format", "json"], + STRICT, + ); + expect(exitCode).toBe(2); + const parsed = JSON.parse(stdout); + expect(parsed.error.code).toBe("validation_failed"); + expect(parsed.issues).toBeInstanceOf(Array); + expect(parsed.issues.length).toBeGreaterThan(0); + expect(parsed.issues[0].field).toBe("rating"); + expect(parsed.issues[0].code).toBe("number_too_large"); + }); + + it("prints issues in text output when validation fails", () => { + const { exitCode, stderr } = run( + ["update", "hello.md", "-f", "rating=10"], + STRICT, + ); + expect(exitCode).toBe(2); + expect(stderr).toContain("Validation failed on update"); + expect(stderr).toContain("rating"); + expect(stderr).toContain("number_too_large"); + }); + }); + describe("file persistence", () => { it("actually writes changes to disk", () => { backupFile("hello.md");