Skip to content

Commit 2ee8383

Browse files
fix(cli): os migrate recorded-by, resume, account-issuer and apply rethrow oclif's exit signal, so a completed --json run prints one document and exits 0 (#21495)
Fixes #21434 Clause-②: no ## What was wrong `this.exit(n)` throws oclif's exit signal (`code: 'EEXIT'`, `oclif.exit: n`). When it runs inside a `try`, the `try`'s own `catch` sees the signal first. Four migrate commands had a `catch` that reported whatever it caught, so the signal came back out as an error. Measured at the public door on base `aa4632235` (CLI run from source through `bin/run-dev.js`, a sqlite file with one `sys_metadata_history` row whose `recorded_by` is `system`): | command | before (`aa4632235`) | after (`34a0cd121e`) | |---|---|---| | `os migrate recorded-by --apply --yes --json` | row converted; result document, then `{"error":"EEXIT: 0","duration":1020}`; **exit 1** | row converted; one document (`JSON.parse` of the whole stdout succeeds); **exit 0** | | `os migrate resume --run RUN_ID --json` (run already concluded) | refusal-shaped document, then `{"error":"EEXIT: 0",…}`; **exit 1** | one document; **exit 0** | | `os migrate resume --run RUN_ID --yes --json` (journal row `run_done` removed) | "no loaded package registers" document, then `{"error":"EEXIT: 1",…}`; exit 1 | one document; exit 1 | The merge of `main` after `34a0cd121e` touched none of the four command files. ## The fix The existing idiom, `if (isExitSignal(error)) throw error;` (`packages/cli/src/utils/format.ts`), as the first statement of the swallowing `catch` in: - `migrate/recorded-by.ts`. 5 `this.exit` sites in the `try`, the completed-apply `this.exit(0)` among them. - `migrate/resume.ts`. 8 sites: resumed run, already-concluded run, unknown run id, plan not loaded, confirmation required, failed run. - `migrate/account-issuer.ts`. 2 sites: the refused pre-flight on the JSON face (second document `{"error":"EEXIT: 1"}`) and on the text face (extra `EEXIT: 1` line). - `migrate/apply.ts`. 2 sites, text face only: the `sys_account.issuer` pre-flight refusals. Its JSON face returns before them. There is no second helper and no producer elsewhere. The swallowing happens in each command's own `catch`, and nothing outside `packages/cli/src/commands/migrate/` changes at runtime. ## Census: every command that takes a JSON face, at `88ae5769c6` Derived from the oclif declarations. Each module under `src/commands` (the `src/` twin of `oclif.commands` = pattern, `./dist/commands`, `**/*.js`) is imported, and its default export's `static flags` is read, inherited flags included. A member declares a boolean `json` flag (32 commands) or a flag whose `options` include `'json'` (`--format json`, 14 commands). That makes 46 commands with 105 `this.exit`-in-`try` sites. - **In-family (4, fixed here):** `migrate recorded-by` (5 leaking sites), `migrate resume` (8), `migrate account-issuer` (2), `migrate apply` (2, text face). - **Already correct (14):** `cloud whoami` (1 site), `compile` (21), `build` (inherits `compile`'s 21), `environments bind` (2), `environments create` (1), `migrate audit-metadata-bodies` (2), `migrate files-to-references` (2), `migrate multi-value-columns` (4), `migrate summary-nulls` (2), `migrate value-shapes` (3), `secret orphans` (7), `secret rewrap` (6), `validate` (15), `verify` (1). - **Not affected, no `this.exit` inside a `try` (28):** `cloud login`, `cloud logout`, `data create`, `data delete`, `data get`, `data query`, `data update`, `diff`, `environments list`, `environments show`, `explain`, `i18n check`, `i18n extract`, `info`, `lint`, `login`, `logout`, `meta delete`, `meta get`, `meta list`, `meta register`, `meta resync`, `migrate`, `migrate meta`, `migrate plan`, `register`, `storage orphans`, `whoami`. **`secret rewrap`, which landed in #21469 while this branch was open, is already correct.** Its 6 sites sit in the `try` at `rewrap.ts` line 195, whose `catch` (line 310) opens with the rethrow. #21469 added that line, so nothing here edits `rewrap.ts`. This PR adds no driven case for it: it is not in-family, and the structural half covers its exit path. `json-stdout-purity.e2e.test.ts` is not touched. `this.error(…)` also raises a signal `isExitSignal` recognises. Measured: no JSON-capable command calls it inside a `try`. The only `this.error` calls inside a `try` are 2 in `init.ts`, which has no JSON face. ## The enumeration pin: `packages/cli/test/json-exit-signal.pin.test.ts` (unit tier) 1. **Analyzer fixtures (15 cases).** The detector is shown to fail on each shape it must catch: no rethrow, a catch with no binding, the rethrow not first, a second helper, `isExitSignal` imported from somewhere other than `utils/format.js`, an exit through a same-class helper or an arrow-function property, nested tries, an exit in an inner catch, an exit in a callback. It also passes the idiom, an unconditional rethrow, and try/finally. 2. **Structural, over the whole discovered population (46 cases + 3 meta).** Every `this.exit` inside a `try`, direct or through a same-class method, must have the `isExitSignal` rethrow as the first statement of every enclosing `catch`. The check covers the command's own source and its superclasses' sources. The meta checks are: package.json's oclif command strategy is the one the walk mirrors; every walked module is a command; the population floor (46) and site floor (105); the named anchors; and that `os build`'s chain reaches `compile.ts`. 3. **Driven, in-family completed paths (13 cases).** `recorded-by`, `resume` and `account-issuer` run in-process through oclif with a preloaded `Config`. `bootSchemaStack`, the journal runner, the sentinel scan and the collision probe are replaced through `vi.mock`. Each case asserts one JSON document on stdout (a bare `JSON.parse` of the whole of it) and the exit status. The cases are recorded-by `--apply --yes` completed → 0, compensated → 1, failed → 1, and the confirmation refusal → 1. For resume: `--run --yes` completed → 0, compensated → 0, failed → 1; already concluded → 0; unknown id → 1; confirmation → 1; plan not loaded → 1. For account-issuer: ok → 0, refused → 1. **How a later command enters:** there is no roster. A module under `src/commands` that declares either flag is in the population on the day it lands. `secret rewrap` is the first to have entered that way, and no line names it. The floors are the only hand-edited numbers. They catch a discovery or analyzer that silently returns zero. **Tier:** `unit`, measured with `tierOfFile`: signals `none`. Nothing is spawned and nothing boots. The boot seam is replaced through `vi.mock` and never value-imported. **Cost:** about 23 to 26 s of import at collection on a shared box, outside any clocked window. A single-command file in the same package (`recorded-by.test.ts`) imports in 15.7 s on the same box, so the all-commands discovery adds about 10 s. ## Evidence - **Pin** at `88ae5769c6`: `vitest run --project unit test/json-exit-signal.pin.test.ts` gives 77 passed (77), `VERDICT command-exit 0`. - **Ablations** at `9490dd3d75`, before the merge. Both mutations went through `scripts/ablation-replace.mjs` in a trap-restoring script, and the expected direction was red. - `recorded-by.ts` rethrow replaced. The tool's literal count went 1 → 0 for the anchor and 0 → 1 for the marker, and the blob went `e74172d2` → `31085626`. Result: **5 failed / 71 passed**. The failures are the structural member (5 sites "swallowed by the catch at line 196") and the 4 driven recorded-by cases, which show `stdout carried 2 JSON documents … {"error":"EEXIT: 0","duration":1}`. Restored: blob == HEAD and `git diff HEAD` empty. - `resume.ts` rethrow replaced. Anchor 1 → 0, blob `6e2c7de0` → `2cfe0d4c`. Result: **8 failed / 68 passed** (structural member + all 7 driven resume cases). Restored: blob == HEAD. - The hand `grep -c` of the first marker inside the wrapped command read 0. That marker contains `*`, which grep reads as a regex, so the hand count is void. The tool's literal before/after counts and blob hashes are the evidence that the mutation landed. - **Unit tests that reach the changed commands**, at `88ae5769c6`: the pin, `recorded-by.test.ts`, `multi-value-columns.no-auto-run.test.ts`, `artifact-boot-migration.test.ts`, `format.exit-code.test.ts` and `schema-migration-plugins.test.ts` gave 6 files, 134 tests passed, `VERDICT command-exit 0`. - **Typecheck** at `88ae5769c6`: `pnpm --filter @objectstack/cli typecheck` gave `VERDICT command-exit 0`. `tsc --noEmit` passed. `check:test-typecheck` is OK with its ledger unchanged (3 files / 28 errors / 6 pinned signatures). - **Gates** at `88ae5769c6`: all 65 families from `node scripts/pm/dispatch-gates.mjs --commands` exit 0. `--ran` reconciliation: 65 derived, 65 run, 0 NOT-MEASURED. That zero is derived, because every line carries its exit code. `check:i18n`, `check:i18n-coverage` and `check:i18n-walk-parity` first answered exit 3 (prerequisite: CLI not built) and are green after building their declared closure. `check:dual-build-cjs-loads` first answered exit 3 (6 unrelated packages had no `dist/`) and is green after building them. - **Lint**, as a proven narrowing. ESLint over the 5 changed `.ts` files with `--no-inline-config --format json` read 5 files, 0 errors, 0 warnings. The population comes from ESLint itself: `isPathIgnored` is false for all 5 and `calculateConfigForFile` resolves rules for each. Invariance: `eslint.config.mjs` never enables type-aware linting (no `parserOptions.project`, no typed rules, stated at line 327), so this diff cannot move the verdict on any untouched file. The full `pnpm lint` is CI's. ## NOT MEASURED - **`packages/cli` full unit tier**, not measured locally. Two attempts at `vitest run --project unit` were cut off: exit 137, then a container restart mid-run. It is narrowed to the 6 unit files above, which import or reach the four changed commands. CI runs the whole tier. - **`packages/cli` integration tier**, declared to CI. The diff touches no integration-tier file and no spawn entry. `artifact-boot-migration.unbuildable-index.test.ts` and `sqlite-occupancy.test.ts` reach the changed commands and live in that tier. - **Nightly e2e** (`json-stdout-purity`, `migrate-exit-code`): not run. They drive only the bare `--json` forms, which never reached the defect. - **Public door, `os migrate account-issuer --json` refusal.** It needs a stack that registers `sys_account` (plugin-auth's object) with colliding rows. The plain project stack does not register it, so the door answers a read refusal before the path is reached. Measured at `88ae5769c6`: `{"error":"Cannot enumerate sys_account: …"}`, one document, exit 1. The driven unit case covers the refusal path. - **Public door, `os migrate resume --run … --yes` completed.** It is unreachable at the door today; see the first acceptance note. The driven unit case covers it. ## Acceptance notes - `os migrate resume --run RUN_ID --yes` cannot resume any run at the public door. `MigrationRecoveryPlugin`, which owns the `migration-plans` registry, is exported from `@objectstack/runtime` but composed nowhere in `packages/cli`. So every interrupted run answers "belongs to plan …, which no loaded package registers … Load the package that owns this migration". That holds even for `metadata.recorded-by-sentinel-to-null`, whose owner `@objectstack/metadata-protocol` the CLI itself loads. `recorded-by`'s in-process `plans.register(plan)` lands in the no-registry `catch` for the same reason. Measured above, and reported to the seat as a separate finding. - On an already-concluded run, `os migrate resume --run RUN_ID --json` exits 0 but puts its message under the `error` key. That is unchanged here, and noted only. - The same `catch` shape sits on three commands with no JSON face: `os package install`, `os package publish` and `os plugin sign`. Measured: `os package install ./does-not-exist.json` prints `✗ Cannot read artifact: …`, then `✗ EEXIT: 1`, and exits 1. The exit status is right and the extra line is wrong. These files are outside this claim's declared file surface, which covers JSON-face commands, so they are not touched here. They are reported to the seat as a finding. --- _Generated by [Claude Code](https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 85e29b8 commit 2ee8383

