From 037b0fa53a1afc0b0567c8855e8cbbadb3b5c884 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 00:09:55 +0000 Subject: [PATCH] fix(cli): carry the four structural advisories in `os validate --json` (#10953) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JSON payload was emitted and the command returned above the block that computes the no-objects / no-apps+plugins / missing-manifest.id / missing-manifest.namespace advisories, so all four were printed for a human and structurally unreachable for `--json` — the one audience the flag exists for. Compute them once above the `if (flags.json)` branch and let both faces consume that list, the move this file already made for `unknownKeyWarnings`. Text-face warning order is unchanged. Pinned by an equivalence test asserting both faces carry the same warning set for the same config, with each side derived from its own real output. Co-Authored-By: Claude Opus 5 --- .../validate-json-structural-warnings.md | 40 +++ packages/cli/src/commands/validate.ts | 43 ++- .../validate-json-warning-parity.e2e.test.ts | 281 ++++++++++++++++++ 3 files changed, 351 insertions(+), 13 deletions(-) create mode 100644 .changeset/validate-json-structural-warnings.md create mode 100644 packages/cli/test/validate-json-warning-parity.e2e.test.ts diff --git a/.changeset/validate-json-structural-warnings.md b/.changeset/validate-json-structural-warnings.md new file mode 100644 index 0000000000..1ead65bd17 --- /dev/null +++ b/.changeset/validate-json-structural-warnings.md @@ -0,0 +1,40 @@ +--- +"@objectstack/cli": patch +--- + +Carry the four structural advisories in `os validate --json` (#10953) + +`--json` exists so CI can gate on the advisories `os validate` computes, and +four of them could never reach it. In `commands/validate.ts` the JSON payload +was emitted and the command `return`ed **above** the block that computes them, +so these four were printed for a human and structurally unreachable for the +machine: + +- `No objects defined — this stack has no data model` +- `No apps or plugins defined — this stack may not do much` +- `Missing manifest.id — required for deployment` +- `Missing manifest.namespace — required for multi-app hosting` + +Measured before the fix on a config with no manifest, no objects and no apps — +the text face printed all four; `os validate --json` reported `"warnings": []` +for the byte-identical config. The documented purpose of the flag was defeated. + +The four conditions now compute once, above the `if (flags.json)` branch, and +both faces consume that one list — the same move this file already made for +`unknownKeyWarnings`, for the same reason: a single list cannot drift from +itself. The text face's warning order is unchanged. + +**Declared shape is unchanged; content is not.** `warnings` was already a +heterogeneous array — registry and package-doc findings ride as objects, +unknown-key advisories as strings — so the four arriving as strings introduces +no new element type and no new key. What changes is reachability: a pipeline +gating on `warnings.length === 0` will now see these four where it previously +saw an empty array. That is the defect being corrected rather than a new +signal, which is why this is a patch and matches the bump this repo used for +the previous fix of the same class (readonly flow-write warnings missing from +the same array). + +Pinned by `test/validate-json-warning-parity.e2e.test.ts`, which asserts the +two faces carry the same warning **set** for the same config — both sides +derived from their own real output — so the class stays closed rather than +just these four instances. diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 6c7cd6e07e..96c198b6d7 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -223,6 +223,30 @@ export default class Validate extends Command { // is a newer major than the app declares, point at the migration guide. const specGap = checkSpecVersionGap(config.manifest); + // 4b. Structural advisories (non-blocking) — computed HERE, above the + // `if (flags.json)` branch, for exactly the reason `unknownKeyWarnings` + // is computed up beside `normalized`: everything below that branch only + // ever feeds the text path. These four were in that state — printed for + // a human, structurally unreachable for `--json`, which is the one + // audience the flag exists for. A CI script gating on + // `os validate --json` advisories saw `warnings: []` however true the + // conditions were. Computed once and consumed by BOTH faces below, so + // the two cannot disagree by construction — the same "a single list + // cannot drift from itself" move this file already had to make twice. + const structuralWarnings: string[] = []; + if (stats.objects === 0) { + structuralWarnings.push('No objects defined — this stack has no data model'); + } + if (stats.apps === 0 && stats.plugins === 0) { + structuralWarnings.push('No apps or plugins defined — this stack may not do much'); + } + if (!config.manifest?.id) { + structuralWarnings.push('Missing manifest.id — required for deployment'); + } + if (!config.manifest?.namespace) { + structuralWarnings.push('Missing manifest.namespace — required for multi-app hosting'); + } + if (flags.json) { await emitJson({ valid: true, @@ -232,7 +256,7 @@ export default class Validate extends Command { // hand-maintained concatenation of per-gate arrays, and it leaked // twice: warnings computed and then dropped from `--json` while the // console printed them. A single list cannot drift from itself. - warnings: [...ruleAdvisories, ...docWarnings, ...unknownKeyWarnings, ...capProviderWarnings], + warnings: [...ruleAdvisories, ...docWarnings, ...unknownKeyWarnings, ...capProviderWarnings, ...structuralWarnings], conversions: conversionNotices, specVersionGap: specGap, duration: timer.elapsed(), @@ -271,18 +295,11 @@ export default class Validate extends Command { warnings.push(`${w.path}: ${w.message}`); } - if (stats.objects === 0) { - warnings.push('No objects defined — this stack has no data model'); - } - if (stats.apps === 0 && stats.plugins === 0) { - warnings.push('No apps or plugins defined — this stack may not do much'); - } - if (!config.manifest?.id) { - warnings.push('Missing manifest.id — required for deployment'); - } - if (!config.manifest?.namespace) { - warnings.push('Missing manifest.namespace — required for multi-app hosting'); - } + // The four structural advisories, computed above the `if (flags.json)` + // branch so `--json` carries them too. Appended HERE, in the position the + // four inline `if` blocks used to occupy, so the text face's warning ORDER + // is byte-for-byte what it was. + warnings.push(...structuralWarnings); // 6. Display results console.log(''); diff --git a/packages/cli/test/validate-json-warning-parity.e2e.test.ts b/packages/cli/test/validate-json-warning-parity.e2e.test.ts new file mode 100644 index 0000000000..0409c5d086 --- /dev/null +++ b/packages/cli/test/validate-json-warning-parity.e2e.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #10953 — `os validate` and `os validate --json` carry the SAME warning set + * for the same config. + * + * ## The defect this pins shut + * + * `--json` exists so CI can gate on the advisories `os validate` computes. Four + * of them could never reach it: `commands/validate.ts` emitted the JSON payload + * and `return`ed *above* the block that computes them, so the four structural + * advisories (no objects / no apps+plugins / missing `manifest.id` / missing + * `manifest.namespace`) were printed for a human and structurally unreachable + * for the machine. Measured on the `bare` fixture below before the fix: the text + * face printed four warnings, `--json` reported `"warnings": []` for the byte + * -identical config. The documented purpose of the flag was defeated. + * + * ## Why this pin is an EQUIVALENCE pin and not four `toContain` assertions + * + * Asserting "these four now appear in the JSON" would close four holes and leave + * the CLASS open: the next advisory added below the `if (flags.json)` branch + * would be text-only again and nothing would say so. So both sides are derived + * from their own PRODUCTION SOURCE — the real text stdout and the real JSON + * payload of two real CLI runs — and compared as sets. Nothing in this file + * hardcodes an expected message: a transcribed list would test the transcription, + * not the command. + * + * The one number each fixture does state is a `floor` — the minimum warning + * count the run must produce. It is not an expected list; it exists solely so + * the set equality cannot pass VACUOUSLY. Without it a config that produces zero + * warnings on both faces satisfies "the sets are equal" perfectly, and this file + * would stay green with the fix reverted. + * + * ## The two declared fields this comparison deliberately excludes + * + * The text face folds two more advisory streams into the same `⚠` block that + * the JSON payload carries as its own top-level fields instead — `conversions` + * (ADR-0087 D2 load-time conversion notices) and `specVersionGap`. Those are a + * declared difference in SHAPE, not a drop: the information is reachable on both + * faces. Rather than silently ignoring them, every fixture ASSERTS both are + * empty, so the exact set equality below is honest about its scope — and if a + * future change makes either non-empty for these fixtures, this file fails + * loudly instead of quietly comparing a subset. + * + * ## Not in scope: zero-state stat rows + * + * `printMetadataStats` prints zero-state section rows (`UI: 0 Apps`, and more of + * them once the `zeroFallback` work lands). Those are a different output element + * from a non-blocking warning — they carry no `⚠` and never enter either + * `warnings` array — so they cannot enter this comparison. Verified against the + * fixtures below, whose text output carries such rows while the warning sets + * stay exactly equal. + * + * Spawns the real CLI (the `validate-top-level-strict.e2e.test.ts` pattern: + * `bin/run-dev.js` + tsx, no dependency on `packages/cli/dist`) because the + * divergence being pinned is between two real invocations, not two functions. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFile } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +interface Fixture { + /** Directory name / test label. */ + readonly name: string; + readonly what: string; + readonly source: string; + /** + * Minimum warnings this config must raise on the text face. An anti-vacuity + * floor, NOT an expected list — see the header. + */ + readonly floor: number; +} + +const FIXTURES: readonly Fixture[] = [ + { + name: 'bare', + what: 'all four structural advisories at once (the card\'s measurement)', + // No `manifest` at all, no objects, no apps: the only shape that trips all + // four conditions in one run. `manifest.id` is schema-REQUIRED when a + // `manifest` is present, so a config that merely omits the id fails the + // parse and exits long before any warning is computed. + source: ` +export default { + objects: [], + apps: [], +}; +`, + floor: 4, + }, + { + name: 'mixed', + what: 'a registry advisory (structured) beside a structural one (string)', + // The JSON `warnings` array is heterogeneous by construction: registry and + // doc findings ride as OBJECTS, unknown-key and structural advisories as + // STRINGS. This fixture puts one of each in the same run, so the parity + // check is exercised across both representations rather than only over the + // string ones. `externalSharingModel` is ledger-marked `authorWarn`, which + // is what raises the registry-side advisory. + source: ` +export default { + manifest: { id: 'com.example.parity', name: 'parity', version: '1.0.0', type: 'app', namespace: 'parity' }, + objects: [{ + name: 'parity_ticket', + label: 'Ticket', + sharingModel: 'private', + externalSharingModel: 'private', + fields: { title: { type: 'text', label: 'Title' } }, + }], +}; +`, + floor: 2, + }, +]; + +/** The zero-warning control — see the test that uses it. */ +const CLEAN_SOURCE = ` +export default { + manifest: { id: 'com.example.clean', name: 'clean', version: '1.0.0', type: 'app', namespace: 'clean' }, + objects: [{ + name: 'clean_ticket', + label: 'Ticket', + sharingModel: 'private', + fields: { title: { type: 'text', label: 'Title' } }, + }], + apps: [{ name: 'clean_app', label: 'Clean App' }], +}; +`; + +interface Run { + code: number; + stdout: string; + stderr: string; +} + +function runCli(args: string[], cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + TSX, + [CLI, ...args], + { cwd, maxBuffer: 16 * 1024 * 1024, env: { ...process.env, NO_COLOR: '1' } }, + (err, stdout, stderr) => { + resolvePromise({ + code: err ? (typeof (err as { code?: unknown }).code === 'number' ? (err as unknown as { code: number }).code : 1) : 0, + stdout: String(stdout), + stderr: String(stderr), + }); + }, + ); + }); +} + +/** + * The text face's warning set, read off the rendered output — its production + * source. Every non-blocking warning `os validate` prints goes through the one + * `⚠` line shape. + */ +function textWarnings(stdout: string): string[] { + return stdout + .split('\n') + .filter((l) => l.includes('⚠')) + .map((l) => l.slice(l.indexOf('⚠') + 1).trim()) + .filter((l) => l.length > 0); +} + +/** + * The JSON face's warning set, read off the emitted payload — its production + * source. Entries ride either as a bare string or as a finding object; the + * message is the part the text face renders, so it is the comparable key. + */ +function jsonWarnings(payload: { warnings?: unknown }): string[] { + const raw = Array.isArray(payload.warnings) ? payload.warnings : []; + return raw.map((w) => { + if (typeof w === 'string') return w; + const message = (w as { message?: unknown }).message; + if (typeof message === 'string') return message; + throw new Error(`warning entry is neither a string nor a {message} object: ${JSON.stringify(w)}`); + }); +} + +/** + * Match each JSON message to a DISTINCT text line by containment, and report + * what is left over on either side. + * + * Containment rather than equality because a structured finding keeps `where` + * as its own field while the text face renders `${where}: ${message}` — the + * same advisory, one face carrying the locus separately. Comparing on the + * message avoids re-implementing that join here, which would make this file a + * pin on the formatting rather than on the parity. + * + * Consuming a distinct line per message is what makes this a bijection and not + * a mutual-covering check: two JSON copies of one advisory cannot both be + * satisfied by a single printed line. + */ +function pair(jsonMessages: string[], lines: string[]): { jsonOnly: string[]; textOnly: string[] } { + const remaining = [...lines]; + const jsonOnly: string[] = []; + for (const m of jsonMessages) { + const i = remaining.findIndex((l) => l.includes(m)); + if (i === -1) jsonOnly.push(m); + else remaining.splice(i, 1); + } + return { jsonOnly, textOnly: remaining }; +} + +const dirs = new Map(); + +beforeAll(() => { + for (const f of FIXTURES) { + const dir = mkdtempSync(join(tmpdir(), `os-validate-parity-${f.name}-`)); + writeFileSync(join(dir, 'objectstack.config.ts'), f.source); + dirs.set(f.name, dir); + } + const cleanDir = mkdtempSync(join(tmpdir(), 'os-validate-parity-clean-')); + writeFileSync(join(cleanDir, 'objectstack.config.ts'), CLEAN_SOURCE); + dirs.set('clean', cleanDir); +}); + +afterAll(() => { + for (const dir of dirs.values()) rmSync(dir, { recursive: true, force: true }); +}); + +describe('#10953 — text and --json carry the same warning set', () => { + for (const f of FIXTURES) { + it(`${f.name}: ${f.what}`, async () => { + const dir = dirs.get(f.name)!; + + const text = await runCli(['validate'], dir); + expect(text.code, `text run failed:\n${text.stdout}\n${text.stderr}`).toBe(0); + + const json = await runCli(['validate', '--json'], dir); + expect(json.code, `json run failed:\n${json.stdout}\n${json.stderr}`).toBe(0); + + const payload = JSON.parse(json.stdout) as { + warnings?: unknown; + conversions?: unknown; + specVersionGap?: unknown; + }; + + // Scope declaration, asserted rather than assumed — see the header. + expect(payload.conversions, 'fixture must raise no conversion notices').toEqual([]); + expect(payload.specVersionGap, 'fixture must raise no spec-version gap').toBeNull(); + + const lines = textWarnings(text.stdout); + const messages = jsonWarnings(payload); + + // Anti-vacuity: a run with nothing to compare would satisfy set equality. + expect(lines.length, `text face raised too few warnings to compare:\n${text.stdout}`) + .toBeGreaterThanOrEqual(f.floor); + + const { jsonOnly, textOnly } = pair(messages, lines); + expect( + { jsonOnly, textOnly }, + `warning sets diverge for the same config.\n` + + `text (${lines.length}):\n ${lines.join('\n ')}\n` + + `json (${messages.length}):\n ${messages.join('\n ')}`, + ).toEqual({ jsonOnly: [], textOnly: [] }); + }, 120_000); + } + + it('control: a config with nothing to warn about is empty on BOTH faces', async () => { + // Proves the parity above is not satisfied by one face simply echoing the + // other's non-emptiness — the fix must not manufacture warnings either. + const dir = dirs.get('clean')!; + + const text = await runCli(['validate'], dir); + expect(text.code, `text run failed:\n${text.stdout}\n${text.stderr}`).toBe(0); + expect(textWarnings(text.stdout)).toEqual([]); + + const json = await runCli(['validate', '--json'], dir); + expect(json.code, `json run failed:\n${json.stdout}\n${json.stderr}`).toBe(0); + expect(jsonWarnings(JSON.parse(json.stdout))).toEqual([]); + }, 120_000); +});