From 7956a7c308ca69e8683fc50f676cc8a34b4fd01a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 17:36:12 +0000 Subject: [PATCH 1/4] fix(cli): `i18n extract --check --json` compares instead of exiting 0 (#16600) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The machine face returned before anything was compared: `if (flags.json) { … return; }` sat ahead of both the `--check` needs-`--out` guard and the comparison block, so `--check --json` exited 0 on a tree whose bundles had provably drifted. Driven on one fixture, two invocations differing only by `--json`: the first exited 1 reporting `missing:` and the drift sentence, the second exited 0 with the ordinary payload. Same shape as the `--dry-run` branch in #16480. Under `--json`, `--check` is a verdict mode, so the comparison now runs before the one document the run is allowed to write. Drift leaves through this command's existing `{ error, …errorCodeFields }` envelope with exit 1 — no new payload member — and the needs-`--out` refusal is reachable there too. An in-sync tree and a `--json` run that did not ask for `--check` are unchanged. The file list and the comparison are now one closure each (`emittedFiles`, `compareCommitted`, `driftMessage`), read by both faces, so the console and machine `--check` cannot diverge about what this run produces. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- .../i18n-extract-check-json-compares.md | 18 + packages/cli/src/commands/i18n/extract.ts | 177 +++++++-- .../cli/test/i18n-extract-check-json.test.ts | 346 ++++++++++++++++++ 3 files changed, 501 insertions(+), 40 deletions(-) create mode 100644 .changeset/i18n-extract-check-json-compares.md create mode 100644 packages/cli/test/i18n-extract-check-json.test.ts diff --git a/.changeset/i18n-extract-check-json-compares.md b/.changeset/i18n-extract-check-json-compares.md new file mode 100644 index 0000000000..52a6e3d7de --- /dev/null +++ b/.changeset/i18n-extract-check-json-compares.md @@ -0,0 +1,18 @@ +--- +"@objectstack/cli": patch +--- + +`os i18n extract --check --json` now COMPARES. It used to exit 0 having compared nothing, on a tree whose bundles had provably drifted. + +The machine face returned before the comparison ran: `if (flags.json) { … return; }` sat ahead of both the `--check` needs-`--out` guard and the comparison block. Driven on one fixture, two invocations differing only by `--json` — the first exited 1 with `missing: OUT/zh-CN.objects.generated.ts` and `Translation bundles have drifted from the schema`, the second exited 0 with the ordinary extract payload. The first run is the second one's positive control: the drift was really there. Same shape as the `--dry-run` branch repaired one release earlier, and `--json` is if anything the more likely CI spelling of the two, because a pipeline that wants to parse the result reaches for it. + +⚠️ **A pipeline that runs `os i18n extract … --check --json` and was green may now go red, and that is this repair working.** The green was a comparison that never happened; the red is the drift that was already in the tree. The fix is the one the failure names — re-run the same command without `--check` and commit what it writes. + +What each invocation now does, with no new member on any published payload: + +- **drift found** — the run ends on this command's existing `{ "error": … }` envelope with exit 1, carrying the same sentence the console face prints, the regenerate-and-commit command included. Deliberately not a new `drift` / `missing` / `stale` payload member: every other way this command can fail already speaks that envelope, and naming the drifted files in the machine payload would widen a published output face. +- **in sync** — unchanged: the ordinary extract payload, exit 0. +- **`--check` with no `--out`** — the refusal is now reachable under `--json` too, in the same `{ "error": … }` envelope with exit 1. It used to exit 0 with a payload, having been asked for a comparison it could not make. +- **`--json` without `--check`** — unchanged in every respect. + +The run leaves through exactly one of those faces, so stdout still parses as exactly one JSON document. diff --git a/packages/cli/src/commands/i18n/extract.ts b/packages/cli/src/commands/i18n/extract.ts index bf9f8909e0..94597785a8 100644 --- a/packages/cli/src/commands/i18n/extract.ts +++ b/packages/cli/src/commands/i18n/extract.ts @@ -31,6 +31,15 @@ import { const FILL_STRATEGIES: FillStrategy[] = ['empty', 'default', 'todo']; +/** + * The refusal `--check` without `--out` ends on — one string, because two faces + * now reach it. The console run throws it below the skeleton summary; a + * `--json` run throws it from the machine face, where it lands in this + * command's ordinary `{ error }` envelope (#16600). + */ +const CHECK_NEEDS_OUT = + '--check needs --out= — it compares a fresh extract against the bundles committed there.'; + /** * A path for one of this command's output lines: relative to the cwd while that * is still a NAME for the file, absolute once it stops being one. @@ -431,7 +440,126 @@ export default class I18nExtract extends Command { return narrowToCommittedSections(table, committed); }; + /** + * Every file a normal run would write into `dir`, paired with its + * rendered content — the ONE list every face that names this run's files + * reads: the write loop, the console `--check`, and the `--json` + * `--check` below. So no two of them can disagree about what this run + * produces, and in particular `--check` can never compare something the + * write path would not have written. + * + * It was a straight-line `const emitted` built after the `--dry-run` + * branch, which is below the machine face and therefore out of its reach. + * A `--json --check` run needs the same list, so the list moved rather + * than being rebuilt beside it (#16600). + */ + const emittedFiles = (dir: string): Array<{ file: string; content: string; keys: number }> => { + const files: Array<{ file: string; content: string; keys: number }> = []; + for (const locale of localesEmitted) { + for (const mod of emittedModules(locale)) { + files.push({ + file: path.join(dir, `${locale}.${mod.suffix}`), + content: renderTranslationModule(result.bundles[locale], { locale, kind: mod.kind }), + keys: mod.keys, + }); + } + // The provenance companion rides in the SAME list, so `--check` compares + // it by the same byte-for-byte rule as the bundles it belongs to and can + // never diverge from what a real extract writes. + const table = committedSourceHashes(locale); + if (flags['source-hashes'] && table) { + files.push({ + file: path.join(dir, `${locale}.source-hashes.generated.ts`), + content: renderSourceHashModule(table, { locale }), + keys: Object.keys(table).length, + }); + } + } + return files; + }; + + /** What `--check` found: committed files that are absent, and ones whose bytes differ. */ + const compareCommitted = ( + files: ReadonlyArray<{ file: string; content: string }>, + ): { missing: string[]; stale: string[] } => { + const missing: string[] = []; + const stale: string[] = []; + for (const { file, content } of files) { + const shown = displayPath(file); + if (!fs.existsSync(file)) missing.push(shown); + else if (fs.readFileSync(file, 'utf8') !== content) stale.push(shown); + } + return { missing, stale }; + }; + + /** + * The sentence a drifted `--check` ends on, built once so both faces end + * on the same words. {@link rerunWithoutCheck} says why the command it + * names is spelled as a deletion and what the degraded line is for. + */ + const driftMessage = (): string => { + const rerun = rerunWithoutCheck(this.config.bin, this.id, this.argv); + return ( + 'Translation bundles have drifted from the schema. Regenerate and commit:\n' + + (rerun ? ` ${rerun}` : ' re-run the same command without `--check`') + ); + }; + if (flags.json) { + /** + * ⭐ `--check` is a VERDICT mode, so under `--json` the comparison runs + * HERE — before the one document this run is allowed to write (#16600). + * + * ## What was wrong + * + * This branch emitted and returned unconditionally, which put it ahead + * of both the `--check` needs-`--out` guard and the comparison itself. + * Driven on one drifted fixture, the two invocations differing ONLY by + * `--json`: + * + * $ os i18n extract CONFIG --locales=zh-CN --no-metadata-forms + * --out=OUT --check + * missing: OUT/zh-CN.objects.generated.ts + * Translation bundles have drifted from the schema. … + * -> exit 1 + * + * $ … --out=OUT --check --json + * {"totalExpected":…,"counts":…,"bundles":…} + * -> exit 0, nothing compared + * + * The first run is the second one's positive control: the drift is + * provably there and the second reported success. Same shape as the + * `--dry-run` branch in #16480, and `--json` is if anything the more + * likely CI spelling of the two — a pipeline that wants to parse the + * result reaches for it. A check that cannot fail is indistinguishable + * from a check that finds nothing. + * + * ## Why the failure is this command's `{ error }` envelope and NOT a + * new payload member + * + * ⛔ The drift report is deliberately NOT widened into the published + * payload — no `drift` / `missing` / `stale` member is added here. This + * command already has exactly one machine-readable failure envelope, + * twenty lines down in the `catch`: `{ error, …errorCodeFields }`, + * compact, exit 1. Every other way this command can fail already speaks + * it, the `--check` needs-`--out` refusal above included, so routing + * drift through the same `throw` is copying the convention rather than + * settling a second one for the same mode. Which files drifted is a + * genuine addition to a published output face and is its own card. + * + * ⚠️ And it must stay ONE document: emitting the payload here and an + * error envelope afterwards is the two-JSON-documents defect + * {@link isExitSignal} records — unparseable as either one document or + * as JSONL. So the verdict is reached before anything is written, and + * the run leaves through exactly one of the two faces. + * + * ⛔ Returning 0 without comparing must not come back. + */ + if (flags.check) { + if (!flags.out) throw new Error(CHECK_NEEDS_OUT); + const { missing, stale } = compareCommitted(emittedFiles(outDir as string)); + if (missing.length > 0 || stale.length > 0) throw new Error(driftMessage()); + } await emitJson({ totalExpected: result.totalExpected, // Leaves of the `bundles` payload below, locale by locale, so this @@ -526,7 +654,7 @@ export default class I18nExtract extends Command { console.log(''); if (flags.check && !flags.out) { - throw new Error('--check needs --out= — it compares a fresh extract against the bundles committed there.'); + throw new Error(CHECK_NEEDS_OUT); } /** @@ -579,39 +707,12 @@ export default class I18nExtract extends Command { // under `--check`, and `--check` without `--out` already threw. const resolvedOutDir = outDir as string; - // Every file a normal run would emit, paired with its rendered content. - // Both branches below iterate this, so `--check` can never diverge from - // what a real extract writes. - const emitted: Array<{ file: string; content: string; keys: number }> = []; - for (const locale of localesEmitted) { - for (const mod of emittedModules(locale)) { - emitted.push({ - file: path.join(resolvedOutDir, `${locale}.${mod.suffix}`), - content: renderTranslationModule(result.bundles[locale], { locale, kind: mod.kind }), - keys: mod.keys, - }); - } - // The provenance companion rides in the SAME list, so `--check` compares - // it by the same byte-for-byte rule as the bundles it belongs to and can - // never diverge from what a real extract writes. - const table = committedSourceHashes(locale); - if (flags['source-hashes'] && table) { - emitted.push({ - file: path.join(resolvedOutDir, `${locale}.source-hashes.generated.ts`), - content: renderSourceHashModule(table, { locale }), - keys: Object.keys(table).length, - }); - } - } + // Every file a normal run would emit, paired with its rendered content — + // {@link emittedFiles}, the same list the machine face compares. + const emitted = emittedFiles(resolvedOutDir); if (flags.check) { - const stale: string[] = []; - const missing: string[] = []; - for (const { file, content } of emitted) { - const shown = displayPath(file); - if (!fs.existsSync(file)) missing.push(shown); - else if (fs.readFileSync(file, 'utf8') !== content) stale.push(shown); - } + const { missing, stale } = compareCommitted(emitted); if (missing.length === 0 && stale.length === 0) { console.log(''); printSuccess(`${emitted.length} bundle(s) are in sync with the schema ${chalk.dim(`(${timer.display()})`)}`); @@ -621,15 +722,11 @@ export default class I18nExtract extends Command { for (const shown of stale) printError(`out of date: ${shown}`); console.log(''); // The command that regenerates these bytes is THIS run without - // `--check` — the two branches share the `emitted` list above, so the + // `--check` — the two faces share the `emittedFiles` list above, so the // write path cannot produce anything other than what was just - // compared. {@link rerunWithoutCheck} says why it is spelled as a - // deletion and what the degraded line is for. - const rerun = rerunWithoutCheck(this.config.bin, this.id, this.argv); - printError( - 'Translation bundles have drifted from the schema. Regenerate and commit:\n' + - (rerun ? ` ${rerun}` : ' re-run the same command without `--check`'), - ); + // compared. {@link driftMessage} is the sentence, built once so the + // `--json` face ends on the same words. + printError(driftMessage()); process.exit(1); } diff --git a/packages/cli/test/i18n-extract-check-json.test.ts b/packages/cli/test/i18n-extract-check-json.test.ts new file mode 100644 index 0000000000..9914d825a4 --- /dev/null +++ b/packages/cli/test/i18n-extract-check-json.test.ts @@ -0,0 +1,346 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os i18n extract --check --json` COMPARES, and reaches the same verdict as + * the same run without `--json` (#16600). + * + * ## What was wrong + * + * The machine face returned before anything was compared. Driven on one + * drifted fixture, the two invocations differing ONLY by `--json`: + * + * $ os i18n extract CONFIG --locales=zh-CN --no-metadata-forms --out=OUT --check + * missing: OUT/zh-CN.objects.generated.ts + * Translation bundles have drifted from the schema. Regenerate and commit: + * -> exit 1 + * + * $ os i18n extract CONFIG --locales=zh-CN --no-metadata-forms --out=OUT --check --json + * {"totalExpected":775,"counts":{"zh-CN":2},"bundles":{…}} + * -> exit 0, with no drift field, no comparison and no failure + * + * The first run is the second one's positive control: the drift is provably + * there, and the second reported success. `if (flags.json) { … return; }` sat + * ahead of both the `--check` needs-`--out` guard and the comparison block — + * the same shape the `--dry-run` branch had in #16480, and `--json` is if + * anything the more likely CI spelling of the two, because a pipeline that + * wants to parse the result reaches for it. A check that cannot fail is + * indistinguishable from a check that finds nothing. + * + * ## Why these shapes + * + * ⚠️ A case asserting only the new exit code would be satisfied by a `--check` + * that still compares nothing and merely fails, so every drift case here pins + * WHAT WAS REPORTED beside the code — and the report is read out of the JSON + * document, not out of the console text, because the document is the face this + * card is about. + * + * - the drifted cases are stated as an EQUALITY against the same invocation + * WITHOUT `--json`, which is the card's own method rather than a + * re-derivation of it, and the expected values are spelled out as well, + * because an equality alone is also satisfied by two runs that are both + * broken; + * - the in-sync case is what no unconditional failure can pass. ⚠️ It is NOT + * a falsifier for this defect and must not be read as one: before the + * repair, `--check --json` on an in-sync tree ALSO exited 0 carrying the + * payload, so this case is green on both sides of the mutation. That is + * the #16480 lesson in this card's own terms — an exit-code-only suite + * would have stayed green over the very regression it was written for — + * and it is why the drift cases assert the reported drift; + * - `--json` PURITY is asserted on every machine run: stdout has to parse as + * exactly ONE document. A repair that emitted the ordinary payload and + * then an error envelope would pass an "exit 1 and the word drifted" + * reading while producing output that is neither one document nor JSONL — + * the two-document defect `isExitSignal` records in `utils/format.ts`; + * - the needs-`--out` refusal is pinned because it is the OTHER thing the + * early return skipped: `--check --json` with no `--out` used to exit 0 + * with a payload, having been asked for a comparison it could not make; + * - a plain `--json` run with no `--check` is pinned unchanged, so the + * repair cannot be satisfied by turning the machine face into a checker. + * + * ⛔ Nothing here asserts a `drift` / `missing` / `stale` MEMBER on the + * payload. The repair routes drift through this command's existing + * `{ error, …errorCodeFields }` envelope — the one every other failure of this + * command already speaks — and adds no new member to a published output face. + * Naming the drifted files in the machine payload is a widening, and a + * widening is its own card; a case pinning one here would settle that contract + * by test instead. + * + * ## Why this file is not named `.e2e` + * + * The `.e2e` filename tier runs NIGHTLY on `main` and not on a pull request or + * in the merge queue (`scripts/nightly-tiers.mjs`). That is the wrong trade for + * this card's class, for the reason its `--dry-run` sibling records: the + * regression reads GREEN, so between reintroduction and the next nightly every + * run of the pair reports success about a comparison that is not happening, and + * PRs merge on top of it. Its PROJECT is still decided by what it does — it + * spawns the CLI, so `vitest-tiers.ts` classifies it `integration` either way. + * + * ## Fixture placement + * + * The stack config goes under this package's git-ignored `tmp/` and the `--out` + * roots in the system temp dir, for the reason `i18n-extract-check-hint.e2e` + * records: `bundle-require` writes its bundled module next to the config, so + * Node resolves the bare `@objectstack/spec` specifier from THAT directory, and + * only under `packages/cli/tmp/` does that lookup reach this package's real + * `node_modules`. `afterAll` removes only this suite's own `mkdtemp` + * directories — several suites share that root and run concurrently. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { childEnv } from './helpers/serve-process.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); +const CLI_PACKAGE_ROOT = resolve(HERE, '..'); + +/** One object, and a `defaultLocale` equal to the only locale asked for. */ +const STACK_CONFIG = [ + "import { defineStack } from '@objectstack/spec';", + '', + 'export default defineStack({', + " i18n: { defaultLocale: 'zh-CN', supportedLocales: ['zh-CN'] },", + " objects: [{ name: 'kpi_metric', label: 'Metric', fields: { name: { type: 'text', label: 'Name' } } }],", + '});', + '', +].join('\n'); + +/** The one bundle this invocation commits. `--no-metadata-forms` keeps it at one. */ +const BUNDLE = 'zh-CN.objects.generated.ts'; +const FLAGS = ['--locales=zh-CN', '--no-metadata-forms']; + +/** The first line of the sentence both faces end a drifted `--check` on. */ +const DRIFTED = 'Translation bundles have drifted from the schema.'; +/** The refusal a `--check` with no `--out` ends on, on both faces. */ +const NEEDS_OUT = '--check needs --out='; + +let fixtureRoot: string; +let outRoot: string; +let CONFIG: string; +/** An `--out` whose committed bundle is in sync — written once, copied per case. */ +let syncedOut: string; + +/** Text with SGR sequences removed — chalk is off through a pipe, belt and braces. */ +function plain(text: string): string { + // The escape byte is SPELLED, never embedded: a raw control byte in a source + // file renders as nothing and is findable by neither spelling. + return text.replace(/\u001b\[[0-9;]*m/g, ''); +} + +interface Run { + /** stdout ALONE — the machine channel, kept apart so a document can be parsed from it. */ + stdout: string; + /** stdout and stderr together, for the console face's own lines. */ + output: string; + status: number | null; +} + +/** The CLI, from source, with `--check`'s non-zero exit treated as data. */ +function runCli(args: readonly string[]): Run { + const child = spawnSync(TSX, [CLI, 'i18n', 'extract', ...args], { + cwd: CLI_PACKAGE_ROOT, + encoding: 'utf8', + env: childEnv(), + timeout: 180_000, + }); + const stdout = plain(child.stdout ?? ''); + return { stdout, output: `${stdout}${plain(child.stderr ?? '')}`, status: child.status }; +} + +/** + * What the MACHINE face said, in one comparable reading. + * + * `documents` is the count stdout parses into — 1 for a well-formed run, and + * the reading that catches the payload-then-error-envelope shape, which is + * unparseable as one document and would otherwise look like a repair. + */ +function jsonVerdict(run: Run): { + status: number | null; + documents: number; + /** The `error` sentence's first line, or `null` when the run carried no envelope. */ + error: string | null; + /** Whether the ordinary extract payload was emitted (its `bundles` member). */ + payload: boolean; +} { + let parsed: unknown; + let documents = 0; + try { + parsed = JSON.parse(run.stdout); + documents = 1; + } catch { + // Anything that is not exactly one document — none, or two concatenated. + documents = run.stdout.trim() === '' ? 0 : 2; + } + const doc = (parsed ?? {}) as { error?: unknown; bundles?: unknown }; + return { + status: run.status, + documents, + error: typeof doc.error === 'string' ? (doc.error.split('\n')[0] as string) : null, + payload: typeof doc.bundles === 'object' && doc.bundles !== null, + }; +} + +/** What the CONSOLE face said — the positive control's own reading. */ +function consoleVerdict(run: Run): { + status: number | null; + drift: string[]; + drifted: boolean; + inSync: boolean; +} { + return { + status: run.status, + drift: [...run.output.matchAll(/(missing:|out of date:)\s+(\S+)/g)].map((m) => `${m[1]} ${m[2]}`), + drifted: run.output.includes(DRIFTED), + inSync: run.output.includes('in sync with the schema'), + }; +} + +/** A private `--out` for one case, optionally seeded from the in-sync tree. */ +function outDir(name: string, seeded = false): string { + const dir = join(outRoot, name); + if (seeded) cpSync(syncedOut, dir, { recursive: true }); + else mkdirSync(dir, { recursive: true }); + return dir; +} + +beforeAll(() => { + const sharedRoot = join(CLI_PACKAGE_ROOT, 'tmp'); + mkdirSync(sharedRoot, { recursive: true }); + fixtureRoot = mkdtempSync(join(sharedRoot, 'os-i18n-16600-fixture-')); + CONFIG = join(fixtureRoot, 'objectstack.config.ts'); + writeFileSync(CONFIG, STACK_CONFIG, 'utf8'); + outRoot = mkdtempSync(join(tmpdir(), 'os-i18n-16600-')); + + // A real extract, so the "in sync" tree is what the command itself writes + // rather than bytes this file predicted. + syncedOut = join(outRoot, 'synced'); + const wrote = runCli([CONFIG, ...FLAGS, `--out=${syncedOut}`]); + expect({ status: wrote.status, files: readdirSync(syncedOut) }).toEqual({ status: 0, files: [BUNDLE] }); +}, 300_000); + +afterAll(() => { + // This suite's own directories only. Never the shared `tmp/` root. + rmSync(fixtureRoot, { recursive: true, force: true }); + rmSync(outRoot, { recursive: true, force: true }); +}); + +describe('os i18n extract --check --json — compares, and reports what it found (#16600)', () => { + /** + * The card's own table: two invocations differing only by `--json` must reach + * the same exit code, and the `--json` one must say it found DRIFT rather + * than merely failing. + * + * Falsifier: restoring the unconditional `return` in the `--json` branch + * gives `{ status: 0, documents: 1, error: null, payload: true }` against the + * control's exit 1 and reported `missing:`. + */ + it('reports nothing committed with the same exit code as the same run without --json', () => { + const out = outDir('missing'); + const args = [CONFIG, ...FLAGS, `--out=${out}`, '--check']; + + const control = runCli(args); + const json = runCli([...args, '--json']); + + // The positive control the card supplies: the drift is really there. + expect(consoleVerdict(control)).toEqual({ + status: 1, + drift: [`missing: ${join(out, BUNDLE)}`], + drifted: true, + inSync: false, + }); + // The reading under test — spelled out as well as compared, so two runs + // that BOTH compare nothing cannot satisfy this by agreeing with each other. + expect(jsonVerdict(json)).toEqual({ + status: 1, + documents: 1, + error: DRIFTED + ' Regenerate and commit:', + payload: false, + }); + // The card's acceptance, stated as the equality it is. + expect(json.status).toBe(control.status); + // …while writing nothing, which is the half `--check` contributes. + expect(readdirSync(out)).toEqual([]); + }); + + /** + * The second drift shape, and the stronger "writes nothing": a committed + * bundle that is out of date is REPORTED and left byte-for-byte alone. + */ + it('reports a stale committed bundle without rewriting it', () => { + const out = outDir('stale', true); + const bundle = join(out, BUNDLE); + const stale = `${readFileSync(bundle, 'utf8')}\n// edited by hand\n`; + writeFileSync(bundle, stale, 'utf8'); + + // Spelled out rather than compared against a second control run: the + // control-vs-json equality is the case above, and naming the expected + // report is the stronger half of it anyway. One spawn saved. + expect(jsonVerdict(runCli([CONFIG, ...FLAGS, `--out=${out}`, '--check', '--json']))).toEqual({ + status: 1, + documents: 1, + error: DRIFTED + ' Regenerate and commit:', + payload: false, + }); + expect(readFileSync(bundle, 'utf8')).toBe(stale); + }); + + /** + * The direction no unconditional failure can pass: an in-sync tree exits 0 on + * both faces and the machine one still emits its ordinary payload. + * + * ⚠️ Green on BOTH sides of this card's mutation, deliberately — see the file + * header. Its job is to stop "always fail under `--check --json`" from + * satisfying every other case here, not to detect the defect. + */ + it('passes an in-sync tree on both faces, and still emits the payload', () => { + const out = outDir('in-sync', true); + const args = [CONFIG, ...FLAGS, `--out=${out}`, '--check']; + + const control = runCli(args); + const json = runCli([...args, '--json']); + + expect(consoleVerdict(control)).toEqual({ status: 0, drift: [], drifted: false, inSync: true }); + expect(jsonVerdict(json)).toEqual({ status: 0, documents: 1, error: null, payload: true }); + expect(json.status).toBe(control.status); + expect(readdirSync(out)).toEqual([BUNDLE]); + }); + + /** + * The other thing the early return skipped. `--check` with no `--out` has + * nothing to compare against, and the console face has always refused it; + * under `--json` the refusal was unreachable, so the run exited 0 with a + * payload having been asked for a comparison it could not make. + */ + it('refuses --check with no --out on the machine face too', () => { + const json = runCli([CONFIG, ...FLAGS, '--check', '--json']); + + expect(json.status).toBe(1); + const verdict = jsonVerdict(json); + expect({ documents: verdict.documents, payload: verdict.payload }).toEqual({ documents: 1, payload: false }); + expect(verdict.error).toContain(NEEDS_OUT); + }); + + /** + * The repair may not be satisfied by turning the machine face into a checker: + * a `--json` run that did not ask for `--check` still emits the payload and + * exits 0, on the very tree the case above fails on. + */ + it('leaves a --json run that did not ask for --check alone', () => { + const out = outDir('no-check'); + const json = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--json']); + + expect(jsonVerdict(json)).toEqual({ status: 0, documents: 1, error: null, payload: true }); + // `--json` is "output JSON instead of writing files", so the directory the + // drift cases found empty is still empty. + expect(readdirSync(out)).toEqual([]); + }); + // Every case spawns the CLI through `tsx`; measured at ~4 s per run on a + // shared box, over vitest's 5 s default once a case spawns twice. Same + // instrument and the same generous ceiling as the sibling CLI-spawning pins + // in this directory. +}, 900_000); From cba32a42b9f201024be7528d79c6c588db2872fc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 18:03:16 +0000 Subject: [PATCH 2/4] docs(cli): correct the distance the `--json` note gives for the catch block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note said the `{ error, …errorCodeFields }` envelope sits "twenty lines down"; it is at the end of the method, ~200 lines below. Comment only. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- packages/cli/src/commands/i18n/extract.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/i18n/extract.ts b/packages/cli/src/commands/i18n/extract.ts index 94597785a8..abcb73a72b 100644 --- a/packages/cli/src/commands/i18n/extract.ts +++ b/packages/cli/src/commands/i18n/extract.ts @@ -539,8 +539,8 @@ export default class I18nExtract extends Command { * * ⛔ The drift report is deliberately NOT widened into the published * payload — no `drift` / `missing` / `stale` member is added here. This - * command already has exactly one machine-readable failure envelope, - * twenty lines down in the `catch`: `{ error, …errorCodeFields }`, + * command already has exactly one machine-readable failure envelope — + * the `catch` at the end of this method: `{ error, …errorCodeFields }`, * compact, exit 1. Every other way this command can fail already speaks * it, the `--check` needs-`--out` refusal above included, so routing * drift through the same `throw` is copying the convention rather than From 75fc1c3423f810a61959b8ea2ff4d3620f18dcf1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 18:15:04 +0000 Subject: [PATCH 3/4] =?UTF-8?q?chore(changeset):=20grade=20`@objectstack/c?= =?UTF-8?q?li`=20minor=20under=20the=20clause-=E2=91=A1=20ruling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PM ruled clause-② `yes` on card #16600: copying this command's existing `{ error, ... }` envelope onto a path that could not reach it is the widening branch triage enumerated. `check-changeset-no-major`'s way 1 then applies — the declaration is right and the level was wrong. Records why no BREAKING banner rides with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- .changeset/i18n-extract-check-json-compares.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/i18n-extract-check-json-compares.md b/.changeset/i18n-extract-check-json-compares.md index 52a6e3d7de..067c1d48e5 100644 --- a/.changeset/i18n-extract-check-json-compares.md +++ b/.changeset/i18n-extract-check-json-compares.md @@ -1,5 +1,5 @@ --- -"@objectstack/cli": patch +"@objectstack/cli": minor --- `os i18n extract --check --json` now COMPARES. It used to exit 0 having compared nothing, on a tree whose bundles had provably drifted. @@ -16,3 +16,5 @@ What each invocation now does, with no new member on any published payload: - **`--json` without `--check`** — unchanged in every respect. The run leaves through exactly one of those faces, so stdout still parses as exactly one JSON document. + +Graded `minor` rather than `patch` because the PM's clause-② ruling on this card reads the reuse of the existing envelope on a newly reachable path as a widening of the published output face; the maintainer's 2026-09-04 rule puts an already-declared widening at `minor` at least. Nothing an author can write is removed, renamed or narrowed here, and the failure prescribes no consumer code change — so no `**BREAKING**` banner and no ADR-0087 disposition ride along. From 3fd7a6b0a6ad2f28342e222df3b2cfb20af66a10 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 18:35:23 +0000 Subject: [PATCH 4/4] fix(cli): the drifted `--check` remedy drops `--json`, which regenerates nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rerunThatRegenerates` (was `rerunWithoutCheck`) stripped only `--check`, so on the machine face the command it named kept `--json` — "output JSON instead of writing files". Running exactly what the failure printed emitted a payload, wrote zero files, and left the next `--check --json` failing with the same advice: the #14895 loop reproduced on the face this branch creates. Measured end to end on the card's fixture; after the change the printed command writes the bundle and the following `--check --json` exits 0. The degraded fallback line names the same two tokens under `--json`, so the built command and the fallback cannot prescribe different things. The pin reads the remedy as well as the sentence: the drift envelope is two lines and the second one is where this face can go wrong on its own, which is why reading only line one let this through. `remedyNamesOut` / `remedyCarriesJson` are asserted on every machine case. Changeset back to `patch` under the at-tier `Clause-②: no` ruling, and its remedy sentence amended to name both flags. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --- .../i18n-extract-check-json-compares.md | 6 +- packages/cli/src/commands/i18n/extract.ts | 65 ++++++++++++++----- .../cli/test/i18n-extract-check-json.test.ts | 52 ++++++++++++++- 3 files changed, 99 insertions(+), 24 deletions(-) diff --git a/.changeset/i18n-extract-check-json-compares.md b/.changeset/i18n-extract-check-json-compares.md index 067c1d48e5..880ec0c6ea 100644 --- a/.changeset/i18n-extract-check-json-compares.md +++ b/.changeset/i18n-extract-check-json-compares.md @@ -1,12 +1,12 @@ --- -"@objectstack/cli": minor +"@objectstack/cli": patch --- `os i18n extract --check --json` now COMPARES. It used to exit 0 having compared nothing, on a tree whose bundles had provably drifted. The machine face returned before the comparison ran: `if (flags.json) { … return; }` sat ahead of both the `--check` needs-`--out` guard and the comparison block. Driven on one fixture, two invocations differing only by `--json` — the first exited 1 with `missing: OUT/zh-CN.objects.generated.ts` and `Translation bundles have drifted from the schema`, the second exited 0 with the ordinary extract payload. The first run is the second one's positive control: the drift was really there. Same shape as the `--dry-run` branch repaired one release earlier, and `--json` is if anything the more likely CI spelling of the two, because a pipeline that wants to parse the result reaches for it. -⚠️ **A pipeline that runs `os i18n extract … --check --json` and was green may now go red, and that is this repair working.** The green was a comparison that never happened; the red is the drift that was already in the tree. The fix is the one the failure names — re-run the same command without `--check` and commit what it writes. +⚠️ **A pipeline that runs `os i18n extract … --check --json` and was green may now go red, and that is this repair working.** The green was a comparison that never happened; the red is the drift that was already in the tree. The fix is the one the failure names — re-run the same command without `--check` **and without `--json`**, then commit what it writes. Neither of those two flags writes files, and the command the failure prints now has both taken out of it. What each invocation now does, with no new member on any published payload: @@ -17,4 +17,4 @@ What each invocation now does, with no new member on any published payload: The run leaves through exactly one of those faces, so stdout still parses as exactly one JSON document. -Graded `minor` rather than `patch` because the PM's clause-② ruling on this card reads the reuse of the existing envelope on a newly reachable path as a widening of the published output face; the maintainer's 2026-09-04 rule puts an already-declared widening at `minor` at least. Nothing an author can write is removed, renamed or narrowed here, and the failure prescribes no consumer code change — so no `**BREAKING**` banner and no ADR-0087 disposition ride along. +One more thing moved with it: the command a drifted `--check` prints as its remedy now has `--json` taken out of it as well as `--check`. It used to keep `--json`, so the machine face named a command that emits a payload, writes zero files, and leaves the next run failing with the same advice. diff --git a/packages/cli/src/commands/i18n/extract.ts b/packages/cli/src/commands/i18n/extract.ts index abcb73a72b..997160d6b2 100644 --- a/packages/cli/src/commands/i18n/extract.ts +++ b/packages/cli/src/commands/i18n/extract.ts @@ -104,40 +104,60 @@ function shellToken(token: string): string { * An assembled command is wrong in exactly one way and it is unbounded — every * flag that exists now, and every flag added later, has to be remembered at * this print site or it silently goes missing. So this does not enumerate - * flags at all. It takes the argv oclif was handed and removes one token from - * it, which makes the echo correct for flags this file has never heard of. + * flags at all. It takes the argv oclif was handed and removes the tokens that + * make a run WRITE NOTHING, which keeps the echo correct for flags this file + * has never heard of. * - * ⛔ It also never GUESSES. If `--check` is not in the argv the flag was not - * spelled there, this function cannot point at what it removed, and the caller - * prints "re-run the same command without `--check`" instead — the degraded - * line the report itself asked for, on the grounds that a correct vague - * sentence beats a complete-looking wrong command. Today's flag surface has no - * other way to set `--check` (no `env`, no default, no `allowNo`), so that is - * defence rather than a path a user can reach; it is what keeps "assemble an + * ## Which tokens, and why it is not just `--check` (#16600) + * + * There are exactly two, and both are "write nothing" spellings: + * + * - `--check` — the mode being escaped. Removing it is the whole point. + * - `--json` — "output JSON instead of writing files", so a run carrying it + * regenerates nothing either. It became reachable here the moment the + * machine face started reporting drift, and until it was dropped this + * function named a command that emits a payload, writes zero files, and + * leaves the next `--check --json` failing with the same advice: the + * #14895 loop above, reproduced one face over. A remedy that cannot heal + * the failure it is printed under is worse than none, because it looks + * like one. + * + * ⛔ It never GUESSES. If `--check` is not in the argv the flag was not spelled + * there, this function cannot point at what it removed, and the caller prints a + * degraded sentence instead — on the grounds that a correct vague sentence + * beats a complete-looking wrong command. `--json`'s absence is NOT such a + * signal: it is dropped when present and its absence means only that the run + * was on the console face. Today's flag surface has no other way to set + * `--check` (no `env`, no default, no `allowNo`), so the guard is defence + * rather than a path a user can reach; it is what keeps "assemble an * approximation" from ever becoming the fallback. * * `--` is honoured because it changes what a token MEANS: after it, `--check` * is a positional argument and removing it would rewrite the invocation rather - * than trim it. + * than trim it. The same holds for `--json`. * * @param bin `config.bin` — `os`, the name the command is installed under * @param id `this.id` — `i18n:extract`, oclif's colon spelling of the path * @param argv `this.argv` — the arguments as typed, the command id stripped * @returns the command to print, or `undefined` when it cannot be built */ -function rerunWithoutCheck(bin: string, id: string | undefined, argv: readonly string[]): string | undefined { +function rerunThatRegenerates(bin: string, id: string | undefined, argv: readonly string[]): string | undefined { const kept: string[] = []; - let dropped = 0; + let droppedCheck = 0; let afterTerminator = false; for (const token of argv) { if (!afterTerminator && token === '--') afterTerminator = true; else if (!afterTerminator && (token === '--check' || token.startsWith('--check='))) { - dropped += 1; + droppedCheck += 1; + continue; + } else if (!afterTerminator && (token === '--json' || token.startsWith('--json='))) { + // Dropped without being counted: only `--check`'s absence means "this + // function cannot say what it removed". continue; } kept.push(token); } - if (dropped === 0) return undefined; + if (droppedCheck === 0) return undefined; return [bin, ...(id ?? 'i18n:extract').split(':'), ...kept.map(shellToken)].join(' '); } @@ -494,14 +514,23 @@ export default class I18nExtract extends Command { /** * The sentence a drifted `--check` ends on, built once so both faces end - * on the same words. {@link rerunWithoutCheck} says why the command it - * names is spelled as a deletion and what the degraded line is for. + * on the same words. {@link rerunThatRegenerates} says which tokens the + * command it names has had deleted and why it is spelled as a deletion. + * + * ⭐ The degraded line names the SAME tokens the built command would have + * removed, so the two spellings of this advice cannot prescribe different + * things: under `--json` a run without `--check` still writes nothing, and + * a fallback that said only "without `--check`" would send an operator + * round the #14895 loop exactly as a built command carrying `--json` did. */ const driftMessage = (): string => { - const rerun = rerunWithoutCheck(this.config.bin, this.id, this.argv); + const rerun = rerunThatRegenerates(this.config.bin, this.id, this.argv); + const degraded = flags.json + ? ' re-run the same command without `--check` and without `--json` — neither of them writes files' + : ' re-run the same command without `--check`'; return ( 'Translation bundles have drifted from the schema. Regenerate and commit:\n' + - (rerun ? ` ${rerun}` : ' re-run the same command without `--check`') + (rerun ? ` ${rerun}` : degraded) ); }; diff --git a/packages/cli/test/i18n-extract-check-json.test.ts b/packages/cli/test/i18n-extract-check-json.test.ts index 9914d825a4..57a2fc965b 100644 --- a/packages/cli/test/i18n-extract-check-json.test.ts +++ b/packages/cli/test/i18n-extract-check-json.test.ts @@ -51,6 +51,14 @@ * then an error envelope would pass an "exit 1 and the word drifted" * reading while producing output that is neither one document nor JSONL — * the two-document defect `isExitSignal` records in `utils/format.ts`; + * - the REMEDY is pinned as well as the sentence, on its own two booleans. + * The drift envelope is two lines and the second one is a command; an + * earlier revision of this file read only the first, and a remedy that + * named a `--json` run — which emits a payload and writes zero files — + * sat green under it. Running exactly what the failure prints then heals + * nothing and the next `--check --json` fails identically: the #14895 + * loop, one face over. So the remedy must name an `--out` and must not + * carry `--json`; * - the needs-`--out` refusal is pinned because it is the OTHER thing the * early return skipped: `--check --json` with no `--out` used to exit 0 * with a payload, having been asked for a comparison it could not make; @@ -158,12 +166,26 @@ function runCli(args: readonly string[]): Run { * `documents` is the count stdout parses into — 1 for a well-formed run, and * the reading that catches the payload-then-error-envelope shape, which is * unparseable as one document and would otherwise look like a repair. + * + * ⭐ The drift envelope is TWO lines — a sentence and the command that heals + * it — and both are read, because the second one is where this face can go + * wrong on its own. Reading only the first line is what let a remedy naming a + * `--json` run (which emits a payload and writes zero files) sit green: an + * operator or CI log reader who runs exactly what the failure prints gets + * nothing written and the identical failure next time. That is the #14895 + * loop — "the failure is self-healable and the advice is what stops it + * healing" — so the remedy's two load-bearing properties are pinned as their + * own booleans rather than left to a substring check on line one. */ function jsonVerdict(run: Run): { status: number | null; documents: number; /** The `error` sentence's first line, or `null` when the run carried no envelope. */ error: string | null; + /** Whether the remedy line names an `--out`, i.e. whether it writes anywhere. */ + remedyNamesOut: boolean; + /** Whether the remedy line still carries `--json`, which writes nothing. */ + remedyCarriesJson: boolean; /** Whether the ordinary extract payload was emitted (its `bundles` member). */ payload: boolean; } { @@ -177,10 +199,14 @@ function jsonVerdict(run: Run): { documents = run.stdout.trim() === '' ? 0 : 2; } const doc = (parsed ?? {}) as { error?: unknown; bundles?: unknown }; + const lines = typeof doc.error === 'string' ? doc.error.split('\n') : []; + const remedy = lines[1] ?? ''; return { status: run.status, documents, - error: typeof doc.error === 'string' ? (doc.error.split('\n')[0] as string) : null, + error: lines.length > 0 ? (lines[0] as string) : null, + remedyNamesOut: remedy.includes('--out='), + remedyCarriesJson: remedy.includes('--json'), payload: typeof doc.bundles === 'object' && doc.bundles !== null, }; } @@ -259,6 +285,10 @@ describe('os i18n extract --check --json — compares, and reports what it found status: 1, documents: 1, error: DRIFTED + ' Regenerate and commit:', + // The remedy has to be a command that actually regenerates: it keeps the + // `--out` it was given and sheds the `--json` that writes nothing. + remedyNamesOut: true, + remedyCarriesJson: false, payload: false, }); // The card's acceptance, stated as the equality it is. @@ -284,6 +314,8 @@ describe('os i18n extract --check --json — compares, and reports what it found status: 1, documents: 1, error: DRIFTED + ' Regenerate and commit:', + remedyNamesOut: true, + remedyCarriesJson: false, payload: false, }); expect(readFileSync(bundle, 'utf8')).toBe(stale); @@ -305,7 +337,14 @@ describe('os i18n extract --check --json — compares, and reports what it found const json = runCli([...args, '--json']); expect(consoleVerdict(control)).toEqual({ status: 0, drift: [], drifted: false, inSync: true }); - expect(jsonVerdict(json)).toEqual({ status: 0, documents: 1, error: null, payload: true }); + expect(jsonVerdict(json)).toEqual({ + status: 0, + documents: 1, + error: null, + remedyNamesOut: false, + remedyCarriesJson: false, + payload: true, + }); expect(json.status).toBe(control.status); expect(readdirSync(out)).toEqual([BUNDLE]); }); @@ -334,7 +373,14 @@ describe('os i18n extract --check --json — compares, and reports what it found const out = outDir('no-check'); const json = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--json']); - expect(jsonVerdict(json)).toEqual({ status: 0, documents: 1, error: null, payload: true }); + expect(jsonVerdict(json)).toEqual({ + status: 0, + documents: 1, + error: null, + remedyNamesOut: false, + remedyCarriesJson: false, + payload: true, + }); // `--json` is "output JSON instead of writing files", so the directory the // drift cases found empty is still empty. expect(readdirSync(out)).toEqual([]);