From 79844ec32de73077901c78f9d203557856b1920a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:55:27 +0000 Subject: [PATCH 1/2] fix(cli): make `os validate --json --strict` exit 1 where `--strict` already does (#11174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `--json` branch emitted its payload and returned above the only `flags.strict` reader, which sat inside the text-rendering block. On one config with one flag the two faces answered differently: text exited 1 with "Strict mode: warnings treated as errors", `--json --strict` exited 0. The flag was accepted, documented for CI in `content/docs/deployment/cli.mdx`, and inert. Assemble the warning list once, above the `if (flags.json)` branch, and have both faces gate on that same list, so the two exit codes cannot drift apart again. The JSON status rides in `emitJson`'s `CliExitCode` slot — the declared channel for pairing a `--json` document with the status the shell reads — so the payload stays exactly one parseable document. The gate reads the text face's list rather than the payload's `warnings` field: the two differ by the ADR-0087 conversion notices, which the payload carries under `conversions`, and gating on the field would have left the same divergence for a conversion-only config. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../validate-json-strict-exit-parity.md | 51 +++++ .../validate-json-strict-exit.e2e.test.ts | 195 ++++++++++++++++++ packages/cli/src/commands/validate.ts | 87 ++++++-- 3 files changed, 313 insertions(+), 20 deletions(-) create mode 100644 .changeset/validate-json-strict-exit-parity.md create mode 100644 packages/cli/src/commands/validate-json-strict-exit.e2e.test.ts diff --git a/.changeset/validate-json-strict-exit-parity.md b/.changeset/validate-json-strict-exit-parity.md new file mode 100644 index 0000000000..f7d467cf9c --- /dev/null +++ b/.changeset/validate-json-strict-exit-parity.md @@ -0,0 +1,51 @@ +--- +"@objectstack/cli": minor +--- + +fix(cli): `os validate --json --strict` exits 1 on the configs `--strict` already exits 1 for (#11174) + +`commands/validate.ts` emitted the `--json` payload and `return`ed *above* the only +`flags.strict` reader, which sat inside the text-rendering block. So on one config, +one flag, two answers: + +``` +os validate --strict → exit 1 ("Strict mode: warnings treated as errors") +os validate --json --strict → exit 0 +``` + +`--strict` was accepted, documented — `content/docs/deployment/cli.mdx` spells +`os validate --json --strict` twice in its CI/CD section, once as a GitHub Actions +step — and inert whenever `--json` was also passed. That combination is the one +audience the flag exists for: a pipeline gating on the exit status of the documented +invocation read 0 and concluded the stack was clean. + +The `--strict` gate now reads the text face's own warning list, which is assembled +once and consumed by both faces, so the two exit codes cannot drift apart again. The +gate deliberately does **not** read the payload's `warnings` field: the two differ by +the ADR-0087 load-time conversion notices, which the text face folds into its warning +block while the payload carries them under `conversions`. Gating on the field would +have left the same divergence in place for a config whose only advisories are +conversion notices. `specVersionGap` stays outside `--strict` on both faces, as it +always has been on the text one. + +`valid: true` beside a non-zero exit is the text face verbatim, not a contradiction: +that path prints "Validation passed" and *then* fails for strict. The stack is +schema-valid; `--strict` is what promotes its advisories to a failure. + +**BREAKING** for one caller shape, and the reason this is not a patch: a pipeline +running `os validate --json --strict` over a stack that raises non-blocking +advisories was green and will now be red. Nothing was removed or renamed and no +authored metadata changes — the accept set is identical and the exit status is the +only thing that moves — but a release a CI system can take unattended must not flip +a green build to red, so this does not belong in a patch. It is not a major either: +the new behaviour *restores* what `--strict` declares ("Treat warnings as errors") +and what the docs already advertise, rather than contradicting a contract. Under +this repo's launch-window convention (breaking changes ship as `minor` while the +stack versions in lockstep) `minor` is the honest slot. + +If a pipeline wants the old exit status, the fix is to say so rather than to pass a +flag that means the opposite: drop `--strict` and gate on the payload's `warnings` +array, which has carried the full advisory set since #10953. If it goes red instead, +the advisories were always there — the text face had been printing them all along. + + diff --git a/packages/cli/src/commands/validate-json-strict-exit.e2e.test.ts b/packages/cli/src/commands/validate-json-strict-exit.e2e.test.ts new file mode 100644 index 0000000000..ab4b1e8067 --- /dev/null +++ b/packages/cli/src/commands/validate-json-strict-exit.e2e.test.ts @@ -0,0 +1,195 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #11174 — `os validate --strict` reaches the SAME exit status with `--json` as + * without it, over the real CLI process. + * + * ## The defect this pins shut + * + * `commands/validate.ts` emitted the `--json` payload and `return`ed *above* the + * only `flags.strict` reader, which lived inside the text-rendering block. So on + * a config raising four non-blocking advisories: + * + * os validate --strict → exit 1 ("Strict mode: warnings treated as errors") + * os validate --json --strict → exit 0 (same config, same flag) + * + * `--strict` was accepted, documented — `content/docs/deployment/cli.mdx` spells + * `os validate --json --strict` twice in its CI/CD section, once as a GitHub + * Actions step — and inert. A pipeline gating on the exit status of the exact + * documented invocation read 0 and concluded the stack was clean. + * + * This is the second half of a pair. The first (#10953) made the four structural + * advisories *reachable* in the payload, so a pipeline could at least gate on + * `warnings.length` itself; it did not touch the exit code, and a pipeline + * trusting the exit status still could not. + * + * ## Why the assertions are a PARITY matrix and not `expect(code).toBe(1)` + * + * A hardcoded `1` pins one cell and says nothing about the property the card is + * about: that the two faces of one command agree. Both sides here are read from + * their own production source — the real exit status of two real CLI runs of the + * same config — and compared to each other. Nothing states an expected code. + * + * That comparison alone is satisfiable two dishonest ways, and both are closed: + * + * - **vacuously**, by a config that exits 0 on both faces. The `warns` fixture + * carries a FLOOR — its text run must exit non-zero — so equality is only + * ever asserted over a run that genuinely had something to fail on. Without + * it this file stays green with the fix reverted. + * - **by breaking the other face** — making `--json --strict` and text agree at + * 0. The `clean` fixture pins the zero end, so both faces are held to 1 on + * warnings and to 0 without them; neither can move to meet the other. + * + * And one further run, without `--strict`, separates "gates on `--strict`" from + * "fails whenever `--json` sees a warning" — two fixes that pass the matrix + * above identically, only one of which is the one asked for. + * + * ## Why a real child process + * + * `process.exitCode` set inside a vitest worker is not an exit status: the + * number only exists once Node has exited and the kernel has masked it to + * `& 0xFF`. `test/migrate-exit-code.e2e.test.ts` is the precedent and states + * this in the same words — the audience `--json` exists for reads the SHELL, so + * that is what gets asserted. Spawned through `bin/run-dev.js` + tsx, so the + * suite does not depend on `packages/cli/dist` having been built. + * + * ## Why this file is not beside its siblings in `packages/cli/test/` + * + * That directory was held by another in-flight card while this one was written, + * so it was read-only to this change. `src/` turns out to be the stronger of the + * two homes anyway, and deliberately so for the same reason + * `utils/format.exit-code.test.ts` gives for living here: `packages/cli/ + * tsconfig.json` includes `src`, so `pnpm typecheck` compiles this file, while + * no tsc program reads `packages/cli/test/`. `tsconfig.build.json` excludes + * `src/**\/*.test.ts`, so nothing here ships. + */ + +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'); + +/** + * Raises all four structural advisories at once and nothing else: no + * `manifest`, no objects, no apps. The card's own measurement used this shape, + * and it still parses — `manifest.id` is schema-REQUIRED once a `manifest` is + * present, so a config that merely omits the id fails the parse and exits long + * before any advisory is computed. + */ +const WARNS_SOURCE = ` +export default { + objects: [], + apps: [], +}; +`; + +/** The zero-warning control — pins the other end of the matrix. */ +const CLEAN_SOURCE = ` +export default { + manifest: { id: 'com.example.strictexit', name: 'strictexit', version: '1.0.0', type: 'app', namespace: 'strictexit' }, + objects: [{ + name: 'strictexit_ticket', + label: 'Ticket', + sharingModel: 'private', + fields: { title: { type: 'text', label: 'Title' } }, + }], + apps: [{ name: 'strictexit_app', label: 'Strict Exit 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), + }); + }, + ); + }); +} + +let warnsDir: string; +let cleanDir: string; + +beforeAll(() => { + warnsDir = mkdtempSync(join(tmpdir(), 'os-validate-strict-exit-warns-')); + writeFileSync(join(warnsDir, 'objectstack.config.ts'), WARNS_SOURCE); + cleanDir = mkdtempSync(join(tmpdir(), 'os-validate-strict-exit-clean-')); + writeFileSync(join(cleanDir, 'objectstack.config.ts'), CLEAN_SOURCE); +}); + +afterAll(() => { + rmSync(warnsDir, { recursive: true, force: true }); + rmSync(cleanDir, { recursive: true, force: true }); +}); + +describe('#11174 — --strict reaches the same exit status on both faces', () => { + it('a config with advisories: --json --strict exits exactly as --strict does', async () => { + const text = await runCli(['validate', '--strict'], warnsDir); + const json = await runCli(['validate', '--json', '--strict'], warnsDir); + + // Anti-vacuity floor: equality below is only meaningful over a run that had + // something to fail on. This is the reference face, so it is also the + // statement that the text side has not moved to meet the JSON one. + expect( + text.code, + `text --strict must fail on this config for the parity below to mean anything:\n${text.stdout}\n${text.stderr}`, + ).not.toBe(0); + + expect( + json.code, + `--json --strict exited ${json.code} where --strict exited ${text.code}, same config.\n` + + `json stdout:\n${json.stdout}\njson stderr:\n${json.stderr}`, + ).toBe(text.code); + }, 120_000); + + it('the failing --json run still emits exactly one parseable document, carrying the cause', async () => { + // The exit code must not cost the payload. This file's sibling defect + // (`isExitSignal` in `utils/format.ts`) was a `--json` failure path that + // emitted TWO documents back to back, parseable as neither one document nor + // as JSONL — so a non-zero `--json` run is pinned on the document too. + const json = await runCli(['validate', '--json', '--strict'], warnsDir); + + const payload = JSON.parse(json.stdout) as { valid?: unknown; warnings?: unknown }; + + // `valid: true` beside exit 1 is the text face's contract verbatim: it + // prints "Validation passed" and THEN fails for strict. The config is + // schema-valid; `--strict` is what turns its advisories into a failure. + // Pinned because the pairing is new here and reads like a bug otherwise. + expect(payload.valid).toBe(true); + expect(Array.isArray(payload.warnings) && (payload.warnings as unknown[]).length).toBeGreaterThan(0); + }, 120_000); + + it('control: a config with nothing to warn about exits 0 on BOTH faces under --strict', async () => { + const text = await runCli(['validate', '--strict'], cleanDir); + expect(text.code, `text --strict:\n${text.stdout}\n${text.stderr}`).toBe(0); + + const json = await runCli(['validate', '--json', '--strict'], cleanDir); + expect(json.code, `json --strict:\n${json.stdout}\n${json.stderr}`).toBe(0); + }, 120_000); + + it('control: without --strict, the same advisory-raising config still exits 0 under --json', async () => { + // Separates "gates on --strict" from "fails whenever --json sees a warning". + // Both satisfy the parity matrix above; only the first is the flag's meaning. + const json = await runCli(['validate', '--json'], warnsDir); + expect(json.code, `--json without --strict must stay 0:\n${json.stdout}\n${json.stderr}`).toBe(0); + }, 120_000); +}); diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 96c198b6d7..aa3f641ec2 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -247,24 +247,23 @@ export default class Validate extends Command { structuralWarnings.push('Missing manifest.namespace — required for multi-app hosting'); } - if (flags.json) { - await emitJson({ - valid: true, - manifest: config.manifest, - stats, - // One advisory list for the whole registry. This used to be a - // 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, ...structuralWarnings], - conversions: conversionNotices, - specVersionGap: specGap, - duration: timer.elapsed(), - }); - return; - } - - // 5. Warnings (non-blocking) + // 5. Warnings (non-blocking) — assembled HERE, above the `if (flags.json)` + // branch, because this is the list `--strict` gates on and the JSON + // face has to reach the SAME verdict from it. It could not: the payload + // was emitted and `return`ed above the only `flags.strict` reader, so + // `os validate --json --strict` exited 0 on the very configs + // `os validate --strict` exited 1 for. The flag was accepted, + // documented (`content/docs/deployment/cli.mdx` spells the pair twice + // in its CI/CD section, once as a GitHub Actions step) and inert — a + // pipeline gating on the exit status of the documented invocation read + // 0 and called the stack clean. + // + // Hoisting the assembly rather than restating the condition is the same + // move `structuralWarnings` just above and `unknownKeyWarnings` up + // beside `normalized` already made, for the third time in this file: + // ONE list, consumed by both faces, so the two exit codes cannot drift + // from each other by construction. The push ORDER is unchanged, so the + // text face's warning output is byte-for-byte what it was. const warnings: string[] = []; // [#3366] Installable-provider hints — a declared capability whose provider @@ -295,12 +294,56 @@ export default class Validate extends Command { warnings.push(`${w.path}: ${w.message}`); } - // The four structural advisories, computed above the `if (flags.json)` - // branch so `--json` carries them too. Appended HERE, in the position the + // The four structural advisories, computed further up so the `--json` + // payload can carry them too. Appended HERE, last, 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); + if (flags.json) { + await emitJson( + { + valid: true, + manifest: config.manifest, + stats, + // One advisory list for the whole registry. This used to be a + // 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, ...structuralWarnings], + conversions: conversionNotices, + specVersionGap: specGap, + duration: timer.elapsed(), + }, + // `--strict` means one thing — "treat warnings as errors" — and it now + // means it on both faces. The gate reads `warnings`, the text face's + // OWN list, rather than the payload's `warnings` field: the two differ + // by the ADR-0087 conversion notices, which the text face folds into + // its `⚠` block while the payload carries them under `conversions`. + // Gating on the payload field would have left `--json --strict` at 0 + // for a config whose only advisories are conversion notices — the + // same divergence one collection narrower. `specVersionGap` stays out + // on both faces; it is never gated by `--strict` (see below). + // + // `valid: true` beside a 1 is not a contradiction, it is the text + // face verbatim: that path prints "Validation passed" and THEN fails + // for strict. The stack IS schema-valid; `--strict` is what promotes + // its advisories to a failure. + // + // The status rides in `emitJson`'s `CliExitCode` slot rather than a + // following `this.exit(1)`, unlike the failure paths above: those + // must stop a fall-through into the text rendering, while here the + // payload is complete and the `return` is right there. The slot is + // the declared channel for pairing a `--json` document with the + // status the shell reads (`utils/format.ts`; pinned by + // `utils/format.exit-code.test.ts` and `test/migrate-exit-code.e2e.test.ts`), + // and it emits the one document without an ExitError unwinding + // through the catch below. + flags.strict && warnings.length > 0 ? 1 : 0, + ); + return; + } + // 6. Display results console.log(''); printSuccess(`Validation passed ${chalk.dim(`(${timer.display()})`)}`); @@ -321,6 +364,10 @@ export default class Validate extends Command { for (const w of warnings) { console.log(chalk.yellow(` ⚠ ${w}`)); } + // The text face's half of the `--strict` gate. Its JSON counterpart is + // the `CliExitCode` argument at the `emitJson` call above, reading this + // same `warnings` list — change one and change the other, or the two + // faces start disagreeing about the exit status again. if (flags.strict) { console.log(''); printError('Strict mode: warnings treated as errors'); From bb07f74ffa5b3ea2eb614ccdd8a649e62dc8c1b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 09:22:56 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(changeset):=20correct=20the=20migratio?= =?UTF-8?q?n=20advice=20=E2=80=94=20reproducing=20`--strict`=20needs=20`co?= =?UTF-8?q?nversions`=20too=20(#11174)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset told a pipeline to "drop `--strict` and gate on the payload's `warnings` array, which has carried the full advisory set since #10953". That is false in exactly the dimension this PR makes load-bearing: the payload's `warnings` field is the five-way spread WITHOUT the ADR-0087 conversion notices, which ride under `conversions`. A pipeline following that advice would have got a strictly weaker gate than `--strict` — a conversions-only config passes it and fails `--strict` — which is the same silent under-reporting this change exists to remove, published as guidance. Say `warnings.length > 0 || conversions.length > 0`, say plainly that `warnings` alone is narrower and why, and state the resulting payload shape outright: a conversions-only config exits 1 with `"warnings": []` and a populated `conversions`, so predicting the exit code from `warnings.length` is wrong for that config. Changeset prose only — no source, test or bump change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../validate-json-strict-exit-parity.md | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/.changeset/validate-json-strict-exit-parity.md b/.changeset/validate-json-strict-exit-parity.md index f7d467cf9c..76427a0251 100644 --- a/.changeset/validate-json-strict-exit-parity.md +++ b/.changeset/validate-json-strict-exit-parity.md @@ -43,9 +43,32 @@ and what the docs already advertise, rather than contradicting a contract. Under this repo's launch-window convention (breaking changes ship as `minor` while the stack versions in lockstep) `minor` is the honest slot. -If a pipeline wants the old exit status, the fix is to say so rather than to pass a -flag that means the opposite: drop `--strict` and gate on the payload's `warnings` -array, which has carried the full advisory set since #10953. If it goes red instead, -the advisories were always there — the text face had been printing them all along. +## What a pipeline gating on the payload has to read + +`--strict` gates on the text face's warning list, and that list is **not** the payload's +`warnings` field. The two differ by the ADR-0087 load-time conversion notices: the text +face folds them into its warning block, while the payload carries them separately under +`conversions`. So a pipeline that wants to reproduce `--strict` from the document must +read **both**: + +``` +warnings.length > 0 || conversions.length > 0 +``` + +Gating on `warnings` alone is strictly weaker than `--strict` — a config whose only +advisories are conversion notices passes that check and fails `--strict`. That is the +same silent under-reporting this change exists to remove, so do not reach for the +narrower spelling. + +The consequence is reachable and worth stating outright, because it is surprising: a +conversions-only config now exits **1** with `"warnings": []` and a populated +`conversions`. Predicting the exit code from `warnings.length` alone will be wrong for +exactly that config. Nothing is missing from the document — both advisory streams are in +it — but they sit in two fields and the exit code answers to both. + +If a pipeline genuinely wants the old exit status, the honest fix is to say so rather +than to keep passing a flag that means the opposite: drop `--strict` and read the +payload. If it goes red instead, the advisories were always there — the text face had +been printing them all along.