fix(cli): os i18n extract --check --dry-run compares instead of exiting 0 having compared nothing

.changeset/i18n-extract-check-dry-run-compares.md

@@ -0,0 +1,12 @@
+---
+"@objectstack/cli": patch
+---
+
+`os i18n extract --check --dry-run` now compares the bundles and reports what it found, instead of exiting 0 having compared nothing.
+
+`--check` and `--dry-run` are both "write nothing" modes, so the pair reads as the safest spelling to put in CI — and it was the one spelling that measured nothing. The `--dry-run` branch returned before the `--check` block was reached, so the same tree that failed `--check` with `Translation bundles have drifted from the schema` reported success as soon as `--dry-run` was added to the command line. A check that cannot fail is indistinguishable from a check that finds nothing: the pipeline went green and nobody learned the bundles had drifted.
+
+⚠️ **A pipeline running `--check --dry-run` against drifted bundles starts failing on this release, and that is the repair working.** The failure is not new — the drift it names was already there and the old exit code was wrong about it. The fix is the one `--check` has always printed: regenerate the bundles and commit them. Nothing else about the pair changes, and a tree that is in sync still exits 0, now with the `bundle(s) are in sync with the schema` line it never printed under `--dry-run` before.
+
+- **What each flag contributes is unchanged.** `--dry-run` still prints the rendered modules to stdout, `--check` still compares them against what is committed in `--out`, and neither writes a file — on any path, including a bundle that is present but out of date, which keeps its bytes.
+- **The `--out` advice no longer contradicts the command line it is printed on.** `Dry run — no files written (pass --out=<dir> to write)` was printed even to runs that had just passed `--out`, which reads as "your directory was ignored" when it had not been. A run with an `--out` now names the directory it did not write to; a run without one still gets the advice.

packages/cli/src/commands/i18n/extract.ts

@@ -529,18 +529,54 @@ export default class I18nExtract extends Command {
         throw new Error('--check needs --out=<dir> — it compares a fresh extract against the bundles committed there.');
       }
 
+      /**
+       * The stdout dump `--dry-run` asks for — and, when `--check` is also on,
+       * NOT a place this run may leave from (#16480).
+       *
+       * `--dry-run` and `--check` are both "write nothing" modes, so the pair
+       * is not a contradiction: one says print the modules instead of writing
+       * them, the other says compare them against what is committed. Neither
+       * cancels the other, and an operator reaching for both in CI is reaching
+       * for the spelling that looks safest.
+       *
+       * This branch used to `return` unconditionally, which put it AHEAD of the
+       * `--check` block: `--check --dry-run --out=DIR` printed the dump and
+       * exited 0 on a tree the very same invocation without `--dry-run` failed
+       * on with `Translation bundles have drifted from the schema`. That is the
+       * dangerous direction of an ignored flag — not bad advice on a real
+       * failure, but a green tick over a comparison that never ran, and a check
+       * that cannot fail is indistinguishable from a check that finds nothing.
+       *
+       * ⛔ So the return is conditional on `--check` being OFF, and exiting 0
+       * without comparing must not come back. When `--check` is on, execution
+       * falls through to the comparison below; the write loop past it is still
+       * unreachable, because `--check` either returns in sync or exits 1.
+       */
       if (flags['dry-run'] || !flags.out) {
         for (const locale of localesEmitted) {
           for (const mod of emittedModules(locale)) {
             console.log(chalk.dim(`── ${locale} (${mod.label}) ──`));
             console.log(renderTranslationModule(result.bundles[locale], { locale, kind: mod.kind }));
           }
         }
-        printInfo('Dry run — no files written (pass --out=<dir> to write).');
-        return;
+        if (!flags.check) {
+          // The advice is for the run that HAS no `--out`. Printed
+          // unconditionally, it told an operator who had just passed `--out` to
+          // pass `--out`, which reads as "your directory was ignored" — and it
+          // was not (#16480). With one, name it: that is the same reading in the
+          // direction that is true.
+          printInfo(
+            outDir
+              ? `Dry run — no files written to ${chalk.white(displayPath(outDir))}.`
+              : 'Dry run — no files written (pass --out=<dir> to write).',
+          );
+          return;
+        }
       }
 
-      // `flags.out` is non-empty here — the two branches above return otherwise.
+      // `flags.out` is non-empty here. Of the branches above, the `--json` one
+      // and the `--dry-run` one return; the `--dry-run` one falls through only
+      // 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.

packages/cli/test/i18n-extract-check-dry-run.test.ts

