Skip to content

Commit 309bee5

Browse files
claude[bot]os-salesclaude
authored
fix(cli): os i18n extract --check --dry-run compares instead of exiting 0 having compared nothing (#16627)
* fix(cli): i18n extract --check --dry-run compares instead of exiting 0 `--check --dry-run --out=DIR` exited 0 having compared 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 once `--dry-run` was added. Both flags mean "write nothing", so the pair reads as the safest spelling for CI, and a check that cannot fail is indistinguishable from a check that finds nothing. The dump `--dry-run` asks for still happens; the return out of that branch is now conditional on `--check` being off, so the comparison below runs and reports. Nothing is written on either path. Same branch, same card: the `pass --out=<dir> to write` advice was printed even to runs that had just passed `--out`, which reads as "your directory was ignored". With an `--out` the line now names it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ * docs(changeset): i18n extract --check --dry-run compares Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ --------- Co-authored-by: os-sales <sales@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 0cde37d commit 309bee5

3 files changed

Lines changed: 318 additions & 3 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`os i18n extract --check --dry-run` now compares the bundles and reports what it found, instead of exiting 0 having compared nothing.
6+
7+
`--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.
8+
9+
⚠️ **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.
10+
11+
- **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.
12+
- **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

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -529,18 +529,54 @@ export default class I18nExtract extends Command {
529529
throw new Error('--check needs --out=<dir> — it compares a fresh extract against the bundles committed there.');
530530
}
531531

532+
/**
533+
* The stdout dump `--dry-run` asks for — and, when `--check` is also on,
534+
* NOT a place this run may leave from (#16480).
535+
*
536+
* `--dry-run` and `--check` are both "write nothing" modes, so the pair
537+
* is not a contradiction: one says print the modules instead of writing
538+
* them, the other says compare them against what is committed. Neither
539+
* cancels the other, and an operator reaching for both in CI is reaching
540+
* for the spelling that looks safest.
541+
*
542+
* This branch used to `return` unconditionally, which put it AHEAD of the
543+
* `--check` block: `--check --dry-run --out=DIR` printed the dump and
544+
* exited 0 on a tree the very same invocation without `--dry-run` failed
545+
* on with `Translation bundles have drifted from the schema`. That is the
546+
* dangerous direction of an ignored flag — not bad advice on a real
547+
* failure, but a green tick over a comparison that never ran, and a check
548+
* that cannot fail is indistinguishable from a check that finds nothing.
549+
*
550+
* ⛔ So the return is conditional on `--check` being OFF, and exiting 0
551+
* without comparing must not come back. When `--check` is on, execution
552+
* falls through to the comparison below; the write loop past it is still
553+
* unreachable, because `--check` either returns in sync or exits 1.
554+
*/
532555
if (flags['dry-run'] || !flags.out) {
533556
for (const locale of localesEmitted) {
534557
for (const mod of emittedModules(locale)) {
535558
console.log(chalk.dim(`── ${locale} (${mod.label}) ──`));
536559
console.log(renderTranslationModule(result.bundles[locale], { locale, kind: mod.kind }));
537560
}
538561
}
539-
printInfo('Dry run — no files written (pass --out=<dir> to write).');
540-
return;
562+
if (!flags.check) {
563+
// The advice is for the run that HAS no `--out`. Printed
564+
// unconditionally, it told an operator who had just passed `--out` to
565+
// pass `--out`, which reads as "your directory was ignored" — and it
566+
// was not (#16480). With one, name it: that is the same reading in the
567+
// direction that is true.
568+
printInfo(
569+
outDir
570+
? `Dry run — no files written to ${chalk.white(displayPath(outDir))}.`
571+
: 'Dry run — no files written (pass --out=<dir> to write).',
572+
);
573+
return;
574+
}
541575
}
542576

543-
// `flags.out` is non-empty here — the two branches above return otherwise.
577+
// `flags.out` is non-empty here. Of the branches above, the `--json` one
578+
// and the `--dry-run` one return; the `--dry-run` one falls through only
579+
// under `--check`, and `--check` without `--out` already threw.
544580
const resolvedOutDir = outDir as string;
545581

546582
// Every file a normal run would emit, paired with its rendered content.
Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,267 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `os i18n extract --check --dry-run` COMPARES, and says what it found (#16480).
5+
*
6+
* ## What was wrong
7+
*
8+
* The two flags are both "write nothing" modes, so the pair reads as the safest
9+
* spelling to put in CI. It was the one spelling that measured nothing. Driven
10+
* on one drifted fixture, the two invocations differing ONLY by `--dry-run`:
11+
*
12+
* $ os i18n extract CONFIG --locales=zh-CN --no-metadata-forms --out=OUT --check
13+
* missing: OUT/zh-CN.objects.generated.ts
14+
* Translation bundles have drifted from the schema. Regenerate and commit:
15+
* -> exit 1
16+
*
17+
* $ os i18n extract CONFIG --locales=zh-CN --no-metadata-forms --out=OUT --check --dry-run
18+
* Dry run — no files written (pass --out=<dir> to write).
19+
* -> exit 0, with no `missing:` / `out of date:` / in-sync line at all
20+
*
21+
* The first run is the second one's positive control: the drift is provably
22+
* there, and the second reported success. The `--dry-run` branch returned
23+
* before the `--check` block was reached, so nothing was compared — and a
24+
* check that cannot fail is indistinguishable from a check that finds nothing.
25+
* Unlike an ignored flag that produces bad advice on a real failure, this
26+
* direction is silent: the pipeline goes green and nobody learns the bundles
27+
* have drifted.
28+
*
29+
* ## Why these shapes
30+
*
31+
* ⚠️ A case asserting only the new exit code would be satisfied by a `--check`
32+
* that still compares nothing and merely fails — so every case here pins the
33+
* REPORTED DRIFT beside the code, and the suite pins the comparison in both
34+
* directions:
35+
*
36+
* - the drifted cases are stated as an EQUALITY against the same invocation
37+
* WITHOUT `--dry-run`, which is the card's own method rather than a
38+
* re-derivation of it. The expected values are spelled out too, because an
39+
* equality alone is also satisfied by two runs that are both broken;
40+
* - the in-sync case is what no unconditional failure can pass, and it is
41+
* asserted as `exit 0` AND the in-sync sentence — the same "code plus
42+
* report" rule, in the direction where the code is the passing one;
43+
* - "writing nothing" is measured from the filesystem on both drift shapes:
44+
* an empty `--out` stays empty, and a committed-but-stale bundle keeps its
45+
* bytes.
46+
*
47+
* ## Why this file is not named `.e2e`
48+
*
49+
* The `.e2e` filename tier runs NIGHTLY on `main` and not on a pull request or
50+
* in the merge queue (`scripts/nightly-tiers.mjs`), which is the right trade
51+
* for most of this package's CLI-spawning suites. It is the wrong one for this
52+
* card's class: the regression it pins reads GREEN, so between reintroduction
53+
* and the next nightly every run of the pair reports success about a
54+
* comparison that is not happening, and PRs merge on top of it. So this file
55+
* takes the queue run by carrying no tier in its name. Its PROJECT is still
56+
* decided by what it does, not by what it is called — it spawns the CLI, so
57+
* `vitest-tiers.ts` classifies it `integration` either way. Cost, MEASURED on
58+
* the (shared, contended) box this landed on rather than estimated: 7 CLI
59+
* spawns, ~11s each, 77s for the file. That is the price of the trade and it
60+
* is written here so the trade can be re-made against a number: renaming this
61+
* file to `.e2e` moves it to the nightly and costs nothing else.
62+
*
63+
* ## Fixture placement
64+
*
65+
* The stack config goes under this package's git-ignored `tmp/` and the `--out`
66+
* roots in the system temp dir, for the reason `i18n-extract-check-hint.e2e`
67+
* records: `bundle-require` writes its bundled module next to the config, so
68+
* Node resolves the bare `@objectstack/spec` specifier from THAT directory, and
69+
* only under `packages/cli/tmp/` does that lookup reach this package's real
70+
* `node_modules`. `afterAll` removes only this suite's own `mkdtemp`
71+
* directories — several suites share that root and run concurrently.
72+
*/
73+
74+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
75+
import { spawnSync } from 'node:child_process';
76+
import { cpSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs';
77+
import { tmpdir } from 'node:os';
78+
import { join, resolve } from 'node:path';
79+
import { fileURLToPath } from 'node:url';
80+
import { childEnv } from './helpers/serve-process.js';
81+
82+
const HERE = resolve(fileURLToPath(import.meta.url), '..');
83+
const CLI = resolve(HERE, '../bin/run-dev.js');
84+
const TSX = resolve(HERE, '../../../node_modules/.bin/tsx');
85+
const CLI_PACKAGE_ROOT = resolve(HERE, '..');
86+
87+
/** One object, and a `defaultLocale` equal to the only locale asked for. */
88+
const STACK_CONFIG = [
89+
"import { defineStack } from '@objectstack/spec';",
90+
'',
91+
'export default defineStack({',
92+
" i18n: { defaultLocale: 'zh-CN', supportedLocales: ['zh-CN'] },",
93+
" objects: [{ name: 'kpi_metric', label: 'Metric', fields: { name: { type: 'text', label: 'Name' } } }],",
94+
'});',
95+
'',
96+
].join('\n');
97+
98+
/** The one bundle this invocation commits. `--no-metadata-forms` keeps it at one. */
99+
const BUNDLE = 'zh-CN.objects.generated.ts';
100+
const FLAGS = ['--locales=zh-CN', '--no-metadata-forms'];
101+
102+
let fixtureRoot: string;
103+
let outRoot: string;
104+
let CONFIG: string;
105+
/** An `--out` whose committed bundle is in sync — written once, copied per case. */
106+
let syncedOut: string;
107+
108+
/** stdout with SGR sequences removed — chalk is off through a pipe, belt and braces. */
109+
function plain(text: string): string {
110+
// The escape byte is SPELLED, never embedded: a raw control byte in a source
111+
// file renders as nothing and is findable by neither spelling.
112+
return text.replace(/\u001b\[[0-9;]*m/g, '');
113+
}
114+
115+
interface Run {
116+
stdout: string;
117+
status: number | null;
118+
}
119+
120+
/** The CLI, from source, with `--check`'s non-zero exit treated as data. */
121+
function runCli(args: readonly string[]): Run {
122+
const child = spawnSync(TSX, [CLI, 'i18n', 'extract', ...args], {
123+
cwd: CLI_PACKAGE_ROOT,
124+
encoding: 'utf8',
125+
env: childEnv(),
126+
timeout: 180_000,
127+
});
128+
return { stdout: plain(`${child.stdout ?? ''}${child.stderr ?? ''}`), status: child.status };
129+
}
130+
131+
/** The `missing:` / `out of date:` paths `--check` reported, verb included, in order. */
132+
function driftReport(stdout: string): string[] {
133+
return [...stdout.matchAll(/(missing:|out of date:)\s+(\S+)/g)].map((m) => `${m[1]} ${m[2]}`);
134+
}
135+
136+
/** Exit code, reported drift and the summary sentence — one comparable reading. */
137+
function verdict(run: Run): { status: number | null; drift: string[]; drifted: boolean; inSync: boolean } {
138+
return {
139+
status: run.status,
140+
drift: driftReport(run.stdout),
141+
drifted: run.stdout.includes('Translation bundles have drifted from the schema'),
142+
inSync: run.stdout.includes('in sync with the schema'),
143+
};
144+
}
145+
146+
/** A private `--out` for one case, optionally seeded from the in-sync tree. */
147+
function outDir(name: string, seeded = false): string {
148+
const dir = join(outRoot, name);
149+
if (seeded) cpSync(syncedOut, dir, { recursive: true });
150+
else mkdirSync(dir, { recursive: true });
151+
return dir;
152+
}
153+
154+
beforeAll(() => {
155+
const sharedRoot = join(CLI_PACKAGE_ROOT, 'tmp');
156+
mkdirSync(sharedRoot, { recursive: true });
157+
fixtureRoot = mkdtempSync(join(sharedRoot, 'os-i18n-16480-fixture-'));
158+
CONFIG = join(fixtureRoot, 'objectstack.config.ts');
159+
writeFileSync(CONFIG, STACK_CONFIG, 'utf8');
160+
outRoot = mkdtempSync(join(tmpdir(), 'os-i18n-16480-'));
161+
162+
// A real extract, so the "in sync" tree is what the command itself writes
163+
// rather than bytes this file predicted.
164+
syncedOut = join(outRoot, 'synced');
165+
const wrote = runCli([CONFIG, ...FLAGS, `--out=${syncedOut}`]);
166+
expect({ status: wrote.status, files: readdirSync(syncedOut) }).toEqual({ status: 0, files: [BUNDLE] });
167+
}, 300_000);
168+
169+
afterAll(() => {
170+
// This suite's own directories only. Never the shared `tmp/` root.
171+
rmSync(fixtureRoot, { recursive: true, force: true });
172+
rmSync(outRoot, { recursive: true, force: true });
173+
});
174+
175+
describe('os i18n extract --check --dry-run — compares, and reports what it found (#16480)', () => {
176+
/**
177+
* The card's own table: two invocations differing only by `--dry-run` must
178+
* reach the same verdict, and that verdict is a REPORTED drift.
179+
*
180+
* Falsifier: restoring the unconditional `return` in the `--dry-run` branch
181+
* gives the second run `{ status: 0, drift: [], drifted: false }` against the
182+
* control's `{ status: 1, drift: ['missing: …'], drifted: true }`.
183+
*/
184+
it('reports nothing committed exactly as the same run without --dry-run does', () => {
185+
const out = outDir('missing');
186+
const args = [CONFIG, ...FLAGS, `--out=${out}`, '--check'];
187+
188+
const control = runCli(args);
189+
const dryRun = runCli([...args, '--dry-run']);
190+
191+
const expected = {
192+
status: 1,
193+
drift: [`missing: ${join(out, BUNDLE)}`],
194+
drifted: true,
195+
inSync: false,
196+
};
197+
// Spelled out as well as compared, so two runs that BOTH compare nothing
198+
// cannot satisfy this by agreeing with each other.
199+
expect(verdict(control)).toEqual(expected);
200+
expect(verdict(dryRun)).toEqual(expected);
201+
// …while writing nothing, which is the half `--dry-run` contributes.
202+
expect(readdirSync(out)).toEqual([]);
203+
});
204+
205+
/**
206+
* The second drift shape, and the stronger "writes nothing": a committed
207+
* bundle that is out of date is REPORTED and left byte-for-byte alone.
208+
*/
209+
it('reports a stale committed bundle without rewriting it', () => {
210+
const out = outDir('stale', true);
211+
const bundle = join(out, BUNDLE);
212+
const stale = `${readFileSync(bundle, 'utf8')}\n// edited by hand\n`;
213+
writeFileSync(bundle, stale, 'utf8');
214+
const dryRun = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--check', '--dry-run']);
215+
216+
// Spelled out rather than compared against a second control run: the
217+
// control-vs-dry-run equality is the case above, and naming the expected
218+
// report is the stronger half of it anyway. One spawn saved, ~11s.
219+
expect(verdict(dryRun)).toEqual({
220+
status: 1,
221+
drift: [`out of date: ${bundle}`],
222+
drifted: true,
223+
inSync: false,
224+
});
225+
expect(readFileSync(bundle, 'utf8')).toBe(stale);
226+
});
227+
228+
/**
229+
* The direction no unconditional failure can pass: an in-sync tree exits 0
230+
* AND says so. Without this case, "always fail under --check --dry-run"
231+
* would satisfy every other assertion in this file.
232+
*/
233+
it('passes an in-sync tree and says it compared it', () => {
234+
const out = outDir('in-sync', true);
235+
const run = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--check', '--dry-run']);
236+
237+
expect(verdict(run)).toEqual({ status: 0, drift: [], drifted: false, inSync: true });
238+
expect(readdirSync(out)).toEqual([BUNDLE]);
239+
});
240+
241+
/**
242+
* The secondary wrinkle on the same branch: the advice to pass `--out` was
243+
* printed to runs that had just passed `--out`, which reads as "your
244+
* directory was ignored" — and it was not. The second case is the falsifier
245+
* that separates repairing the line from deleting it.
246+
*/
247+
it('names the --out it was given instead of advising the flag that was passed', () => {
248+
const out = outDir('wrinkle');
249+
const run = runCli([CONFIG, ...FLAGS, `--out=${out}`, '--dry-run']);
250+
251+
expect(run.status).toBe(0);
252+
expect(run.stdout).toContain(`Dry run — no files written to ${out}.`);
253+
expect(run.stdout).not.toContain('pass --out=');
254+
expect(readdirSync(out)).toEqual([]);
255+
});
256+
257+
it('keeps advising --out on a run that has none', () => {
258+
const run = runCli([CONFIG, ...FLAGS, '--dry-run']);
259+
260+
expect(run.status).toBe(0);
261+
expect(run.stdout).toContain('Dry run — no files written (pass --out=<dir> to write).');
262+
});
263+
// Every case spawns the CLI through `tsx`; measured at ~4 s per run on a
264+
// shared box, over vitest's 5 s default once a case spawns twice. Same
265+
// instrument and the same generous ceiling as the sibling CLI-spawning pins
266+
// in this directory.
267+
}, 900_000);

0 commit comments

Comments
 (0)