Skip to content

Commit 62bc129

Browse files
fix(cli)!: os migrate apply exits 1 when the confirmation it needs is not given (#25004)
Fixes #24949 Clause-②: no (narrowing) `os migrate apply` now exits `1` when the confirmation it needs is not given (stdin is not a terminal, or `--json`, and no `--yes`), as every other `os migrate` subcommand with a confirmation gate already does. Nothing else about that run changes: it still applies nothing, prints the same warning and, under `--json`, emits the same document. An operator who answers `n` at the terminal prompt still gets exit `0`. ## Measured before the change (the CLI binary built at the branch base `e4d604efd3`) The fixture is a config-only project on a SQLite file (`OS_DATABASE_URL`, `NODE_ENV=production`). It has one object whose `legacy_note` field was removed after a first `os migrate apply --yes`, so `os migrate plan` lists one destructive change, `os24949_thing.legacy_note [drop_column]`. A `safe drift` copy instead declares one new field (an `add_column`). For `recorded-by` and `resume`, a copy was seeded with one sentinel history row and one interrupted journal run, so each run reaches its gate. `organization-ownership` was given a plan file its own plan run wrote. "Written" compares a content hash of the schema plus every row of every table, read on a separate connection before and after each run. It is never a file hash. The terminal column used a real pseudo-terminal that answered `n` at the `[y/N]` prompt. | subcommand, writing mode, no `--yes` | stdin `/dev/null` | `--json` | terminal, answered `n` | written | | :-- | :-- | :-- | :-- | :-- | | `apply --allow-destructive` | **0** | **0** | 0 | nothing (the column is still there) | | `apply` (safe drift) | **0** | **0** | n/a | nothing | | `activity-changed-fields --apply` | 1 | 1 | 0 | nothing | | `audit-mail-bodies --apply` | 1 | 1 | 0 | nothing | | `audit-metadata-bodies --apply` | 1 | 1 | 0 | nothing | | `comment-parent-pair --apply` | 1 | 1 | 0 | nothing | | `comment-reactions --apply` | 1 | 1 | 0 | nothing | | `files-to-references --apply` | 1 | 1 | 0 | nothing | | `meta --stored --apply` | 1 | 1 | 0 | nothing | | `multi-value-columns --apply` | 1 | 1 | 0 | nothing | | `recorded-by --apply` | 1 | 1 | 0 | nothing | | `resume --run RUN` | 1 | 1 | 0 | nothing | | `security-catalog-overlays --apply` | 1 | 1 | 0 | nothing | | `summary-nulls --apply` | 1 | 1 | 0 | nothing | | `value-shapes --apply` | 1 | 1 | 0 | nothing | | `organization-ownership --apply --plan-file PLAN` (no `--backup-taken`) | 1 | 1 | 1 | nothing | - Every non-interactive run printed a line naming its confirmation flag: `--yes` for every row except the last, which names `--backup-taken`. No run wrote anything without its confirmation. - `apply` was the only member answering 0. `summary-nulls` and `value-shapes` were not in the claim's candidate list. They have `--yes` gates too and were measured as control rows. - **Detection is one rule across the family.** Every gate is `flags.json || !process.stdin.isTTY`, and every `confirm()` returns false on a non-TTY stdin before it builds a prompt. No CI variable is read, `apply` included. The divergence was only the exit. - `recorded-by --apply` and `resume --run` gate after a plain boot. Re-measured on a copy whose config was one field ahead of the database, the content hash was unchanged: that boot added no column. ## The change - `packages/cli/src/commands/migrate/apply.ts`: the text face now ends with `this.exit(1)` after its warning. The `--json` face emits its document with exit `1` (`emitJson(…, 1); return;`), which is the spelling `apply`'s own three refusals already use, so the one-shot exit at the end of `run()` leaves with it. The warning text and the document are byte-for-byte unchanged. - **Route.** No shared helper or named exit constant exists for this. Each sibling spells it inline (`printWarning(…); this.exit(1)`), and `CliExitCode` in `src/utils/format.ts` is `0 | 1`. A new helper adopted by the 14 sites would rewrite 13 that are already right. The pin below holds the family to one code instead, and its population is discovered. ## The pin: `packages/cli/test/migrate-confirmation-exit.pin.test.ts` (unit tier) - **Population, discovered.** Every module under `src/commands/migrate` whose command class declares a `yes` or `apply` flag. That is 15 members, and the roster must equal them. A future subcommand with a confirmation gate fails the first case by id until its row is added. - **Per member, both faces**, with `process.stdin.isTTY` set to false by the case itself: exit `1`, the text face names the member's confirmation flag, and no write seam is reached. For members that gate before the boot, the write seam is the boot. For `apply` it is `flushSchemaDdl` / `applyMigrationEntries`, and for `recorded-by` / `resume` the journal runner. - **Controls.** `apply --allow-destructive --yes` applies and exits `0`. `apply` on a terminal answered `n` asks the prompt once, writes nothing and exits `0`. - **Replaced seams:** the occupancy probe, the boot, the journal runner and reads, the sentinel scan, the one-shot exit and readline. The parse, the gates, `emitJson` and `this.exit` run for real. The commands are imported from `src/`, so no `dist/` sits on the resolution path. Evidence: - **Red on `main`.** `apply.ts` was restored from the base into the tree only (tree blob `091e6bb` equals the base blob). The pin ran 2 failed, 32 passed: `apply — text face` and `apply — --json face`, each `expected +0 to be 1`. The restore was `git checkout HEAD`, and the blob `5581f5d` equals HEAD, `git diff HEAD` is empty and `git status` is clean. - **Green on the branch:** 34 passed. - **Ablations** through `scripts/ablation-replace.mjs`. Each mutation was proven on disk (anchor 1 → 0, blob changed) and restored hash-equal to HEAD with `git diff HEAD` empty: - removing `apply`'s text-face `this.exit(1)` → 1 failed (`apply — text face`), 33 passed; - `apply`'s `--json` exit `1` → `0` → 1 failed (`apply — --json face`), 33 passed; - removing `comment-reactions`'s text-face `this.exit(1)` → 1 failed (`comment-reactions — text face`), 33 passed. The per-member rows hold the siblings, not only `apply`. - A first attempt at the text-face ablation was refused by the tool before any test ran. Its replacement was a substring of the anchor, so the replacement count could not rise. The file was restored hash-equal and the ablation was re-run with a marked replacement. The first attempt was a no-op and is not counted. ## After, at the public door (the CLI built from this branch) | command | exit | written | | :-- | :-- | :-- | | `apply --allow-destructive`, stdin `/dev/null` | 1 | nothing; the same `⚠ Confirmation required …` line | | `apply --allow-destructive --json` | 1 | nothing; `message: confirmation_required`, `hint: pass --yes` | | `apply` (safe drift), stdin `/dev/null` | 1 | nothing | | `apply --allow-destructive`, terminal answered `n` | 0 | nothing; `Aborted — no changes made.` | | `apply --allow-destructive --yes` (control) | 0 | the column dropped; `✓ Applied 1 change(s).` | ## Local verification (final head `5a3e45078e`) - `@objectstack/cli` unit tier (`vitest run --project unit`): 316 files, 4684 tests passed, at `ddd7dfdb62`. The only later commit is a type cast in the new test file, and that file re-ran green at `5a3e45078e` (34 passed). - Integration tier declared to CI: the diff touches no integration-tier file and no spawn entry. The one integration file whose run reaches `apply`'s gate (`plan.reads-env-files.integration.test.ts`, which reads the document and not the exit) was run anyway: 6 passed. - `pnpm --filter @objectstack/cli typecheck`: exit 0 (`check:test-typecheck: OK`). The first run flagged one TS2352 in the new test file, fixed in `5a3e45078e`. - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 68 commands, and its 11 declared WIDE-population families were run too, 79 in all. 77 exited 0 on the first pass. `check:dual-build-cjs-loads` and `check:i18n-coverage` exited 3 (`PREREQUISITE NOT MET`: no `dist/` for packages this diff does not touch). After a workspace build both exited 0. `--ran` printed `✓ dispatch-gates --ran: 68 derived famil(ies) accounted for — 68 run, 0 NOT-MEASURED`. - `node scripts/check-changeset-no-major.mjs --base origin/main`: exit 0, `✓ This diff introduces no major bump.` - `node scripts/check-adr-0087-registration.mjs --base e4d604e`: exit 0. It lists `24949-migrate-apply-confirmation-exit.md [BREAKING+bang+clause-②-narrowing] not-required (no-migration-prescription)`. - `pnpm lint` (the whole repository, `eslint . --no-inline-config`): exit 0. Changeset: `.changeset/24949-migrate-apply-confirmation-exit.md`, `@objectstack/cli` `minor`, with a **BREAKING** banner, the FROM → TO, what is unchanged, the one-line script fix (pass `--yes`, or run `os migrate plan` to preview) and its ADR-0087 disposition. Clause-② narrowing means a `CONTRACT_REVIEW_TIER` review of record is owed before the queue. ## Acceptance notes - **`organization-ownership`'s terminal decline exits 1; every other member's exits 0.** Its prompt asks whether a backup was taken, and a `n` there is the refusal "--apply needs a backup of this database first". This was measured and left as it is: an interactive decline keeps its code, and that is a hold-off of this card. No carrier. - **Docs that still say exit 0.** `content/docs/upgrading.mdx` (the v18.0.0 step 8) and `content/docs/releases/v18/18-0.mdx` (the 18.0.0 checklist, from #24922: "read the output, not the exit code") say a non-interactive `apply` without `--yes` still exits `0`. Both describe the measured 18.0.0 walk, which stays true of 18.0.0. The release page is release-owned and is not edited in a code PR. The behaviour change reaches the next release's notes through this changeset. Carrier: the release compilation (maintainer). - **`apply`'s `--json` document names the condition in `message`, where its siblings use `error`.** The document shape is a hold-off of this card, and it is unchanged. Noted only. - `main` moved 5 commits past the base while this ran, none of them under `packages/cli`. The branch was not merged with it. --- _Generated by [Claude Code](https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 93e6c1a commit 62bc129

3 files changed

Lines changed: 434 additions & 1 deletion

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/cli': minor
3+
---
4+
5+
fix(cli)!: `os migrate apply` exits `1` when the confirmation it needs is not given, as every other `os migrate` subcommand with a `--yes` gate already does
6+
7+
In a non-interactive shell (stdin is not a terminal) or under `--json`, `os migrate apply` without `--yes` applies nothing and prints `⚠ Confirmation required. Re-run with --yes to apply, or use "os migrate plan" to preview.` (under `--json`, its `confirmation_required` document). It exited `0` there, so a deploy script or an agent reading the exit status took a run that applied nothing for an applied one, and the next boot still reported the drift. Every other `os migrate` subcommand that writes behind `--yes` — `comment-reactions --apply`, `meta --stored --apply`, `recorded-by --apply`, `resume --run` and the rest — already exits `1` in that situation, as `organization-ownership --apply` does without `--backup-taken`. `apply` now answers the same way, in both faces. The warning and the `--json` document are unchanged.
8+
9+
Clause-②: no (narrowing)
10+
11+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, object definition or stored shape is added, removed, renamed or re-shaped, and no exported declaration is removed or narrowed, so there is nothing for `objectstack migrate meta` to rewrite. What changes is the exit status one CLI command returns when it applied nothing because the confirmation it asks for was not given; the remedy is a flag on the caller's command line (`--yes`), never a rewrite of a package source or a stored row. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered); and nothing exported is removed or narrowed (not runtime-interface-only, type-surface-only or runtime-export-retired). -->
12+
13+
**BREAKING** (an exit status narrows), shipped as `minor` under the launch-window convention for breaking changes.
14+
15+
- **FROM:** `os migrate apply` (with or without `--allow-destructive`) on a database with changes to apply, without `--yes`, with stdin not a terminal or with `--json`: nothing applied, exit `0`.
16+
- **TO:** nothing applied, exit `1`. The printed warning and the `--json` document (`message: 'confirmation_required'`, `hint: 'pass --yes'`) are unchanged.
17+
- **Unchanged:** answering `n` at the `[y/N]` prompt in a terminal still prints `Aborted — no changes made.` and exits `0`; a run with `--yes` applies and exits `0`; a database with nothing to apply exits `0` without asking.
18+
- **The fix for a script:** a script that means to apply passes `--yes` (`os migrate apply --yes`, or `os migrate apply --allow-destructive --yes`); one that only wants to know what would change runs `os migrate plan`.

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -612,10 +612,19 @@ export default class MigrateApply extends Command {
612612
}
613613

614614
// Confirmation gate. Nothing above this line has touched the database.
615+
//
616+
// [#24949] A confirmation the run needs and did not get — a non-TTY
617+
// stdin, or `--json`, without `--yes` — applies nothing and exits 1, in
618+
// both faces, as every other `os migrate` subcommand with a `--yes` gate
619+
// does (`test/migrate-confirmation-exit.pin.test.ts` holds the family to
620+
// it). It used to exit 0, so a deploy script reading `$?` took a run
621+
// that applied nothing for an applied one. An operator who answers `n`
622+
// at the prompt below declined on purpose: that path keeps exit 0.
615623
if (!flags.yes) {
616624
if (flags.json || !process.stdin.isTTY) {
617-
if (flags.json) { await emitJson({ ...target, applied: [], skipped: drift, pending, message: 'confirmation_required', hint: 'pass --yes' }, 0, { compact: true }); return; }
625+
if (flags.json) { await emitJson({ ...target, applied: [], skipped: drift, pending, message: 'confirmation_required', hint: 'pass --yes' }, 1, { compact: true }); return; }
618626
printWarning('Confirmation required. Re-run with --yes to apply, or use "os migrate plan" to preview.');
627+
this.exit(1);
619628
return;
620629
}
621630
const targets = stack.telemetryDatabase !== null

0 commit comments

Comments
 (0)