From 6c11b61d4e6c41f3beb320b06b2ffcbad98eebaf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 04:17:28 +0000 Subject: [PATCH] fix(cli): formatZodErrors expands invalid_union, the branch prescription reaches the terminal (#5341) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zod folds every branch of a failed union into ONE top-level issue whose own message is the literal "Invalid input"; each branch's real rejection sits in `issue.errors[]`. The CLI's `formatZodErrors` walked only the top level, so `os validate`, `os build` (compile) and `os plugin build` — all three print through that one function — showed `invalid_union: Invalid input` and dropped the branch that says WHICH key is wrong. Third consumer of the same defect after `formatZodError` (#4971, PR #5342) and `zodIssuesToFields` (#5014, PR #5362). The branch-selection policy is reused rather than re-derived: because the terminal needs exactly the string spec already exports, this one is a plain `formatZodIssue` import instead of a third copy of the ranking. Strictly additive: the union's own lines still print, non-union issues render unchanged, the footer still counts `error.issues`, and the `--json` path is untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkPSGsX9o17MsGv3Lbxu2w --- .changeset/cli-format-zod-union-branches.md | 49 +++++ packages/cli/src/utils/format.ts | 54 +++++ packages/cli/test/format-zod-union.test.ts | 226 ++++++++++++++++++++ 3 files changed, 329 insertions(+) create mode 100644 .changeset/cli-format-zod-union-branches.md create mode 100644 packages/cli/test/format-zod-union.test.ts diff --git a/.changeset/cli-format-zod-union-branches.md b/.changeset/cli-format-zod-union-branches.md new file mode 100644 index 0000000000..c9a14d4062 --- /dev/null +++ b/.changeset/cli-format-zod-union-branches.md @@ -0,0 +1,49 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os validate` / `os build` print the union branch's prescription, not a bare `invalid_union: Invalid input` (#5341) + +Zod folds every branch of a failed `z.union` into ONE top-level issue whose own +`message` is the literal `"Invalid input"`; each branch's real rejection — +required-property and unknown-key prescriptions alike — sits in `issue.errors[]` +with paths relative to the union's own. The CLI's `formatZodErrors` +(`packages/cli/src/utils/format.ts`) walked only the top level, so an author who +mistyped a key inside a union member read: + +``` + views: + ✗ views.0.list.sort + invalid_union: Invalid input +``` + +…while the branch that names the key, and the fix, was produced on every run and +delivered on none. Three commands print through that one function — `os +validate`, `os build` (compile) and `os plugin build` — so the terminal was the +one surface where the #4001 campaign's curated prose never arrived. It now +reads: + +``` + views: + ✗ views.0.list.sort + invalid_union: Invalid input + ✗ views.0.list.sort.0.order: Invalid option: expected one of "asc"|"desc" + ✗ views.0.list.sort.0: Unrecognized key(s) on this sort entry: `direction`. … Did you mean `direction` → `order`? +``` + +This is the same defect's **third** consumer, and it reuses the branch-selection +policy the first two landed rather than re-deriving it: drop branches that only +say "wrong kind of value", prefer the branch complaining least so one stray key +is not reported once per branch (the #4001 批 6c regression), break ties on +`unrecognized_keys`, absolute paths, bounded expansion depth. `formatZodError` +(spec, #4971) and `zodIssuesToFields` (the REST wire, #5014) already carry it; +because the terminal needs exactly the string spec already exports, this one is +a plain `formatZodIssue` import instead of a third copy — so one mistake cannot +get three different prescriptions depending on which surface the author hit. + +Strictly additive: the union's own `✗ path` / `invalid_union: Invalid input` +lines still print, non-union issues render byte-for-byte as before, and the +`N validation error(s) total` footer still counts `error.issues` — one union is +one issue however many lines explain it, which keeps the footer agreeing with +the `--json` payload beside it. The `--json` path is untouched; it passes +`error.issues` through and always carried the whole tree. diff --git a/packages/cli/src/utils/format.ts b/packages/cli/src/utils/format.ts index 91df3cdc72..4b40331675 100644 --- a/packages/cli/src/utils/format.ts +++ b/packages/cli/src/utils/format.ts @@ -2,6 +2,7 @@ import chalk from 'chalk'; import type { ZodError } from 'zod'; +import { formatZodIssue } from '@objectstack/spec'; import type { TenancyPosture } from '@objectstack/spec/security'; // ─── Constants ────────────────────────────────────────────────────── @@ -164,6 +165,54 @@ export function createTimer() { // ─── Zod Error Formatting ─────────────────────────────────────────── +/** + * How far the branch lines of an expanded union are pushed to sit UNDER the + * `code: message` line this file prints for the union itself. + * + * `formatZodIssue` indents its own depth-0 line by 2 spaces and each nested + * level by 2 more; this file's per-issue block is at 4/6. Adding 4 puts the + * first branch level at 8 — one step below the `invalid_union: Invalid input` + * line it explains — and keeps every deeper level nested relative to it. + */ +const UNION_BRANCH_REINDENT = ' '; + +/** + * The lines that explain an `invalid_union`, or nothing at all. + * + * Zod folds every branch of a failed union into ONE issue whose own `message` + * is the literal `"Invalid input"`; each branch's real rejection sits in + * `issue.errors[]`, with paths relative to the union's own. A consumer that + * walks only the top level therefore prints `invalid_union: Invalid input` and + * drops the branch that says WHICH key is wrong — which is what `os validate`, + * `os build` and `os plugin build` did until #5341, so every curated + * prescription the #4001 campaign wrote for a strict shape behind a union was + * produced and never delivered to the author's terminal. + * + * The branch SELECTION (drop branches that only say "wrong kind of value", + * prefer the branch complaining least so one stray key is not reported once per + * branch, break ties on `unrecognized_keys`, absolute paths, bounded depth) is + * `@objectstack/spec`'s, reused rather than re-derived: this is the third + * consumer of the same defect after `formatZodError` (#4971, PR #5342) and the + * REST wire's `zodIssuesToFields` (#5014, PR #5362), and one mistake must not + * get three different prescriptions depending on which surface the author hit. + * Unlike the wire — which needs structured `{field, code, message}` entries and + * so had to re-implement the ranking — the terminal needs exactly the STRING + * that spec already exports, so here the reuse is a plain import. + * + * Line 0 of that render is the union's own verdict, which the caller has + * already printed in this file's own idiom; only the explanation is returned, + * so the change is strictly ADDITIVE — nothing that printed before #5341 stops + * printing. A non-union issue renders as a single line, hence never reaches + * here and could not add one anyway. + */ +function unionBranchLines(issue: unknown): string[] { + if ((issue as { code?: unknown } | null)?.code !== 'invalid_union') return []; + return formatZodIssue(issue as Parameters[0]) + .split('\n') + .slice(1) + .map((line) => `${UNION_BRANCH_REINDENT}${line}`); +} + export function formatZodErrors(error: ZodError) { const issues = error.issues || (error as any).errors || []; @@ -199,6 +248,11 @@ export function formatZodErrors(error: ZodError) { if ((issue as any).received) { console.log(chalk.dim(` received: ${chalk.red((issue as any).received)}`)); } + + // [#5341] …and, for a union, the branch that actually explains it. + for (const line of unionBranchLines(issue)) { + console.log(chalk.dim(line)); + } } } diff --git a/packages/cli/test/format-zod-union.test.ts b/packages/cli/test/format-zod-union.test.ts new file mode 100644 index 0000000000..9b996eb1df --- /dev/null +++ b/packages/cli/test/format-zod-union.test.ts @@ -0,0 +1,226 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Does a rejection behind a `z.union` reach the terminal? (#5341) + * + * Zod folds every branch of a failed union into ONE top-level issue whose own + * `message` is the literal `"Invalid input"`; each branch's real rejection — + * required-property and unknown-key prescriptions alike — sits in + * `issue.errors[]` with paths RELATIVE to the union's own. A consumer that + * walks only the top level prints `invalid_union: Invalid input` and drops + * every curated word the #4001 campaign wrote for the strict shapes behind + * that union. + * + * This is the same defect in its THIRD consumer, each a separate piece of code: + * + * 1. `formatZodError` (`spec/src/shared/error-map.zod.ts`) — #4971, PR #5342; + * 2. `zodIssuesToFields` (`rest/src/rest-server.ts`, the wire) — #5014, PR #5362; + * 3. `formatZodErrors` (`cli/src/utils/format.ts`, the terminal) — THIS file. + * + * (3) is what `os validate`, `os build` (compile) and `os plugin build` print + * through — three commands, one function — so until #5341 an author publishing + * from the terminal was the one reader the campaign's prose never reached, + * while the `--json` payload beside it carried the whole tree. + * + * The whole risk of fixing it is the opposite failure: N branches × the same + * mistake reported N times, which is what made `view.zod.ts`'s `submitBehavior` + * reach for `discriminatedUnion` (#4001 批 6c). Both directions are pinned. + */ + +import { describe, expect, it } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { z } from 'zod'; +import { ObjectStackDefinitionSchema } from '@objectstack/spec'; +import { formatZodErrors } from '../src/utils/format'; + +const cliBin = join(fileURLToPath(new URL('.', import.meta.url)), '..', 'bin', 'run-dev.js'); + +/** Drop SGR sequences so an assertion reads the words, not chalk's opinion. */ +const stripAnsi = (s: string) => s.replace(/\u001B\[[0-9;]*m/g, ''); + +/** Run `formatZodErrors` and return everything it printed, as one string. */ +function render(error: z.ZodError): string { + const captured: string[] = []; + const original = console.log; + console.log = (...args: unknown[]) => { + captured.push(args.map(String).join(' ')); + }; + try { + formatZodErrors(error as never); + } finally { + console.log = original; + } + return stripAnsi(captured.join('\n')); +} + +/** The campaign's shape: a string form OR a strict object form. */ +const ACTION_REF = z.union([ + z.string(), + z.strictObject({ type: z.string(), params: z.record(z.string(), z.unknown()).optional() }), +]); + +describe('[#5341] formatZodErrors expands invalid_union branches', () => { + it('prints the failing branch prose under the union line', () => { + const out = render(ACTION_REF.safeParse({ type: 'log', args: { a: 1 } }).error!); + // The union's own two lines are PRESERVED — they are what says "no branch + // matched", and keeping them makes this change strictly additive: nothing + // that printed before #5341 stopped printing. + expect(out).toContain('invalid_union: Invalid input'); + // …and the branch's prescription now arrives with them. + expect(out).toContain('Unrecognized key: "args"'); + }); + + it('drops the kind-mismatch branch that carries no prescription', () => { + const out = render(ACTION_REF.safeParse({ type: 'log', args: 1 }).error!); + // Paired deliberately: the `not` alone would also pass if the expansion + // produced NOTHING — a green for the empty reason. The positive assertion + // is what makes the negative one mean "selected against", not "absent". + expect(out).toContain('args'); + // `expected string, received object` is the string branch complaining that + // the author did not write a string. They never meant to. + expect(out).not.toContain('expected string'); + }); + + it('resolves branch paths against the union, not relative to it', () => { + const schema = z.object({ actions: z.array(ACTION_REF) }); + const out = render(schema.safeParse({ actions: [{ type: 'log', args: { a: 1 } }] }).error!); + expect(out).toContain('✗ actions.0: Unrecognized key: "args"'); + // Never the bare relative path a naive splice would print. + expect(out).not.toContain('✗ (root): Unrecognized key'); + }); + + it('expands a union nested inside a union', () => { + const schema = z.object({ on: z.union([z.string(), z.object({ actions: z.array(ACTION_REF) })]) }); + const out = render(schema.safeParse({ on: { actions: [{ type: 'log', args: { a: 1 } }] } }).error!); + expect(out).toContain('✗ on.actions.0: Invalid input'); + expect(out).toContain('✗ on.actions.0: Unrecognized key: "args"'); + }); + + // ⚠️ THE anti-regression, mirroring the pin #4971 left in + // `spec/src/shared/error-map.test.ts`. #4001 批 6c measured a plain `z.union` + // of four strict members reporting one bad key once per member. Selecting the + // branch that complains LEAST is what keeps the expansion from reintroducing + // it: the member the author was aiming at reports only the stray key, while + // the others also report a wrong discriminator and their own missing requireds. + it('reports one unknown key ONCE, not once per branch', () => { + const union = z.union([ + z.strictObject({ kind: z.literal('a'), x: z.string() }), + z.strictObject({ kind: z.literal('b'), y: z.string() }), + z.strictObject({ kind: z.literal('c'), z: z.string() }), + ]); + const out = render(union.safeParse({ kind: 'a', x: 'ok', bogus: 1 }).error!); + + expect(out.match(/bogus/g)?.length).toBe(1); + // The two shapes the author was not writing stay out of the terminal. + expect(out).not.toContain('expected "b"'); + expect(out).not.toContain('expected "c"'); + }); + + it('leaves a non-union issue rendered exactly as before', () => { + const out = render(z.object({ name: z.string() }).safeParse({ name: 1 }).error!); + expect(out).toContain('✗ name'); + expect(out).toContain('invalid_type:'); + expect(out).toContain('expected: string'); + // The footer counts `error.issues`, unchanged: a union is ONE issue no + // matter how many lines explain it, which is what keeps this number + // agreeing with the `--json` payload beside it. + expect(out).toContain('1 validation error(s) total'); + }); + + it('counts a union as one issue however many lines explain it', () => { + const out = render(ACTION_REF.safeParse({ type: 'log', args: { a: 1 } }).error!); + expect(out).toContain('1 validation error(s) total'); + }); +}); + +/** + * The live specimen, on the surface `os validate` actually parses. + * + * `views[].list.sort` is `z.union([z.string(), z.array()])` + * and the entry declares the #4721 alias `direction → order` — the same tuple + * under a different word, which is worth a prescription precisely because + * getting it wrong REVERSES the sort silently. Behind a union, that + * prescription was produced on every run and delivered on none. + */ +const SORT_ALIAS_STACK = { + manifest: { id: 'union_probe', name: 'Union Probe', namespace: 'union_probe', version: '1.0.0', type: 'app' }, + views: [ + { + name: 'union_probe_view', + object: 'union_probe_obj', + list: { + name: 'union_probe_list', + label: 'Union Probe', + type: 'grid', + columns: ['name'], + sort: [{ field: 'name', direction: 'desc' }], + }, + }, + ], +}; + +/** Run a CLI command in a temp dir holding `stack` as the config. */ +function runCli(command: string, stack: Record, args: string[] = []): { exitCode: number; output: string } { + const dir = mkdtempSync(join(tmpdir(), 'os-union-format-')); + try { + // A plain literal, not `defineStack`/`defineView`: those factories parse + // eagerly and would throw through spec's OWN formatter, which has expanded + // unions since #4971 — the one thing this file must not accidentally + // measure instead of the CLI's renderer. + writeFileSync(join(dir, 'objectstack.config.mjs'), `export default ${JSON.stringify(stack, null, 2)};\n`); + try { + const output = execFileSync(process.execPath, [cliBin, command, ...args], { + cwd: dir, + encoding: 'utf8', + stdio: 'pipe', + }); + return { exitCode: 0, output: stripAnsi(output) }; + } catch (error: any) { + return { exitCode: error.status ?? 1, output: stripAnsi(`${error.stdout ?? ''}${error.stderr ?? ''}`) }; + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +describe('[#5341] `os validate` delivers a union branch prescription', () => { + // Reverse verification, direction declared up front: the failure this + // reports must be the union and nothing else, so the schema-level control + // runs first. If the stack failed for some unrelated reason the terminal + // assertion below could pass on the wrong error entirely. + it('the specimen fails on exactly one issue, and that issue is the union', () => { + const result = ObjectStackDefinitionSchema.safeParse(SORT_ALIAS_STACK); + expect(result.success).toBe(false); + const issues = result.success ? [] : result.error.issues; + expect(issues).toHaveLength(1); + expect(issues[0]!.code).toBe('invalid_union'); + // The prescription exists in the payload — it always has. Delivery is the + // only thing #5341 is about. + expect(JSON.stringify(issues[0])).toContain('`direction` → `order`'); + }); + + it('prints the prescription, not a bare `invalid_union: Invalid input`', () => { + const { exitCode, output } = runCli('validate', SORT_ALIAS_STACK); + expect(exitCode, `os validate accepted a stack with an aliased sort key:\n${output}`).not.toBe(0); + expect(output).toContain('views.0.list.sort'); + expect(output).toContain('`direction` → `order`'); + }, 120_000); + + it('leaves the `--json` payload exactly as it was — full, and nested', () => { + // The machine path never had this defect: it passes `error.issues` through, + // so the branch tree was always on it. Pinned here because the fix is one + // `console.log` loop away from being "helpfully" moved into the payload. + const { exitCode, output } = runCli('validate', SORT_ALIAS_STACK, ['--json']); + expect(exitCode).not.toBe(0); + const payload = JSON.parse(output.slice(output.indexOf('{'))); + expect(payload.valid).toBe(false); + expect(payload.errors).toHaveLength(1); + expect(payload.errors[0].code).toBe('invalid_union'); + // The branch tree, untouched — and NOT flattened into extra `errors[]` rows. + expect(JSON.stringify(payload.errors[0].errors)).toContain('`direction` → `order`'); + }, 120_000); +});