fix(cli): os migrate --json 成功时不再把耗时毫秒当成退出码 (#4873)

.changeset/migrate-json-exit-code.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/cli": patch
+---
+
+fix(cli): `os migrate --json` no longer exits with its own runtime as the status code (#4873)
+
+A **successful** `os migrate recorded-by --json` returned a different non-zero
+exit code on every invocation — 208, 171, 176, 163, 62, 19, 48, 57 — while
+printing correct JSON, printing `✅ Graceful shutdown complete`, and leaving
+stderr completely empty. `os migrate resume --json` had it too. Nothing that an
+author reads was wrong; the only thing that was wrong is the only thing a CI
+step, a `set -e` script, a Makefile, or a container entrypoint reads. `--json`
+exists for programs, and the first thing a program consumes is the exit status.
+
+**Root cause.** `emitJson(payload, exitCode, opts)` takes its exit code as the
+second positional argument, and both commands were passing `timer.elapsed()`
+there — a duration in milliseconds. So a run that took 531 ms set
+`process.exitCode = 531`, and the shell saw `531 & 0xFF` = 19. The codes looked
+random because they *were* the run's duration, and no two runs take the same
+number of milliseconds.
+
+It was not what it looked like from the outside: no native `abort` during
+teardown, no libsql/sqlite handle, no `safeExit`, and not a leftover of #4813
+(whose 120-second hang is fixed and unrelated — the random codes predate and
+survive it).
+
+**What changed.**
+
+- Both commands now report their duration where every other `--json` command in
+ this CLI already reports it — inside the payload, as `duration`. A successful
+ run exits `0`; a failing one still exits `1`, unchanged.
+- `emitJson` / `emitText` narrow that parameter from `number` to
+ `CliExitCode = 0 | 1`, so handing a duration (or any other stray number) to
+ the exit-code slot is now a compile error instead of a silent false failure.
+
+**Payload change.** `os migrate recorded-by --json` and `os migrate resume
+--json` gained a `duration` key (milliseconds). Consumers that were reading the
+exit status of these two commands should note that a zero now means what it
+says.

packages/cli/src/commands/migrate/recorded-by.ts

@@ -126,7 +126,7 @@ export default class MigrateRecordedBy extends Command {
  // ── dry run (default): read-only ─────────────────────────────────
  if (!flags.apply) {
  if (flags.json) {
- await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false }, timer.elapsed());
+ await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false, duration: timer.elapsed() });
  return;
  }
  if (pending.length === 0) {
@@ -141,15 +141,15 @@ export default class MigrateRecordedBy extends Command {
  // ── apply ────────────────────────────────────────────────────────
  if (pending.length === 0) {
  const msg = `No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`;
- if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0 }, timer.elapsed()); return; }
+ if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0, duration: timer.elapsed() }); return; }
  printSuccess(msg);
  return;
  }
 
  if (!flags.yes) {
  const summary = `Rewrite recorded_by '${RECORDED_BY_SENTINEL}' → NULL on ${pending.length} row(s)`;
  if (flags.json || !process.stdin.isTTY) {
- if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+ if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
  printWarning(`Confirmation required: ${summary}. Re-run with --yes.`);
  this.exit(1);
  return;
@@ -162,7 +162,7 @@ export default class MigrateRecordedBy extends Command {
  const result = await runMigrationJournal(engine, plan);
 
  if (flags.json) {
- await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined }, timer.elapsed());
+ await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined, duration: timer.elapsed() });
  this.exit(result.status === 'completed' ? 0 : 1);
  return;
  }
