fix(cli): os migrate plan and apply name each drop gate's command, never --allow-destructive, for the drops apply refuses

.changeset/24958-migrate-drop-gate-remedies.md

@@ -0,0 +1,15 @@
+---
+'@objectstack/cli': patch
+---
+
+fix(cli): `os migrate plan` and `os migrate apply` name the command each held drop waits for, never `--allow-destructive`, for every drop `apply` refuses (#24958)
+
+Clause-②: no
+
+`os migrate apply --allow-destructive` refuses the whole run, whatever flag is passed, while it would drop one of three columns before its precondition holds: `sys_account.issuer` while its retirement pre-flight finds `(provider_id, account_id)` keys held by more than one row (or cannot read `sys_account`), `sys_comment.reactions` while no verified run of `os migrate comment-reactions --apply` is recorded, and an `organization_id` column while the organization-ownership ceremony has not verified. Yet `os migrate plan` offered `--allow-destructive` for the first two, and `os migrate apply` without the flag listed all three with `"os migrate apply --allow-destructive" to drop it.` and closed with `N destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).` and `Skipped N destructive change(s) (re-run with --allow-destructive to include them).` That re-run was refused, exit 1.
+
+- **Now.** Each such drop is still listed and counted as destructive drift, in both commands. Its line names what it waits for: the ceremony, `os migrate comment-reactions`, or the retirement pre-flight `os migrate account-issuer`.
+- **The footer.** One line per gate that holds a drop names that gate's commands and says `--allow-destructive` is refused until they have run. `os migrate plan` offers `--allow-destructive` for nothing while a drop is held. `os migrate apply` prints `N destructive change(s) will be SKIPPED, and a re-run with --allow-destructive cannot include them yet:` before those lines, and `Skipped N destructive change(s); a re-run with --allow-destructive cannot include them yet (see above).` after the apply.
+- **Unchanged.** A database with no held drop prints what it printed before, byte for byte, in both commands. The `--json` documents, the exit codes and every refusal of `os migrate apply --allow-destructive` (its rule and its text) are unchanged.
+
+**What to do.** Nothing. Follow the footer: run the command each held drop names, then re-run `os migrate plan`.

packages/cli/src/commands/migrate/apply.account-issuer-json-refusal.test.ts

@@ -69,7 +69,9 @@ const ISSUER_DROP = {
   table: 'sys_account',
   column: 'issuer',
   op: { type: 'drop_column', column: 'issuer' },
-  description: 'sys_account.issuer: column exists in the database but not in metadata (orphaned)',
+  // The differ's `message` (`ManagedDriftEntry`), remedy sentence included: the
+  // listing above the gate reads it (#24958).
+  message: 'sys_account.issuer: column exists in the database but not in metadata (orphaned) — "os migrate apply --allow-destructive" to drop it.',
 };
 
 const COLLISIONS = {
@@ -188,7 +190,10 @@ describe('controls', () => {
     const { exit } = await runApply(ARGV);
 
     expect(exit).toBe(1);
-    // Refused by the pre-flight, not by anything earlier on the human face.
+    // Refused by the pre-flight, not by anything earlier on the human face:
+    // its refusal printed, and the pre-flight read once — the listing above
+    // the gate shares the gate's read (#24958).
+    expect(log.mock.calls.map((c: unknown[]) => c.map(String).join(' ')).some((l: string) => l.includes('Refusing to drop sys_account.issuer'))).toBe(true);
     expect(mocks.probeAccountIdentityCollisions).toHaveBeenCalledTimes(1);
     expect(applyMigrationEntries).not.toHaveBeenCalled();
   }, RUN_TIMEOUT);

packages/cli/src/commands/migrate/apply.organization-ownership-refusal.test.ts

@@ -64,7 +64,9 @@ const ORGANIZATION_DROP = {
   table: 'sys_platform_setting',
   column: 'organization_id',
   op: { type: 'drop_column', column: 'organization_id' },
-  description: 'sys_platform_setting.organization_id: column exists in the database but not in metadata (orphaned)',
+  // The differ's `message` (`ManagedDriftEntry`), remedy sentence included: the
+  // listing above the gate reads it (#24958).
+  message: 'sys_platform_setting.organization_id: column exists in the database but not in metadata (orphaned) — "os migrate apply --allow-destructive" to drop it.',
 };
 
 /** The ceremony's two steps, as the refusal prescribes them. */

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

@@ -18,7 +18,6 @@ import {
 import {
   bootSchemaStack,
   collectDriftApplyLimits,
-  describeWithApplyLimits,
   describeDatabaseSource,
   describeTelemetryDatabase,
   renderPlan,
@@ -37,12 +36,16 @@ import {
 } from '../../utils/schema-migration-plugins.js';
 import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js';
 import { describeOccupancy } from '../../utils/sqlite-occupancy.js';
+import { ceremonyOrganizationColumnDrops } from '../../utils/organization-ownership-drop-gate.js';
 import {
-  COMMENT_OBJECT,
-  REACTIONS_COLUMN,
-  readCommentReactionsConversion,
-} from '../../utils/comment-reactions-conversion.js';
-import { ceremonyOrganizationColumnDrops, readOrganizationOwnershipMarker } from '../../utils/organization-ownership-drop-gate.js';
+  DropGateReads,
+  describeDriftEntries,
+  describeHeldDrops,
+  isAccountIssuerDrop,
+  isCommentReactionsDrop,
+  readHeldDrops,
+  type HeldDrops,
+} from '../../utils/apply-drop-gates.js';
 
 async function confirm(question: string): Promise<boolean> {
   if (!process.stdin.isTTY) return false; // non-interactive → require --yes
@@ -101,6 +104,13 @@ function describeNotApplied(entry: ManagedDriftEntry): string {
  *    The drift entries are applied as on any database, and **every entry the
  *    driver did not apply is named** by table, column and kind; a destructive
  *    one the operator confirmed makes the run PARTIAL and exit 1.
+ *
+ * [#24958] And it never offers `--allow-destructive` for a drop its own gates
+ * refuse here (`sys_account.issuer`, `sys_comment.reactions`, an
+ * `organization_id` drop ahead of the ceremony). The listing before the prompt
+ * names what each such drop waits for, the skip lines offer no re-run while
+ * one is held, and the gates' predicates and reads are the ones
+ * `utils/apply-drop-gates.ts` holds for `os migrate plan` too.
  */
 export default class MigrateApply extends Command {
   static override description =
@@ -337,23 +347,44 @@ export default class MigrateApply extends Command {
       // the --json document, `notApplied`, PARTIAL and the exit code never read
       // it. Empty for a driver that declares nothing, which reads as it always has.
       const limits = flags.json ? new Map<ManagedDriftEntry, string>() : collectDriftApplyLimits(stack, drift);
+      // [#24958] Each drop gate below reads at most once per run: the listing
+      // and the gate share that read, so they cannot answer differently.
+      const gateReads = new DropGateReads();
+      // [#24958] The drops those gates refuse on this database, each with the
+      // gate that refuses it — read for the human face only, like `limits`:
+      // the --json document, the gates' rule and the exit code never read it.
+      // Empty when nothing is held, and then everything below reads as it
+      // always has.
+      const kernel = stack.kernel as { getService?: (n: string) => unknown };
+      const gateHeld: HeldDrops = flags.json
+        ? new Map()
+        : await readHeldDrops(gateReads, () => kernel.getService?.call(kernel, 'objectql'), drift);
+      // A held-back entry no re-run with --allow-destructive could apply here:
+      // a driver's declared limit keeps it from any run. A gate-held entry is
+      // not one of these — it waits for the gate's own command.
+      const neverReRunnable = (d: ManagedDriftEntry) => limits.has(d) && !gateHeld.has(d);
 
       if (!flags.json) {
         // [#23925] An entry with a limit shows its finding and that limit, never
-        // --allow-destructive.
+        // --allow-destructive. [#24958] Nor does an entry a gate holds: it
+        // names what it waits for.
         await renderPendingSchemaWork(pending);
-        if (limits.size === 0) renderPlan(drift);
-        else renderPlan(drift, await describeWithApplyLimits(limits));
+        renderPlan(drift, await describeDriftEntries(limits, gateHeld));
         if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending));
         printInfo(summarize(drift));
         // [#23925] A re-run with --allow-destructive is offered only for the
         // held-back entries it could apply here; the rest are named as drift
-        // that re-run would leave as it is.
-        const reRunnable = deferred.filter((d) => !limits.has(d));
+        // that re-run would leave as it is. [#24958] While a gate holds a
+        // drop, that re-run is refused whole, so it is offered for nothing:
+        // each gate's line names the command that moves the database on.
+        const reRunnable = deferred.filter((d) => !neverReRunnable(d));
         const notReRunnable = deferred.length - reRunnable.length;
         if (reRunnable.length > 0) {
-          printWarning(`${reRunnable.length} destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).`);
+          printWarning(gateHeld.size === 0
+            ? `${reRunnable.length} destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).`
+            : `${reRunnable.length} destructive change(s) will be SKIPPED, and a re-run with --allow-destructive cannot include them yet:`);
         }
+        for (const line of describeHeldDrops(gateHeld, drift, limits)) printWarning(line);
         if (notReRunnable > 0) {
           printWarning(
             `${notReRunnable} destructive change(s) remain drift that a re-run with --allow-destructive cannot apply `
@@ -440,12 +471,11 @@ export default class MigrateApply extends Command {
       // rows indistinguishable and let a sign-in resolve onto the wrong user's
       // account — silently. Hence a row-level pre-flight, run against the live
       // database at the moment it matters.
-      const dropsAccountIssuer = intended.some(
-        (d) => d.op?.type === 'drop_column' && d.table === 'sys_account' && d.op.column === 'issuer',
-      );
+      // [#24958] Its predicate and read live in `utils/apply-drop-gates.ts`,
+      // where `os migrate plan` and the listing above ask them too.
+      const dropsAccountIssuer = intended.some(isAccountIssuerDrop);
       if (dropsAccountIssuer) {
-        const { probeAccountIdentityCollisions, formatAccountIdentityPreflightReport } =
-          await import('@objectstack/plugin-auth');
+        const { formatAccountIdentityPreflightReport } = await import('@objectstack/plugin-auth');
         const engine = (stack.kernel as { getService?: (n: string) => unknown }).getService?.call(
           stack.kernel,
           'objectql',
@@ -456,7 +486,7 @@ export default class MigrateApply extends Command {
         // caller gating on the status. Each emitted with 0 until this card.
         let preflight;
         try {
-          preflight = await probeAccountIdentityCollisions(engine as never);
+          preflight = await gateReads.accountIssuerPreflight(engine);
         } catch (e: any) {
           // A pre-flight that could not read is not a pre-flight that passed.
           const detail = e?.message ?? String(e);
@@ -513,16 +543,16 @@ export default class MigrateApply extends Command {
       // converted, a member may delete their own reaction, and a live probe
       // would read that as unconverted (`utils/comment-reactions-conversion.ts`
       // says why). The boot drift report and the retirement's upgrade note name
-      // this very command as the remedy, so both arrive here.
-      const dropsCommentReactions = intended.some(
-        (d) => d.op?.type === 'drop_column' && d.table === COMMENT_OBJECT && d.op.column === REACTIONS_COLUMN,
-      );
+      // this very command as the remedy, so both arrive here. [#24958] Its
+      // predicate and read live in `utils/apply-drop-gates.ts`, where
+      // `os migrate plan` and the listing above ask them too.
+      const dropsCommentReactions = intended.some(isCommentReactionsDrop);
       if (dropsCommentReactions) {
         const engine = (stack.kernel as { getService?: (n: string) => unknown }).getService?.call(
           stack.kernel,
           'objectql',
         );
-        const conversion = await readCommentReactionsConversion(engine as never);
+        const conversion = await gateReads.commentReactionsConversion(engine);
         if (!conversion.verified) {
           const detail = conversion.flag
             ? `The last run of "os migrate comment-reactions --apply" (${conversion.flag.last_run_at}) left `
@@ -569,7 +599,7 @@ export default class MigrateApply extends Command {
           stack.kernel,
           'objectql',
         );
-        const marker = await readOrganizationOwnershipMarker(engine as never);
+        const marker = await gateReads.organizationOwnershipMarker(engine);
         if (!marker.verified) {
           // The ceremony is the way on (18.x step 6): the refusal names its
           // steps, as the boot refusal does (`organizationOwnershipRefusalMessage`,
@@ -688,12 +718,16 @@ export default class MigrateApply extends Command {
       printSuccess(`Applied ${applied.length} change(s).`);
       // [#23925] Split as the count before the prompt splits `deferred`: a re-run
       // is offered only for the held-back entries it could apply here, and the
-      // rest are named as drift it could not apply either.
+      // rest are named as drift it could not apply either. [#24958] While a
+      // gate holds a drop, no re-run is offered: the lines before the prompt
+      // name each gate's command.
       const heldBackEntries = skipped.filter((d) => held.has(d));
-      const heldBack = heldBackEntries.filter((d) => !limits.has(d)).length;
+      const heldBack = heldBackEntries.filter((d) => !neverReRunnable(d)).length;
       const heldBackNotReRunnable = heldBackEntries.length - heldBack;
       if (heldBack > 0) {
-        printWarning(`Skipped ${heldBack} destructive change(s) (re-run with --allow-destructive to include them).`);
+        printWarning(gateHeld.size === 0
+          ? `Skipped ${heldBack} destructive change(s) (re-run with --allow-destructive to include them).`
+          : `Skipped ${heldBack} destructive change(s); a re-run with --allow-destructive cannot include them yet (see above).`);
       }
       if (heldBackNotReRunnable > 0) {
         printWarning(