Skip to content

Commit 3d94141

Browse files
baozhoutaoclaude
andauthored
fix(cli): formatZodErrors expands invalid_union, the branch prescription reaches the terminal (#5341) (#5391)
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. Claude-Session: https://claude.ai/code/session_01VkPSGsX9o17MsGv3Lbxu2w Co-authored-by: Claude <noreply@anthropic.com>
1 parent cfc293f commit 3d94141

3 files changed

Lines changed: 329 additions & 0 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): `os validate` / `os build` print the union branch's prescription, not a bare `invalid_union: Invalid input` (#5341)
6+
7+
Zod folds every branch of a failed `z.union` into ONE top-level issue whose own
8+
`message` is the literal `"Invalid input"`; each branch's real rejection —
9+
required-property and unknown-key prescriptions alike — sits in `issue.errors[]`
10+
with paths relative to the union's own. The CLI's `formatZodErrors`
11+
(`packages/cli/src/utils/format.ts`) walked only the top level, so an author who
12+
mistyped a key inside a union member read:
13+
14+
```
15+
views:
16+
✗ views.0.list.sort
17+
invalid_union: Invalid input
18+
```
19+
20+
…while the branch that names the key, and the fix, was produced on every run and
21+
delivered on none. Three commands print through that one function — `os
22+
validate`, `os build` (compile) and `os plugin build` — so the terminal was the
23+
one surface where the #4001 campaign's curated prose never arrived. It now
24+
reads:
25+
26+
```
27+
views:
28+
✗ views.0.list.sort
29+
invalid_union: Invalid input
30+
✗ views.0.list.sort.0.order: Invalid option: expected one of "asc"|"desc"
31+
✗ views.0.list.sort.0: Unrecognized key(s) on this sort entry: `direction`. … Did you mean `direction` → `order`?
32+
```
33+
34+
This is the same defect's **third** consumer, and it reuses the branch-selection
35+
policy the first two landed rather than re-deriving it: drop branches that only
36+
say "wrong kind of value", prefer the branch complaining least so one stray key
37+
is not reported once per branch (the #4001 批 6c regression), break ties on
38+
`unrecognized_keys`, absolute paths, bounded expansion depth. `formatZodError`
39+
(spec, #4971) and `zodIssuesToFields` (the REST wire, #5014) already carry it;
40+
because the terminal needs exactly the string spec already exports, this one is
41+
a plain `formatZodIssue` import instead of a third copy — so one mistake cannot
42+
get three different prescriptions depending on which surface the author hit.
43+
44+
Strictly additive: the union's own `✗ path` / `invalid_union: Invalid input`
45+
lines still print, non-union issues render byte-for-byte as before, and the
46+
`N validation error(s) total` footer still counts `error.issues` — one union is
47+
one issue however many lines explain it, which keeps the footer agreeing with
48+
the `--json` payload beside it. The `--json` path is untouched; it passes
49+
`error.issues` through and always carried the whole tree.

packages/cli/src/utils/format.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import chalk from 'chalk';
44
import type { ZodError } from 'zod';
5+
import { formatZodIssue } from '@objectstack/spec';
56
import type { TenancyPosture } from '@objectstack/spec/security';
67

78
// ─── Constants ──────────────────────────────────────────────────────
@@ -164,6 +165,54 @@ export function createTimer() {
164165

165166
// ─── Zod Error Formatting ───────────────────────────────────────────
166167

168+
/**
169+
* How far the branch lines of an expanded union are pushed to sit UNDER the
170+
* `code: message` line this file prints for the union itself.
171+
*
172+
* `formatZodIssue` indents its own depth-0 line by 2 spaces and each nested
173+
* level by 2 more; this file's per-issue block is at 4/6. Adding 4 puts the
174+
* first branch level at 8 — one step below the `invalid_union: Invalid input`
175+
* line it explains — and keeps every deeper level nested relative to it.
176+
*/
177+
const UNION_BRANCH_REINDENT = ' ';
178+
179+
/**
180+
* The lines that explain an `invalid_union`, or nothing at all.
181+
*
182+
* Zod folds every branch of a failed union into ONE issue whose own `message`
183+
* is the literal `"Invalid input"`; each branch's real rejection sits in
184+
* `issue.errors[]`, with paths relative to the union's own. A consumer that
185+
* walks only the top level therefore prints `invalid_union: Invalid input` and
186+
* drops the branch that says WHICH key is wrong — which is what `os validate`,
187+
* `os build` and `os plugin build` did until #5341, so every curated
188+
* prescription the #4001 campaign wrote for a strict shape behind a union was
189+
* produced and never delivered to the author's terminal.
190+
*
191+
* The branch SELECTION (drop branches that only say "wrong kind of value",
192+
* prefer the branch complaining least so one stray key is not reported once per
193+
* branch, break ties on `unrecognized_keys`, absolute paths, bounded depth) is
194+
* `@objectstack/spec`'s, reused rather than re-derived: this is the third
195+
* consumer of the same defect after `formatZodError` (#4971, PR #5342) and the
196+
* REST wire's `zodIssuesToFields` (#5014, PR #5362), and one mistake must not
197+
* get three different prescriptions depending on which surface the author hit.
198+
* Unlike the wire — which needs structured `{field, code, message}` entries and
199+
* so had to re-implement the ranking — the terminal needs exactly the STRING
200+
* that spec already exports, so here the reuse is a plain import.
201+
*
202+
* Line 0 of that render is the union's own verdict, which the caller has
203+
* already printed in this file's own idiom; only the explanation is returned,
204+
* so the change is strictly ADDITIVE — nothing that printed before #5341 stops
205+
* printing. A non-union issue renders as a single line, hence never reaches
206+
* here and could not add one anyway.
207+
*/
208+
function unionBranchLines(issue: unknown): string[] {
209+
if ((issue as { code?: unknown } | null)?.code !== 'invalid_union') return [];
210+
return formatZodIssue(issue as Parameters<typeof formatZodIssue>[0])
211+
.split('\n')
212+
.slice(1)
213+
.map((line) => `${UNION_BRANCH_REINDENT}${line}`);
214+
}
215+
167216
export function formatZodErrors(error: ZodError) {
168217
const issues = error.issues || (error as any).errors || [];
169218

@@ -199,6 +248,11 @@ export function formatZodErrors(error: ZodError) {
199248
if ((issue as any).received) {
200249
console.log(chalk.dim(` received: ${chalk.red((issue as any).received)}`));
201250
}
251+
252+
// [#5341] …and, for a union, the branch that actually explains it.
253+
for (const line of unionBranchLines(issue)) {
254+
console.log(chalk.dim(line));
255+
}
202256
}
203257
}
204258

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Does a rejection behind a `z.union` reach the terminal? (#5341)
5+
*
6+
* Zod folds every branch of a failed union into ONE top-level issue whose own
7+
* `message` is the literal `"Invalid input"`; each branch's real rejection —
8+
* required-property and unknown-key prescriptions alike — sits in
9+
* `issue.errors[]` with paths RELATIVE to the union's own. A consumer that
10+
* walks only the top level prints `invalid_union: Invalid input` and drops
11+
* every curated word the #4001 campaign wrote for the strict shapes behind
12+
* that union.
13+
*
14+
* This is the same defect in its THIRD consumer, each a separate piece of code:
15+
*
16+
* 1. `formatZodError` (`spec/src/shared/error-map.zod.ts`) — #4971, PR #5342;
17+
* 2. `zodIssuesToFields` (`rest/src/rest-server.ts`, the wire) — #5014, PR #5362;
18+
* 3. `formatZodErrors` (`cli/src/utils/format.ts`, the terminal) — THIS file.
19+
*
20+
* (3) is what `os validate`, `os build` (compile) and `os plugin build` print
21+
* through — three commands, one function — so until #5341 an author publishing
22+
* from the terminal was the one reader the campaign's prose never reached,
23+
* while the `--json` payload beside it carried the whole tree.
24+
*
25+
* The whole risk of fixing it is the opposite failure: N branches × the same
26+
* mistake reported N times, which is what made `view.zod.ts`'s `submitBehavior`
27+
* reach for `discriminatedUnion` (#4001 批 6c). Both directions are pinned.
28+
*/
29+
30+
import { describe, expect, it } from 'vitest';
31+
import { execFileSync } from 'node:child_process';
32+
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
33+
import { tmpdir } from 'node:os';
34+
import { join } from 'node:path';
35+
import { fileURLToPath } from 'node:url';
36+
import { z } from 'zod';
37+
import { ObjectStackDefinitionSchema } from '@objectstack/spec';
38+
import { formatZodErrors } from '../src/utils/format';
39+
40+
const cliBin = join(fileURLToPath(new URL('.', import.meta.url)), '..', 'bin', 'run-dev.js');
41+
42+
/** Drop SGR sequences so an assertion reads the words, not chalk's opinion. */
43+
const stripAnsi = (s: string) => s.replace(/\u001B\[[0-9;]*m/g, '');
44+
45+
/** Run `formatZodErrors` and return everything it printed, as one string. */
46+
function render(error: z.ZodError): string {
47+
const captured: string[] = [];
48+
const original = console.log;
49+
console.log = (...args: unknown[]) => {
50+
captured.push(args.map(String).join(' '));
51+
};
52+
try {
53+
formatZodErrors(error as never);
54+
} finally {
55+
console.log = original;
56+
}
57+
return stripAnsi(captured.join('\n'));
58+
}
59+
60+
/** The campaign's shape: a string form OR a strict object form. */
61+
const ACTION_REF = z.union([
62+
z.string(),
63+
z.strictObject({ type: z.string(), params: z.record(z.string(), z.unknown()).optional() }),
64+
]);
65+
66+
describe('[#5341] formatZodErrors expands invalid_union branches', () => {
67+
it('prints the failing branch prose under the union line', () => {
68+
const out = render(ACTION_REF.safeParse({ type: 'log', args: { a: 1 } }).error!);
69+
// The union's own two lines are PRESERVED — they are what says "no branch
70+
// matched", and keeping them makes this change strictly additive: nothing
71+
// that printed before #5341 stopped printing.
72+
expect(out).toContain('invalid_union: Invalid input');
73+
// …and the branch's prescription now arrives with them.
74+
expect(out).toContain('Unrecognized key: "args"');
75+
});
76+
77+
it('drops the kind-mismatch branch that carries no prescription', () => {
78+
const out = render(ACTION_REF.safeParse({ type: 'log', args: 1 }).error!);
79+
// Paired deliberately: the `not` alone would also pass if the expansion
80+
// produced NOTHING — a green for the empty reason. The positive assertion
81+
// is what makes the negative one mean "selected against", not "absent".
82+
expect(out).toContain('args');
83+
// `expected string, received object` is the string branch complaining that
84+
// the author did not write a string. They never meant to.
85+
expect(out).not.toContain('expected string');
86+
});
87+
88+
it('resolves branch paths against the union, not relative to it', () => {
89+
const schema = z.object({ actions: z.array(ACTION_REF) });
90+
const out = render(schema.safeParse({ actions: [{ type: 'log', args: { a: 1 } }] }).error!);
91+
expect(out).toContain('✗ actions.0: Unrecognized key: "args"');
92+
// Never the bare relative path a naive splice would print.
93+
expect(out).not.toContain('✗ (root): Unrecognized key');
94+
});
95+
96+
it('expands a union nested inside a union', () => {
97+
const schema = z.object({ on: z.union([z.string(), z.object({ actions: z.array(ACTION_REF) })]) });
98+
const out = render(schema.safeParse({ on: { actions: [{ type: 'log', args: { a: 1 } }] } }).error!);
99+
expect(out).toContain('✗ on.actions.0: Invalid input');
100+
expect(out).toContain('✗ on.actions.0: Unrecognized key: "args"');
101+
});
102+
103+
// ⚠️ THE anti-regression, mirroring the pin #4971 left in
104+
// `spec/src/shared/error-map.test.ts`. #4001 批 6c measured a plain `z.union`
105+
// of four strict members reporting one bad key once per member. Selecting the
106+
// branch that complains LEAST is what keeps the expansion from reintroducing
107+
// it: the member the author was aiming at reports only the stray key, while
108+
// the others also report a wrong discriminator and their own missing requireds.
109+
it('reports one unknown key ONCE, not once per branch', () => {
110+
const union = z.union([
111+
z.strictObject({ kind: z.literal('a'), x: z.string() }),
112+
z.strictObject({ kind: z.literal('b'), y: z.string() }),
113+
z.strictObject({ kind: z.literal('c'), z: z.string() }),
114+
]);
115+
const out = render(union.safeParse({ kind: 'a', x: 'ok', bogus: 1 }).error!);
116+
117+
expect(out.match(/bogus/g)?.length).toBe(1);
118+
// The two shapes the author was not writing stay out of the terminal.
119+
expect(out).not.toContain('expected "b"');
120+
expect(out).not.toContain('expected "c"');
121+
});
122+
123+
it('leaves a non-union issue rendered exactly as before', () => {
124+
const out = render(z.object({ name: z.string() }).safeParse({ name: 1 }).error!);
125+
expect(out).toContain('✗ name');
126+
expect(out).toContain('invalid_type:');
127+
expect(out).toContain('expected: string');
128+
// The footer counts `error.issues`, unchanged: a union is ONE issue no
129+
// matter how many lines explain it, which is what keeps this number
130+
// agreeing with the `--json` payload beside it.
131+
expect(out).toContain('1 validation error(s) total');
132+
});
133+
134+
it('counts a union as one issue however many lines explain it', () => {
135+
const out = render(ACTION_REF.safeParse({ type: 'log', args: { a: 1 } }).error!);
136+
expect(out).toContain('1 validation error(s) total');
137+
});
138+
});
139+
140+
/**
141+
* The live specimen, on the surface `os validate` actually parses.
142+
*
143+
* `views[].list.sort` is `z.union([z.string(), z.array(<strict sort entry>)])`
144+
* and the entry declares the #4721 alias `direction → order` — the same tuple
145+
* under a different word, which is worth a prescription precisely because
146+
* getting it wrong REVERSES the sort silently. Behind a union, that
147+
* prescription was produced on every run and delivered on none.
148+
*/
149+
const SORT_ALIAS_STACK = {
150+
manifest: { id: 'union_probe', name: 'Union Probe', namespace: 'union_probe', version: '1.0.0', type: 'app' },
151+
views: [
152+
{
153+
name: 'union_probe_view',
154+
object: 'union_probe_obj',
155+
list: {
156+
name: 'union_probe_list',
157+
label: 'Union Probe',
158+
type: 'grid',
159+
columns: ['name'],
160+
sort: [{ field: 'name', direction: 'desc' }],
161+
},
162+
},
163+
],
164+
};
165+
166+
/** Run a CLI command in a temp dir holding `stack` as the config. */
167+
function runCli(command: string, stack: Record<string, unknown>, args: string[] = []): { exitCode: number; output: string } {
168+
const dir = mkdtempSync(join(tmpdir(), 'os-union-format-'));
169+
try {
170+
// A plain literal, not `defineStack`/`defineView`: those factories parse
171+
// eagerly and would throw through spec's OWN formatter, which has expanded
172+
// unions since #4971 — the one thing this file must not accidentally
173+
// measure instead of the CLI's renderer.
174+
writeFileSync(join(dir, 'objectstack.config.mjs'), `export default ${JSON.stringify(stack, null, 2)};\n`);
175+
try {
176+
const output = execFileSync(process.execPath, [cliBin, command, ...args], {
177+
cwd: dir,
178+
encoding: 'utf8',
179+
stdio: 'pipe',
180+
});
181+
return { exitCode: 0, output: stripAnsi(output) };
182+
} catch (error: any) {
183+
return { exitCode: error.status ?? 1, output: stripAnsi(`${error.stdout ?? ''}${error.stderr ?? ''}`) };
184+
}
185+
} finally {
186+
rmSync(dir, { recursive: true, force: true });
187+
}
188+
}
189+
190+
describe('[#5341] `os validate` delivers a union branch prescription', () => {
191+
// Reverse verification, direction declared up front: the failure this
192+
// reports must be the union and nothing else, so the schema-level control
193+
// runs first. If the stack failed for some unrelated reason the terminal
194+
// assertion below could pass on the wrong error entirely.
195+
it('the specimen fails on exactly one issue, and that issue is the union', () => {
196+
const result = ObjectStackDefinitionSchema.safeParse(SORT_ALIAS_STACK);
197+
expect(result.success).toBe(false);
198+
const issues = result.success ? [] : result.error.issues;
199+
expect(issues).toHaveLength(1);
200+
expect(issues[0]!.code).toBe('invalid_union');
201+
// The prescription exists in the payload — it always has. Delivery is the
202+
// only thing #5341 is about.
203+
expect(JSON.stringify(issues[0])).toContain('`direction` → `order`');
204+
});
205+
206+
it('prints the prescription, not a bare `invalid_union: Invalid input`', () => {
207+
const { exitCode, output } = runCli('validate', SORT_ALIAS_STACK);
208+
expect(exitCode, `os validate accepted a stack with an aliased sort key:\n${output}`).not.toBe(0);
209+
expect(output).toContain('views.0.list.sort');
210+
expect(output).toContain('`direction` → `order`');
211+
}, 120_000);
212+
213+
it('leaves the `--json` payload exactly as it was — full, and nested', () => {
214+
// The machine path never had this defect: it passes `error.issues` through,
215+
// so the branch tree was always on it. Pinned here because the fix is one
216+
// `console.log` loop away from being "helpfully" moved into the payload.
217+
const { exitCode, output } = runCli('validate', SORT_ALIAS_STACK, ['--json']);
218+
expect(exitCode).not.toBe(0);
219+
const payload = JSON.parse(output.slice(output.indexOf('{')));
220+
expect(payload.valid).toBe(false);
221+
expect(payload.errors).toHaveLength(1);
222+
expect(payload.errors[0].code).toBe('invalid_union');
223+
// The branch tree, untouched — and NOT flattened into extra `errors[]` rows.
224+
expect(JSON.stringify(payload.errors[0].errors)).toContain('`direction` → `order`');
225+
}, 120_000);
226+
});

0 commit comments

Comments
 (0)