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
Conversation
…reporting it
`os migrate recorded-by --apply --yes --json` printed its result, then a
second document `{"error":"EEXIT: 0"}`, and exited 1 after a completed run:
the `this.exit(…)` inside the command's `try` throws oclif's exit signal, and
the `catch` reported it as an error. `os migrate resume --run` (resumed or
already-concluded run), `os migrate account-issuer --json` (refusal) and the
text face of `os migrate apply` (account-issuer pre-flight refusal) had the
same catch. Each catch now opens with the existing idiom,
`if (isExitSignal(error)) throw error;`.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz
Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz
…eset Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz
Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz
…p landed Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 27 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin e67bee9e2f2ed3072fd68ae42a4e9fdece99001c && git checkout e67bee9e2f2ed3072fd68ae42a4e9fdece99001c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9ff74285f14b7b60699546211e53e9ccc13c0d61 88ae5769c6650826c892b452adddd1de69a7443b && git checkout -B drift-repro 9ff74285f14b7b60699546211e53e9ccc13c0d61 && git merge --no-ff 88ae5769c6650826c892b452adddd1de69a7443b
node scripts/docs-audit/affected-docs.mjs --json 9ff74285f14b7b60699546211e53e9ccc13c0d61
|
Fixes #21434
Clause-②: no
What was wrong
this.exit(n)throws oclif's exit signal (code: 'EEXIT',oclif.exit: n). When it runs inside atry, thetry's owncatchsees the signal first. Four migrate commands had acatchthat 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 throughbin/run-dev.js, a sqlite file with onesys_metadata_historyrow whoserecorded_byissystem):aa4632235)34a0cd121e)os migrate recorded-by --apply --yes --json{"error":"EEXIT: 0","duration":1020}; exit 1JSON.parseof the whole stdout succeeds); exit 0os migrate resume --run RUN_ID --json(run already concluded){"error":"EEXIT: 0",…}; exit 1os migrate resume --run RUN_ID --yes --json(journal rowrun_doneremoved){"error":"EEXIT: 1",…}; exit 1The merge of
mainafter34a0cd121etouched 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 swallowingcatchin:migrate/recorded-by.ts. 5this.exitsites in thetry, the completed-applythis.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 (extraEEXIT: 1line).migrate/apply.ts. 2 sites, text face only: thesys_account.issuerpre-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 outsidepackages/cli/src/commands/migrate/changes at runtime.Census: every command that takes a JSON face, at
88ae5769c6Derived from the oclif declarations. Each module under
src/commands(thesrc/twin ofoclif.commands= pattern,./dist/commands,**/*.js) is imported, and its default export'sstatic flagsis read, inherited flags included. A member declares a booleanjsonflag (32 commands) or a flag whoseoptionsinclude'json'(--format json, 14 commands). That makes 46 commands with 105this.exit-in-trysites.migrate recorded-by(5 leaking sites),migrate resume(8),migrate account-issuer(2),migrate apply(2, text face).cloud whoami(1 site),compile(21),build(inheritscompile'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).this.exitinside atry(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 thetryatrewrap.tsline 195, whosecatch(line 310) opens with the rethrow. #21469 added that line, so nothing here editsrewrap.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.tsis not touched.this.error(…)also raises a signalisExitSignalrecognises. Measured: no JSON-capable command calls it inside atry. The onlythis.errorcalls inside atryare 2 ininit.ts, which has no JSON face.The enumeration pin:
packages/cli/test/json-exit-signal.pin.test.ts(unit tier)isExitSignalimported from somewhere other thanutils/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.this.exitinside atry, direct or through a same-class method, must have theisExitSignalrethrow as the first statement of every enclosingcatch. 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 thatos build's chain reachescompile.ts.recorded-by,resumeandaccount-issuerrun in-process through oclif with a preloadedConfig.bootSchemaStack, the journal runner, the sentinel scan and the collision probe are replaced throughvi.mock. Each case asserts one JSON document on stdout (a bareJSON.parseof the whole of it) and the exit status. The cases are recorded-by--apply --yescompleted → 0, compensated → 1, failed → 1, and the confirmation refusal → 1. For resume:--run --yescompleted → 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/commandsthat declares either flag is in the population on the day it lands.secret rewrapis 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 withtierOfFile: signalsnone. Nothing is spawned and nothing boots. The boot seam is replaced throughvi.mockand 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
88ae5769c6:vitest run --project unit test/json-exit-signal.pin.test.tsgives 77 passed (77),VERDICT command-exit 0.9490dd3d75, before the merge. Both mutations went throughscripts/ablation-replace.mjsin a trap-restoring script, and the expected direction was red.recorded-by.tsrethrow replaced. The tool's literal count went 1 → 0 for the anchor and 0 → 1 for the marker, and the blob wente74172d2→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 showstdout carried 2 JSON documents … {"error":"EEXIT: 0","duration":1}. Restored: blob == HEAD andgit diff HEADempty.resume.tsrethrow replaced. Anchor 1 → 0, blob6e2c7de0→2cfe0d4c. Result: 8 failed / 68 passed (structural member + all 7 driven resume cases). Restored: blob == HEAD.grep -cof 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.88ae5769c6: the pin,recorded-by.test.ts,multi-value-columns.no-auto-run.test.ts,artifact-boot-migration.test.ts,format.exit-code.test.tsandschema-migration-plugins.test.tsgave 6 files, 134 tests passed,VERDICT command-exit 0.88ae5769c6:pnpm --filter @objectstack/cli typecheckgaveVERDICT command-exit 0.tsc --noEmitpassed.check:test-typecheckis OK with its ledger unchanged (3 files / 28 errors / 6 pinned signatures).88ae5769c6: all 65 families fromnode scripts/pm/dispatch-gates.mjs --commandsexit 0.--ranreconciliation: 65 derived, 65 run, 0 NOT-MEASURED. That zero is derived, because every line carries its exit code.check:i18n,check:i18n-coverageandcheck:i18n-walk-parityfirst answered exit 3 (prerequisite: CLI not built) and are green after building their declared closure.check:dual-build-cjs-loadsfirst answered exit 3 (6 unrelated packages had nodist/) and is green after building them..tsfiles with--no-inline-config --format jsonread 5 files, 0 errors, 0 warnings. The population comes from ESLint itself:isPathIgnoredis false for all 5 andcalculateConfigForFileresolves rules for each. Invariance:eslint.config.mjsnever enables type-aware linting (noparserOptions.project, no typed rules, stated at line 327), so this diff cannot move the verdict on any untouched file. The fullpnpm lintis CI's.NOT MEASURED
packages/clifull unit tier, not measured locally. Two attempts atvitest run --project unitwere 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/cliintegration tier, declared to CI. The diff touches no integration-tier file and no spawn entry.artifact-boot-migration.unbuildable-index.test.tsandsqlite-occupancy.test.tsreach the changed commands and live in that tier.json-stdout-purity,migrate-exit-code): not run. They drive only the bare--jsonforms, which never reached the defect.os migrate account-issuer --jsonrefusal. It needs a stack that registerssys_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 at88ae5769c6:{"error":"Cannot enumerate sys_account: …"}, one document, exit 1. The driven unit case covers the refusal path.os migrate resume --run … --yescompleted. 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 --yescannot resume any run at the public door.MigrationRecoveryPlugin, which owns themigration-plansregistry, is exported from@objectstack/runtimebut composed nowhere inpackages/cli. So every interrupted run answers "belongs to plan …, which no loaded package registers … Load the package that owns this migration". That holds even formetadata.recorded-by-sentinel-to-null, whose owner@objectstack/metadata-protocolthe CLI itself loads.recorded-by's in-processplans.register(plan)lands in the no-registrycatchfor the same reason. Measured above, and reported to the seat as a separate finding.os migrate resume --run RUN_ID --jsonexits 0 but puts its message under theerrorkey. That is unchanged here, and noted only.catchshape sits on three commands with no JSON face:os package install,os package publishandos plugin sign. Measured:os package install ./does-not-exist.jsonprints✗ 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