@@ -0,0 +1,267 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `os i18n extract --check --dry-run` COMPARES, and says what it found (#16480).
+ *
+ * ## What was wrong
+ *
+ * The two flags are both "write nothing" modes, so the pair reads as the safest
+ * spelling to put in CI. It was the one spelling that measured nothing. Driven
+ * on one drifted fixture, the two invocations differing ONLY by `--dry-run`:
+ *
+ *     $ 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 --dry-run
+ *       Dry run — no files written (pass --out=<dir> to write).
+ *     -> exit 0, with no `missing:` / `out of date:` / in-sync line at all
+ *
+ * The first run is the second one's positive control: the drift is provably
+ * there, and the second reported success. The `--dry-run` branch returned
+ * before the `--check` block was reached, so nothing was compared — and a
+ * check that cannot fail is indistinguishable from a check that finds nothing.
+ * Unlike an ignored flag that produces bad advice on a real failure, this
+ * direction is silent: the pipeline goes green and nobody learns the bundles
+ * have drifted.
+ *
+ * ## 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 case here pins the
+ * REPORTED DRIFT beside the code, and the suite pins the comparison in both
+ * directions:
+ *
+ *   - the drifted cases are stated as an EQUALITY against the same invocation
+ *     WITHOUT `--dry-run`, which is the card's own method rather than a
+ *     re-derivation of it. The expected values are spelled out too, 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, and it is
+ *     asserted as `exit 0` AND the in-sync sentence — the same "code plus
+ *     report" rule, in the direction where the code is the passing one;
+ *   - "writing nothing" is measured from the filesystem on both drift shapes:
+ *     an empty `--out` stays empty, and a committed-but-stale bundle keeps its
+ *     bytes.
+ *
+ * ## 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`), which is the right trade
+ * for most of this package's CLI-spawning suites. It is the wrong one for this
+ * card's class: the regression it pins 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. So this file
+ * takes the queue run by carrying no tier in its name. Its PROJECT is still
+ * decided by what it does, not by what it is called — it spawns the CLI, so
+ * `vitest-tiers.ts` classifies it `integration` either way. Cost, MEASURED on
+ * the (shared, contended) box this landed on rather than estimated: 7 CLI
+ * spawns, ~11s each, 77s for the file. That is the price of the trade and it
+ * is written here so the trade can be re-made against a number: renaming this
+ * file to `.e2e` moves it to the nightly and costs nothing else.
+ *
+ * ## 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'];
+
+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;
+
+/** stdout 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: 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,
+  });
+  return { stdout: plain(`${child.stdout ?? ''}${child.stderr ?? ''}`), status: child.status };
+}
+
+/** The `missing:` / `out of date:` paths `--check` reported, verb included, in order. */
+function driftReport(stdout: string): string[] {
+  return [...stdout.matchAll(/(missing:|out of date:)\s+(\S+)/g)].map((m) => `${m[1]} ${m[2]}`);
+}
+
+/** Exit code, reported drift and the summary sentence — one comparable reading. */
+function verdict(run: Run): { status: number | null; drift: string[]; drifted: boolean; inSync: boolean } {
+  return {
+    status: run.status,
+    drift: driftReport(run.stdout),
+    drifted: run.stdout.includes('Translation bundles have drifted from the schema'),
+    inSync: run.stdout.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-16480-fixture-'));
+  CONFIG = join(fixtureRoot, 'objectstack.config.ts');
+  writeFileSync(CONFIG, STACK_CONFIG, 'utf8');
+  outRoot = mkdtempSync(join(tmpdir(), 'os-i18n-16480-'));
+
+  // 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 --dry-run — compares, and reports what it found (#16480)', () => {
+  /**
+   * The card's own table: two invocations differing only by `--dry-run` must
+   * reach the same verdict, and that verdict is a REPORTED drift.
+   *
+   * Falsifier: restoring the unconditional `return` in the `--dry-run` branch
+   * gives the second run `{ status: 0, drift: [], drifted: false }` against the
+   * control's `{ status: 1, drift: ['missing: …'], drifted: true }`.
+   */
+  it('reports nothing committed exactly as the same run without --dry-run does', () => {
+    const out = outDir('missing');
+    const args = [CONFIG, ...FLAGS, `--out=${out}`, '--check'];
+
+    const control = runCli(args);
+    const dryRun = runCli([...args, '--dry-run']);
+
+    const expected = {
+      status: 1,
+      drift: [`missing: ${join(out, BUNDLE)}`],
+      drifted: true,
+      inSync: false,
+    };
+    // Spelled out as well as compared, so two runs that BOTH compare nothing
+    // cannot satisfy this by agreeing with each other.
+    expect(verdict(control)).toEqual(expected);
+    expect(verdict(dryRun)).toEqual(expected);
+    // …while writing nothing, which is the half `--dry-run` 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');
+    const dryRun = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--check', '--dry-run']);
+
+    // Spelled out rather than compared against a second control run: the
+    // control-vs-dry-run equality is the case above, and naming the expected
+    // report is the stronger half of it anyway. One spawn saved, ~11s.
+    expect(verdict(dryRun)).toEqual({
+      status: 1,
+      drift: [`out of date: ${bundle}`],
+      drifted: true,
+      inSync: false,
+    });
+    expect(readFileSync(bundle, 'utf8')).toBe(stale);
+  });
+
+  /**
+   * The direction no unconditional failure can pass: an in-sync tree exits 0
+   * AND says so. Without this case, "always fail under --check --dry-run"
+   * would satisfy every other assertion in this file.
+   */
+  it('passes an in-sync tree and says it compared it', () => {
+    const out = outDir('in-sync', true);
+    const run = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--check', '--dry-run']);
+
+    expect(verdict(run)).toEqual({ status: 0, drift: [], drifted: false, inSync: true });
+    expect(readdirSync(out)).toEqual([BUNDLE]);
+  });
+
+  /**
+   * The secondary wrinkle on the same branch: the advice to pass `--out` was
+   * printed to runs that had just passed `--out`, which reads as "your
+   * directory was ignored" — and it was not. The second case is the falsifier
+   * that separates repairing the line from deleting it.
+   */
+  it('names the --out it was given instead of advising the flag that was passed', () => {
+    const out = outDir('wrinkle');
+    const run = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--dry-run']);
+
+    expect(run.status).toBe(0);
+    expect(run.stdout).toContain(`Dry run — no files written to ${out}.`);
+    expect(run.stdout).not.toContain('pass --out=');
+    expect(readdirSync(out)).toEqual([]);
+  });
+
+  it('keeps advising --out on a run that has none', () => {
+    const run = runCli([CONFIG, ...FLAGS, '--dry-run']);
+
+    expect(run.status).toBe(0);
+    expect(run.stdout).toContain('Dry run — no files written (pass --out=<dir> to write).');
+  });
+  // 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);