Skip to content

Commit 83df2fd

Browse files
qq9340100claude
andauthored
fix(cli): os migrate --json 成功时不再把耗时毫秒当成退出码 (#4873) (#6220)
* fix(cli): os migrate --json 不再把耗时毫秒当成退出码 根因:emitJson/emitText 的第二个位置参数是 exitCode,而 migrate recorded-by / migrate resume 在该槽位传的是 timer.elapsed() (毫秒耗时)。一条完全成功的命令因此设置 process.exitCode = 531, shell 看到 531 & 0xFF = 19 —— 每次运行都不同的随机非零码。 - format.ts: exitCode 参数收窄为 CliExitCode = 0 | 1,让'把耗时塞进 退出码槽位'从静默假失败变成编译错误 - recorded-by.ts / resume.ts: 耗时改为写进 payload 的 duration 字段, 与 os lint / os migrate meta / os meta resync 的既有约定一致 Refs #4873 * test(cli): 钉住 os migrate --json 的退出码契约(双向)+ changeset - src/utils/format.exit-code.test.ts:运行时契约 + 类型钉子。放在 src/ 是因为 packages/cli/tsconfig.json 只 include src,@ts-expect-error 只有 在被 tsc 编译的文件里才是真检查(AGENTS.md PINS_CHECKED) - test/migrate-exit-code.e2e.test.ts:真进程 spawn(bin/run-dev.js + tsx, 不依赖 dist)。成功 x3 恒 exit 0、payload 带 duration、至少一次 duration % 256 != 0(即旧代码下必然非零)、时长上界防 #4813 回归;失败方向用 不支持的 URL scheme 断言仍 exit 1 - 内核 INFO 日志占用 stdout 导致 --json 不可直接解析,已另行归档为 #6217 Refs #4873 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c9bf940 commit 83df2fd

6 files changed

Lines changed: 413 additions & 19 deletions

File tree

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

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ export default class MigrateRecordedBy extends Command {
126126
// ── dry run (default): read-only ─────────────────────────────────
127127
if (!flags.apply) {
128128
if (flags.json) {
129-
await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false }, timer.elapsed());
129+
await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, sentinel: RECORDED_BY_SENTINEL, pending: pending.length, applied: false, duration: timer.elapsed() });
130130
return;
131131
}
132132
if (pending.length === 0) {
@@ -141,15 +141,15 @@ export default class MigrateRecordedBy extends Command {
141141
// ── apply ────────────────────────────────────────────────────────
142142
if (pending.length === 0) {
143143
const msg = `No sys_metadata_history row holds the '${RECORDED_BY_SENTINEL}' sentinel — nothing to convert.`;
144-
if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0 }, timer.elapsed()); return; }
144+
if (flags.json) { await emitJson({ planId: RECORDED_BY_SENTINEL_PLAN_ID, pending: 0, applied: true, status: 'completed', chunksCommitted: 0, duration: timer.elapsed() }); return; }
145145
printSuccess(msg);
146146
return;
147147
}
148148

