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); +});