6 files changed

Lines changed: 779 additions & 0 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
fix(cli): `os migrate recorded-by`, `resume` and `account-issuer` print exactly one `--json` document, and a completed run exits 0 (#21434)
6+
7+
Clause-②: no
8+
9+
`os migrate recorded-by --apply --yes --json` converted the rows, printed its result, then printed a second document, `{"error":"EEXIT: 0","duration":…}`, and exited 1. A script that read the exit status took the completed run for a failure, and a parser that read stdout failed on the second document. The cause was the command's own `catch`: the `this.exit(…)` inside its `try` throws oclif's exit signal, and the `catch` reported the signal as an error.
10+
11+
The same `catch` sat in three more commands:
12+
13+
- **`os migrate resume --run <id> --json`.** A run that was already concluded printed a second `{"error":"EEXIT: 0"}` and exited 1 instead of 0. A resumed run did the same. Every refusal inside the command (unknown run id, plan not loaded, confirmation required) printed a second `{"error":"EEXIT: 1"}` under its own document.
14+
- **`os migrate account-issuer --json`.** A refused pre-flight printed a second `{"error":"EEXIT: 1"}` under its report. Without `--json`, it printed an extra `EEXIT: 1` error line.
15+
- **`os migrate apply`** (text output). A `sys_account.issuer` pre-flight refusal printed an extra `EEXIT: 1` error line.
16+
17+
Each command now prints one document and exits with the status it computes. A completed `recorded-by --apply` and an already-concluded or resumed `resume --run` exit 0. Refusals and failed runs still exit 1. A script that worked around the second document or the exit status 1 can drop that workaround.

‎packages/cli/src/commands/migrate/account-issuer.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
createTimer,
1313
emitJson,
1414
errorCodeFields,
15+
isExitSignal,
1516
} from '../../utils/format.js';
1617
import { bootSchemaStack } from '../../utils/schema-migrate.js';
1718