149149
if (!flags.yes) {
150150
const summary = `Rewrite recorded_by '${RECORDED_BY_SENTINEL}' → NULL on ${pending.length} row(s)`;
151151
if (flags.json || !process.stdin.isTTY) {
152-
if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
152+
if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
153153
printWarning(`Confirmation required: ${summary}. Re-run with --yes.`);
154154
this.exit(1);
155155
return;
@@ -162,7 +162,7 @@ export default class MigrateRecordedBy extends Command {
162162
const result = await runMigrationJournal(engine, plan);
163163

164164
if (flags.json) {
165-
await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined }, timer.elapsed());
165+
await emitJson({ ...result, pending: pending.length, applied: true, error: result.error ? String(result.error) : undefined, duration: timer.elapsed() });
166166
this.exit(result.status === 'completed' ? 0 : 1);
167167
return;
168168
}
@@ -188,7 +188,7 @@ export default class MigrateRecordedBy extends Command {
188188
const msg = error instanceof MigrationJournalRefusal
189189
? `Refused (${error.code}): ${error.message}`
190190
: (error?.message || String(error));
191-
if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
191+
if (flags.json) { await emitJson({ error: msg, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
192192
printError(msg);
193193
this.exit(1);
194194
} finally {

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

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -137,13 +137,11 @@ export default class MigrateResume extends Command {
137137
// ── list mode (no --run): read-only ──────────────────────────────
138138
if (!flags.run) {
139139
if (flags.json) {
140-
await emitJson(
141-
{
142-
interrupted: interrupted.map((r) => ({ ...r, resumable: Boolean(plans?.get(r.planId)) })),
143-
count: interrupted.length,
144-
},
145-
timer.elapsed(),
146-
);
140+
await emitJson({
141+
interrupted: interrupted.map((r) => ({ ...r, resumable: Boolean(plans?.get(r.planId)) })),
142+
count: interrupted.length,
143+
duration: timer.elapsed(),
144+
});
147145
return;
148146
}
149147
if (interrupted.length === 0) {
@@ -167,7 +165,7 @@ export default class MigrateResume extends Command {
167165
: `Run '${flags.run}' is not interrupted — it already concluded (${
168166
events.some((e) => e.kind === 'run_done') ? 'run_done' : 'fully compensated'
169167
}). Nothing to do.`;
170-
if (flags.json) { await emitJson({ error: msg, runId: flags.run }, timer.elapsed(), { compact: true }); this.exit(events.length === 0 ? 1 : 0); return; }
168+
if (flags.json) { await emitJson({ error: msg, runId: flags.run, duration: timer.elapsed() }, 0, { compact: true }); this.exit(events.length === 0 ? 1 : 0); return; }
171169
if (events.length === 0) { printError(msg); this.exit(1); return; }
172170
printSuccess(msg);
173171
return;
@@ -179,7 +177,7 @@ export default class MigrateResume extends Command {
179177
`Run '${target.runId}' belongs to plan '${target.planId}', which no loaded package registers. ` +
180178
`A resume needs the plan's code — the journal stores its hash, not its callbacks. ` +
181179
`Load the package that owns this migration and re-run.`;
182-
if (flags.json) { await emitJson({ error: msg, runId: target.runId, planId: target.planId }, timer.elapsed(), { compact: true }); this.exit(1); return; }
180+
if (flags.json) { await emitJson({ error: msg, runId: target.runId, planId: target.planId, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
183181
printError(msg);
184182
this.exit(1);
185183
return;
@@ -190,7 +188,7 @@ export default class MigrateResume extends Command {
190188
const summary = `${policy === 'compensate' ? 'UNWIND' : 'RESUME FORWARD'} run '${target.runId}' (plan '${plan.id}')`;
191189
if (flags.json || !process.stdin.isTTY) {
192190
const msg = `Confirmation required: ${summary}. Re-run with --yes.`;
193-
if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary }, timer.elapsed(), { compact: true }); this.exit(1); return; }
191+
if (flags.json) { await emitJson({ error: 'confirmation_required', hint: 'pass --yes', summary, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
194192
printWarning(msg);
195193
this.exit(1);
196194
return;
@@ -206,7 +204,7 @@ export default class MigrateResume extends Command {
206204
const result = await resumeMigrationJournal(engine, plan, target.runId);
207205

208206
if (flags.json) {
209-
await emitJson({ ...result, error: result.error ? String(result.error) : undefined }, timer.elapsed());
207+
await emitJson({ ...result, error: result.error ? String(result.error) : undefined, duration: timer.elapsed() });
210208
// A run that ended `failed` left the database in a state no clean story
211209
// covers, so the exit code has to say so — a zero here would let a
212210
// scripted recovery move on from a migration that needs a human.
@@ -237,7 +235,7 @@ export default class MigrateResume extends Command {
237235
// A refusal is the runner working, not breaking — say what it refused.
238236
? `Refused (${error.code}): ${error.message}`
239237
: (error?.message || String(error));
240-
if (flags.json) { await emitJson({ error: msg }, timer.elapsed(), { compact: true }); this.exit(1); return; }
238+
if (flags.json) { await emitJson({ error: msg, duration: timer.elapsed() }, 0, { compact: true }); this.exit(1); return; }
241239
printError(msg);
242240
this.exit(1);
243241
} finally {
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `emitJson` / `emitText` write exactly two things: the payload, and
5+
* `process.exitCode`. This pins the second one (#4873).
6+
*
7+
* The defect these tests exist for was not a wrong value computed somewhere —
8+
* it was an ARGUMENT IN THE WRONG SLOT. `emitJson(payload, exitCode, opts)`
9+
* takes its exit code second, positionally, and `os migrate recorded-by --json`
10+
* / `os migrate resume --json` passed `timer.elapsed()` there: a duration in
11+
* milliseconds. A fully successful run therefore ended with
12+
* `process.exitCode = 531`, which the shell reports as `531 & 0xFF` = 19 — a
13+
* different non-zero code on every run, on a command whose JSON was correct and
14+
* whose stderr was empty.
15+
*
16+
* Two things are pinned here, and the second is the one that lasts:
17+
*
18+
* 1. the runtime contract — silence unless a caller asks for a failure code;
19+
* 2. that a `number` can no longer reach that slot AT ALL (`CliExitCode`),
20+
* so the same mistake is a compile error rather than a false failure for
21+
* every scripted caller.
22+
*
23+
* (2) lives in `src/` deliberately: `packages/cli/tsconfig.json` includes
24+
* `src`, so `pnpm typecheck` compiles this file and its `@ts-expect-error`
25+
* directives are real. The same test under `packages/cli/test/` would be a
26+
* phantom check — no tsc program reads that directory, so every directive in
27+
* it would evaluate never and deleting them would leave every gate green.
28+
*/
29+
30+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
31+
import { emitJson, emitText, createTimer } from './format.js';
32+
33+
/** Whatever the runner was holding before this file ran — restored after each case. */
34+
const OUTER_EXIT_CODE = process.exitCode;
35+
36+
describe('emitJson / emitText — process.exitCode (#4873)', () => {
37+
let written: string[];
38+
let writeSpy: ReturnType<typeof vi.spyOn>;
39+
40+
beforeEach(() => {
41+
written = [];
42+
// The real write must still invoke its callback: `emitText` awaits it, and
43+
// that await is the whole point of the function (the #3512 pipe-truncation
44+
// fix). A mock that swallows the callback hangs the test instead of failing
45+
// it.
46+
writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(((
47+
chunk: unknown,
48+
encodingOrCb: unknown,
49+
maybeCb: unknown,
50+
) => {
51+
written.push(String(chunk));
52+
const done = typeof encodingOrCb === 'function' ? encodingOrCb : maybeCb;
53+
if (typeof done === 'function') done();
54+
return true;
55+
}) as never);
56+
process.exitCode = 0;
57+
});
58+
59+
afterEach(() => {
60+
writeSpy.mockRestore();
61+
process.exitCode = OUTER_EXIT_CODE;
62+
});
63+
64+
it('leaves the exit code alone on the success path', async () => {
65+
await emitJson({ planId: 'x', pending: 0, applied: false, duration: 531 });
66+
67+
expect(JSON.parse(written.join(''))).toMatchObject({ pending: 0, duration: 531 });
68+
expect(process.exitCode).toBe(0);
69+
});
70+
71+
it('still records a failure when one is asked for — the other direction', async () => {
72+
await emitJson({ error: 'confirmation_required' }, 1, { compact: true });
73+
74+
expect(process.exitCode).toBe(1);
75+
// Compact is a formatting choice; it must not change the exit contract.
76+
expect(written.join('')).toBe('{"error":"confirmation_required"}\n');
77+
});
78+
79+
it('emitText carries the same contract — silent by default, 1 on request', async () => {
80+
await emitText('hello');
81+
expect(process.exitCode).toBe(0);
82+
83+
await emitText('goodbye', 1);
84+
expect(process.exitCode).toBe(1);
85+
});
86+
87+
/**
88+
* The regression pin, written as the mistake itself.
89+
*
90+
* Both `@ts-expect-error`s below are the gate: if someone widens
91+
* `CliExitCode` back to `number`, the directives become unused and tsc fails
92+
* on THEM — which is the only way a repo-wide guarantee like this can be
93+
* enforced from one file.
94+
*
95+
* The runtime half is kept because it is the evidence: with the type check
96+
* suppressed, the exact call `recorded-by.ts` used to make still reproduces
97+
* the defect verbatim, so this test states what the type is preventing
98+
* rather than merely asserting that it prevents something.
99+
*/
100+
it('a duration can no longer reach the exit-code slot (#4873)', async () => {
101+
const timer = createTimer();
102+
const durationMs = timer.elapsed() + 531; // a plausible `os migrate` run
103+
104+
// @ts-expect-error — a `number` is not a `CliExitCode`. This is exactly the
105+
// call `migrate/recorded-by.ts` and `migrate/resume.ts` used to make.
106+
const asExitCode: Parameters<typeof emitJson>[1] = durationMs;
107+
expect(asExitCode).toBe(durationMs);
108+
109+
// @ts-expect-error — same rejection at the call site, which is where it bit.
110+
await emitJson({ pending: 0, applied: false }, durationMs);
111+
112+
// And this is why the reported codes looked random rather than wrong: Node
113+
// truncates the exit status to 8 bits, so 531 leaves the process as 19.
114+
expect(process.exitCode).toBe(durationMs);
115+
expect(durationMs & 0xff).toBe(19);
116+
});
117+
});

packages/cli/src/utils/format.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,34 @@ export const CLI_ALIAS = 'os';
1111

1212
// ─── Machine-readable output ────────────────────────────────────────
1313

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

0 commit comments

Comments
 (0)