fix(cli): i18n extract --check --json compares instead of exiting 0

.changeset/i18n-extract-check-json-compares.md

@@ -0,0 +1,20 @@
+---
+"@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 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:
+
+- **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.
+
+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.

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=<dir> — 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.
@@ -95,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.
+ *
+ * ## 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 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
+ * ⛔ 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(' ');
 }
 
@@ -431,7 +460,135 @@ 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 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 = 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}` : degraded)
+        );
+      };
+
       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 —
+         * 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
+         * 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 +683,7 @@ export default class I18nExtract extends Command {
       console.log('');
 
       if (flags.check && !flags.out) {
-        throw new Error('--check needs --out=<dir> — it compares a fresh extract against the bundles committed there.');
+        throw new Error(CHECK_NEEDS_OUT);
       }
 
       /**
@@ -579,39 +736,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 +751,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);
       }