fix(cli): make os validate --json --strict exit 1 where --strict already does

.changeset/validate-json-strict-exit-parity.md

@@ -0,0 +1,74 @@
+---
+"@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.
+
+## 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.
+
+<!-- adr-0087: not-required (no-migration-prescription) An exit-code parity fix on a CLI flag. No authorable key, export, config field or stored `sys_metadata` shape changes, so there is nothing for `objectstack migrate meta` or the upgrade guide to carry — the remedy is a pipeline-side choice of flag, not a rewrite of anything an author wrote. -->

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

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');