@@ -188,7 +188,7 @@ export default class MigrateRecordedBy extends Command {
  const msg = error instanceof MigrationJournalRefusal
  ? `Refused (${error.code}): ${error.message}`
  : (error?.message || String(error));
- if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+ if (flags.json) { await emitJson({ error: msg, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
  printError(msg);
  this.exit(1);
  } finally {

packages/cli/src/commands/migrate/resume.ts

@@ -137,13 +137,11 @@ export default class MigrateResume extends Command {
  // ── list mode (no --run): read-only ──────────────────────────────
  if (!flags.run) {
  if (flags.json) {
- await emitJson(
- {
- interrupted: interrupted.map((r) => ({ ...r, resumable: Boolean(plans?.get(r.planId)) })),
- count: interrupted.length,
- },
- timer.elapsed(),
- );
+ await emitJson({
+ interrupted: interrupted.map((r) => ({ ...r, resumable: Boolean(plans?.get(r.planId)) })),
+ count: interrupted.length,
+ duration: timer.elapsed(),
+ });
  return;
  }
  if (interrupted.length === 0) {
@@ -167,7 +165,7 @@ export default class MigrateResume extends Command {
  : `Run '${flags.run}' is not interrupted — it already concluded (${
  events.some((e) => e.kind === 'run_done') ? 'run_done' : 'fully compensated'
  }). Nothing to do.`;
- if (flags.json) { await emitJson({ error: msg, runId: flags.run }, timer.elapsed(), { compact: true }); this.exit(events.length === 0 ? 1 : 0); return; }
+ if (flags.json) { await emitJson({ error: msg, runId: flags.run, duration: timer.elapsed() }, 0, { compact: true }); this.exit(events.length === 0 ? 1 : 0); return; }
  if (events.length === 0) { printError(msg); this.exit(1); return; }
  printSuccess(msg);
  return;
@@ -179,7 +177,7 @@ export default class MigrateResume extends Command {
  `Run '${target.runId}' belongs to plan '${target.planId}', which no loaded package registers. ` +
  `A resume needs the plan's code — the journal stores its hash, not its callbacks. ` +
  `Load the package that owns this migration and re-run.`;
- if (flags.json) { await emitJson({ error: msg, runId: target.runId, planId: target.planId }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+ if (flags.json) { await emitJson({ error: msg, runId: target.runId, planId: target.planId, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
  printError(msg);
  this.exit(1);
  return;
@@ -190,7 +188,7 @@ export default class MigrateResume extends Command {
  const summary = `${policy === 'compensate' ? 'UNWIND' : 'RESUME FORWARD'} run '${target.runId}' (plan '${plan.id}')`;
  if (flags.json || !process.stdin.isTTY) {
  const msg = `Confirmation required: ${summary}. Re-run with --yes.`;
- if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+ if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
  printWarning(msg);
  this.exit(1);
  return;
@@ -206,7 +204,7 @@ export default class MigrateResume extends Command {
  const result = await resumeMigrationJournal(engine, plan, target.runId);
 
  if (flags.json) {
- await emitJson({ ...result, error: result.error ? String(result.error) : undefined }, timer.elapsed());
+ await emitJson({ ...result, error: result.error ? String(result.error) : undefined, duration: timer.elapsed() });
  // A run that ended `failed` left the database in a state no clean story
  // covers, so the exit code has to say so — a zero here would let a
  // scripted recovery move on from a migration that needs a human.
@@ -237,7 +235,7 @@ export default class MigrateResume extends Command {
  // A refusal is the runner working, not breaking — say what it refused.
  ? `Refused (${error.code}): ${error.message}`
  : (error?.message || String(error));
- if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
+ if (flags.json) { await emitJson({ error: msg, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
  printError(msg);
  this.exit(1);
  } finally {

packages/cli/src/utils/format.exit-code.test.ts

@@ -0,0 +1,117 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `emitJson` / `emitText` write exactly two things: the payload, and
+ * `process.exitCode`. This pins the second one (#4873).
+ *
+ * The defect these tests exist for was not a wrong value computed somewhere —
+ * it was an ARGUMENT IN THE WRONG SLOT. `emitJson(payload, exitCode, opts)`
+ * takes its exit code second, positionally, and `os migrate recorded-by --json`
+ * / `os migrate resume --json` passed `timer.elapsed()` there: a duration in
+ * milliseconds. A fully successful run therefore ended with
+ * `process.exitCode = 531`, which the shell reports as `531 & 0xFF` = 19 — a
+ * different non-zero code on every run, on a command whose JSON was correct and
+ * whose stderr was empty.
+ *
+ * Two things are pinned here, and the second is the one that lasts:
+ *
+ * 1. the runtime contract — silence unless a caller asks for a failure code;
+ * 2. that a `number` can no longer reach that slot AT ALL (`CliExitCode`),
+ * so the same mistake is a compile error rather than a false failure for
+ * every scripted caller.
+ *
+ * (2) lives in `src/` deliberately: `packages/cli/tsconfig.json` includes
+ * `src`, so `pnpm typecheck` compiles this file and its `@ts-expect-error`
+ * directives are real. The same test under `packages/cli/test/` would be a
+ * phantom check — no tsc program reads that directory, so every directive in
+ * it would evaluate never and deleting them would leave every gate green.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { emitJson, emitText, createTimer } from './format.js';
+
+/** Whatever the runner was holding before this file ran — restored after each case. */
+const OUTER_EXIT_CODE = process.exitCode;
+
+describe('emitJson / emitText — process.exitCode (#4873)', () => {
+ let written: string[];
+ let writeSpy: ReturnType<typeof vi.spyOn>;
+
+ beforeEach(() => {
+ written = [];
+ // The real write must still invoke its callback: `emitText` awaits it, and
+ // that await is the whole point of the function (the #3512 pipe-truncation
+ // fix). A mock that swallows the callback hangs the test instead of failing
+ // it.
+ writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(((
+ chunk: unknown,
+ encodingOrCb: unknown,
+ maybeCb: unknown,
+ ) => {
+ written.push(String(chunk));
+ const done = typeof encodingOrCb === 'function' ? encodingOrCb : maybeCb;
+ if (typeof done === 'function') done();
+ return true;
+ }) as never);
+ process.exitCode = 0;
+ });
+
+ afterEach(() => {
+ writeSpy.mockRestore();
+ process.exitCode = OUTER_EXIT_CODE;
+ });
+
+ it('leaves the exit code alone on the success path', async () => {
+ await emitJson({ planId: 'x', pending: 0, applied: false, duration: 531 });
+
+ expect(JSON.parse(written.join(''))).toMatchObject({ pending: 0, duration: 531 });
+ expect(process.exitCode).toBe(0);
+ });
+
+ it('still records a failure when one is asked for — the other direction', async () => {
+ await emitJson({ error: 'confirmation_required' }, 1, { compact: true });
+
+ expect(process.exitCode).toBe(1);
+ // Compact is a formatting choice; it must not change the exit contract.
+ expect(written.join('')).toBe('{"error":"confirmation_required"}\n');
+ });
+
+ it('emitText carries the same contract — silent by default, 1 on request', async () => {
+ await emitText('hello');
+ expect(process.exitCode).toBe(0);
+
+ await emitText('goodbye', 1);
+ expect(process.exitCode).toBe(1);
+ });
+
+ /**
+ * The regression pin, written as the mistake itself.
+ *
+ * Both `@ts-expect-error`s below are the gate: if someone widens
+ * `CliExitCode` back to `number`, the directives become unused and tsc fails
+ * on THEM — which is the only way a repo-wide guarantee like this can be
+ * enforced from one file.
+ *
+ * The runtime half is kept because it is the evidence: with the type check
+ * suppressed, the exact call `recorded-by.ts` used to make still reproduces
+ * the defect verbatim, so this test states what the type is preventing
+ * rather than merely asserting that it prevents something.
+ */
+ it('a duration can no longer reach the exit-code slot (#4873)', async () => {
+ const timer = createTimer();
+ const durationMs = timer.elapsed() + 531; // a plausible `os migrate` run
+
+ // @ts-expect-error — a `number` is not a `CliExitCode`. This is exactly the
+ // call `migrate/recorded-by.ts` and `migrate/resume.ts` used to make.
+ const asExitCode: Parameters<typeof emitJson>[1] = durationMs;
+ expect(asExitCode).toBe(durationMs);
+
+ // @ts-expect-error — same rejection at the call site, which is where it bit.
+ await emitJson({ pending: 0, applied: false }, durationMs);
+
+ // And this is why the reported codes looked random rather than wrong: Node
+ // truncates the exit status to 8 bits, so 531 leaves the process as 19.
+ expect(process.exitCode).toBe(durationMs);
+ expect(durationMs & 0xff).toBe(19);
+ });
+});

packages/cli/src/utils/format.ts

@@ -11,6 +11,34 @@ export const CLI_ALIAS = 'os';
 
 // ─── Machine-readable output ────────────────────────────────────────
 
+/**
+ * The only two exit codes this CLI has: `0` success, `1` failure.
+ *
+ * Deliberately a narrow union rather than `number`, and that narrowness is the
+ * whole point. The value it types sits in the SECOND POSITIONAL slot of
+ * {@link emitJson} / {@link emitText} — immediately after a payload — where
+ * `number` accepted whatever numeric the caller happened to be holding.
+ * `os migrate recorded-by --json` and `os migrate resume --json` were holding
+ * `timer.elapsed()`, a DURATION in milliseconds, and passed it there (#4873).
+ *
+ * The result was invisible in every way an author checks: correct JSON on
+ * stdout, `✅ Graceful shutdown complete`, empty stderr — and
+ * `process.exitCode = 531`, which the shell reports as `531 & 0xFF` = 19. A
+ * different non-zero code on every run, because the code WAS the run's
+ * duration, so every caller that judges success by exit status (CI steps,
+ * `set -e`, Makefiles, container entrypoints) saw a random failure from a
+ * command that had just succeeded — the one audience `--json` exists for.
+ *
+ * Every other `--json` site in this CLI reports its duration INSIDE the
+ * payload (`{ ...report, duration: timer.elapsed() }` — `os lint`,
+ * `os migrate meta`, `os migrate summary-nulls`, `os meta resync`), which is
+ * what those two meant to do as well. With this union a duration in the exit
+ * slot is a compile error, so the mistake cannot be made silently again.
+ *
+ * Widening it is a deliberate act: a third code needs a meaning first.
+ */
+export type CliExitCode = 0 | 1;
+
 export interface EmitJsonOptions {
  /**
  * Emit on a single line instead of 2-space-indented.
@@ -60,7 +88,7 @@ export interface EmitJsonOptions {
  */
 export async function emitJson(
  payload: unknown,
- exitCode = 0,
+ exitCode: CliExitCode = 0,
  opts: EmitJsonOptions = {},
 ): Promise<void> {
  const text = opts.compact ? JSON.stringify(payload) : JSON.stringify(payload, null, 2);
@@ -100,7 +128,7 @@ export function isExitSignal(error: unknown): boolean {
  * why this cannot be fixed at the exit, or globally via blocking stdout,
  * applies here too.
  */
-export async function emitText(text: string, exitCode = 0): Promise<void> {
+export async function emitText(text: string, exitCode: CliExitCode = 0): Promise<void> {
  await new Promise<void>((resolve, reject) => {
  process.stdout.write(text + '\n', (err) => (err ? reject(err) : resolve()));
  });