@@ -161,6 +162,10 @@ export default class MigrateAccountIssuer extends Command {
161162
);
162163
this.exit(1);
163164
} catch (error: any) {
165+
// [#21434] The `this.exit(1)` calls above throw oclif's exit signal from
166+
// inside this `try`; re-reporting it printed a second `--json` document
167+
// (`{"error":"EEXIT: 1"}`) after the refusal report.
168+
if (isExitSignal(error)) throw error;
164169
// A refusal from the probe itself (unreadable table, truncated scan)
165170
// lands here and stays a refusal — it is never softened into a clean run.
166171
if (flags.json) {

‎packages/cli/src/commands/migrate/apply.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
createTimer,
1414
emitJson,
1515
errorCodeFields,
16+
isExitSignal,
1617
} from '../../utils/format.js';
1718
import {
1819
bootSchemaStack,
@@ -447,6 +448,10 @@ export default class MigrateApply extends Command {
447448
console.log(chalk.dim(` ${timer.display()}`));
448449
console.log('');
449450
} catch (error: any) {
451+
// [#21434] The account-issuer pre-flight refusals above `this.exit(1)`
452+
// from inside this `try`; re-reporting the signal printed a second
453+
// error line, `EEXIT: 1`, under the refusal.
454+
if (isExitSignal(error)) throw error;
450455
if (flags.json) { await emitJson({ error: error.message, ...errorCodeFields(error) }, 0, { compact: true }); this.exit(1); }
451456
printError(error.message || String(error));
452457
this.exit(1);

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
createTimer,
2626
emitJson,
2727
errorCodeFields,
28+
isExitSignal,
2829
} from '../../utils/format.js';
2930
import { bootSchemaStack } from '../../utils/schema-migrate.js';
3031
import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js';
@@ -193,6 +194,10 @@ export default class MigrateRecordedBy extends Command {
193194
this.exit(1);
194195
}
195196
} catch (error: any) {
197+
// [#21434] The `this.exit(…)` calls above throw oclif's exit signal from
198+
// inside this `try`. Re-reporting it printed a second `--json` document
199+
// (`{"error":"EEXIT: 0"}`) and turned a completed apply's exit 0 into 1.
200+
if (isExitSignal(error)) throw error;
196201
const msg = error instanceof MigrationJournalRefusal
197202
? `Refused (${error.code}): ${error.message}`
198203
: (error?.message || String(error));

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
createTimer,
2424
emitJson,
2525
errorCodeFields,
26+
isExitSignal,
2627
} from '../../utils/format.js';
2728
import { bootSchemaStack } from '../../utils/schema-migrate.js';
2829
import { buildDataMigrationPlugins } from '../../utils/data-migration-plugins.js';
@@ -238,6 +239,11 @@ export default class MigrateResume extends Command {
238239
this.exit(1);
239240
}
240241
} catch (error: any) {
242+
// [#21434] The `this.exit(…)` calls above throw oclif's exit signal from
243+
// inside this `try`. Re-reporting it printed a second `--json` document
244+
// and turned every exit 0 above (a resumed run, an already-concluded
245+
// run) into exit 1.
246+
if (isExitSignal(error)) throw error;
241247
const msg = error instanceof MigrationJournalRefusal
242248
// A refusal is the runner working, not breaking — say what it refused.
243249
? `Refused (${error.code}): ${error.message}`

0 commit comments

Comments
 (0)