Skip to content

Commit 420c3fe

Browse files
committed
fix(cli): migrate plan and apply name each drop gate's command, never --allow-destructive, for a drop apply refuses
The sys_account.issuer and sys_comment.reactions predicates move out of apply.ts into utils/apply-drop-gates.ts beside the organization_id gate's, with one read per gate per run shared by the listing and the gate. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6
1 parent 62bc129 commit 420c3fe

3 files changed

Lines changed: 359 additions & 98 deletions

File tree

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

Lines changed: 61 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import {
1818
import {
1919
bootSchemaStack,
2020
collectDriftApplyLimits,
21-
describeWithApplyLimits,
2221
describeDatabaseSource,
2322
describeTelemetryDatabase,
2423
renderPlan,
@@ -37,12 +36,16 @@ import {
3736
} from '../../utils/schema-migration-plugins.js';
3837
import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js';
3938
import { describeOccupancy } from '../../utils/sqlite-occupancy.js';
39+
import { ceremonyOrganizationColumnDrops } from '../../utils/organization-ownership-drop-gate.js';
4040
import {
41-
COMMENT_OBJECT,
42-
REACTIONS_COLUMN,
43-
readCommentReactionsConversion,
44-
} from '../../utils/comment-reactions-conversion.js';
45-
import { ceremonyOrganizationColumnDrops, readOrganizationOwnershipMarker } from '../../utils/organization-ownership-drop-gate.js';
41+
DropGateReads,
42+
describeDriftEntries,
43+
describeHeldDrops,
44+
isAccountIssuerDrop,
45+
isCommentReactionsDrop,
46+
readHeldDrops,
47+
type HeldDrops,
48+
} from '../../utils/apply-drop-gates.js';
4649

4750
async function confirm(question: string): Promise<boolean> {
4851
if (!process.stdin.isTTY) return false; // non-interactive → require --yes
@@ -101,6 +104,13 @@ function describeNotApplied(entry: ManagedDriftEntry): string {
101104
* The drift entries are applied as on any database, and **every entry the
102105
* driver did not apply is named** by table, column and kind; a destructive
103106
* one the operator confirmed makes the run PARTIAL and exit 1.
107+
*
108+
* [#24958] And it never offers `--allow-destructive` for a drop its own gates
109+
* refuse here (`sys_account.issuer`, `sys_comment.reactions`, an
110+
* `organization_id` drop ahead of the ceremony). The listing before the prompt
111+
* names what each such drop waits for, the skip lines offer no re-run while
112+
* one is held, and the gates' predicates and reads are the ones
113+
* `utils/apply-drop-gates.ts` holds for `os migrate plan` too.
104114
*/
105115
export default class MigrateApply extends Command {
106116
static override description =
@@ -337,23 +347,44 @@ export default class MigrateApply extends Command {
337347
// the --json document, `notApplied`, PARTIAL and the exit code never read
338348
// it. Empty for a driver that declares nothing, which reads as it always has.
339349
const limits = flags.json ? new Map<ManagedDriftEntry, string>() : collectDriftApplyLimits(stack, drift);
350+
// [#24958] Each drop gate below reads at most once per run: the listing
351+
// and the gate share that read, so they cannot answer differently.
352+
const gateReads = new DropGateReads();
353+
// [#24958] The drops those gates refuse on this database, each with the
354+
// gate that refuses it — read for the human face only, like `limits`:
355+
// the --json document, the gates' rule and the exit code never read it.
356+
// Empty when nothing is held, and then everything below reads as it
357+
// always has.
358+
const kernel = stack.kernel as { getService?: (n: string) => unknown };
359+
const gateHeld: HeldDrops = flags.json
360+
? new Map()
361+
: await readHeldDrops(gateReads, () => kernel.getService?.call(kernel, 'objectql'), drift);
362+
// A held-back entry no re-run with --allow-destructive could apply here:
363+
// a driver's declared limit keeps it from any run. A gate-held entry is
364+
// not one of these — it waits for the gate's own command.
365+
const neverReRunnable = (d: ManagedDriftEntry) => limits.has(d) && !gateHeld.has(d);
340366

341367
if (!flags.json) {
342368
// [#23925] An entry with a limit shows its finding and that limit, never
343-
// --allow-destructive.
369+
// --allow-destructive. [#24958] Nor does an entry a gate holds: it
370+
// names what it waits for.
344371
await renderPendingSchemaWork(pending);
345-
if (limits.size === 0) renderPlan(drift);
346-
else renderPlan(drift, await describeWithApplyLimits(limits));
372+
renderPlan(drift, await describeDriftEntries(limits, gateHeld));
347373
if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending));
348374
printInfo(summarize(drift));
349375
// [#23925] A re-run with --allow-destructive is offered only for the
350376
// held-back entries it could apply here; the rest are named as drift
351-
// that re-run would leave as it is.
352-
const reRunnable = deferred.filter((d) => !limits.has(d));
377+
// that re-run would leave as it is. [#24958] While a gate holds a
378+
// drop, that re-run is refused whole, so it is offered for nothing:
379+
// each gate's line names the command that moves the database on.
380+
const reRunnable = deferred.filter((d) => !neverReRunnable(d));
353381
const notReRunnable = deferred.length - reRunnable.length;
354382
if (reRunnable.length > 0) {
355-
printWarning(`${reRunnable.length} destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).`);
383+
printWarning(gateHeld.size === 0
384+
? `${reRunnable.length} destructive change(s) will be SKIPPED (re-run with --allow-destructive to include them).`
385+
: `${reRunnable.length} destructive change(s) will be SKIPPED, and a re-run with --allow-destructive cannot include them yet:`);
356386
}
387+
for (const line of describeHeldDrops(gateHeld, drift, limits)) printWarning(line);
357388
if (notReRunnable > 0) {
358389
printWarning(
359390
`${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 {
440471
// rows indistinguishable and let a sign-in resolve onto the wrong user's
441472
// account — silently. Hence a row-level pre-flight, run against the live
442473
// database at the moment it matters.
443-
const dropsAccountIssuer = intended.some(
444-
(d) => d.op?.type === 'drop_column' && d.table === 'sys_account' && d.op.column === 'issuer',
445-
);
474+
// [#24958] Its predicate and read live in `utils/apply-drop-gates.ts`,
475+
// where `os migrate plan` and the listing above ask them too.
476+
const dropsAccountIssuer = intended.some(isAccountIssuerDrop);
446477
if (dropsAccountIssuer) {
447-
const { probeAccountIdentityCollisions, formatAccountIdentityPreflightReport } =
448-
await import('@objectstack/plugin-auth');
478+
const { formatAccountIdentityPreflightReport } = await import('@objectstack/plugin-auth');
449479
const engine = (stack.kernel as { getService?: (n: string) => unknown }).getService?.call(
450480
stack.kernel,
451481
'objectql',
@@ -456,7 +486,7 @@ export default class MigrateApply extends Command {
456486
// caller gating on the status. Each emitted with 0 until this card.
457487
let preflight;
458488
try {
459-
preflight = await probeAccountIdentityCollisions(engine as never);
489+
preflight = await gateReads.accountIssuerPreflight(engine);
460490
} catch (e: any) {
461491
// A pre-flight that could not read is not a pre-flight that passed.
462492
const detail = e?.message ?? String(e);
@@ -513,16 +543,16 @@ export default class MigrateApply extends Command {
513543
// converted, a member may delete their own reaction, and a live probe
514544
// would read that as unconverted (`utils/comment-reactions-conversion.ts`
515545
// says why). The boot drift report and the retirement's upgrade note name
516-
// this very command as the remedy, so both arrive here.
517-
const dropsCommentReactions = intended.some(
518-
(d) => d.op?.type === 'drop_column' && d.table === COMMENT_OBJECT && d.op.column === REACTIONS_COLUMN,
519-
);
546+
// this very command as the remedy, so both arrive here. [#24958] Its
547+
// predicate and read live in `utils/apply-drop-gates.ts`, where
548+
// `os migrate plan` and the listing above ask them too.
549+
const dropsCommentReactions = intended.some(isCommentReactionsDrop);
520550
if (dropsCommentReactions) {
521551
const engine = (stack.kernel as { getService?: (n: string) => unknown }).getService?.call(
522552
stack.kernel,
523553
'objectql',
524554
);
525-
const conversion = await readCommentReactionsConversion(engine as never);
555+
const conversion = await gateReads.commentReactionsConversion(engine);
526556
if (!conversion.verified) {
527557
const detail = conversion.flag
528558
? `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 {
569599
stack.kernel,
570600
'objectql',
571601
);
572-
const marker = await readOrganizationOwnershipMarker(engine as never);
602+
const marker = await gateReads.organizationOwnershipMarker(engine);
573603
if (!marker.verified) {
574604
// The ceremony is the way on (18.x step 6): the refusal names its
575605
// steps, as the boot refusal does (`organizationOwnershipRefusalMessage`,
@@ -688,12 +718,16 @@ export default class MigrateApply extends Command {
688718
printSuccess(`Applied ${applied.length} change(s).`);
689719
// [#23925] Split as the count before the prompt splits `deferred`: a re-run
690720
// is offered only for the held-back entries it could apply here, and the
691-
// rest are named as drift it could not apply either.
721+
// rest are named as drift it could not apply either. [#24958] While a
722+
// gate holds a drop, no re-run is offered: the lines before the prompt
723+
// name each gate's command.
692724
const heldBackEntries = skipped.filter((d) => held.has(d));
693-
const heldBack = heldBackEntries.filter((d) => !limits.has(d)).length;
725+
const heldBack = heldBackEntries.filter((d) => !neverReRunnable(d)).length;
694726
const heldBackNotReRunnable = heldBackEntries.length - heldBack;
695727
if (heldBack > 0) {
696-
printWarning(`Skipped ${heldBack} destructive change(s) (re-run with --allow-destructive to include them).`);
728+
printWarning(gateHeld.size === 0
729+
? `Skipped ${heldBack} destructive change(s) (re-run with --allow-destructive to include them).`
730+
: `Skipped ${heldBack} destructive change(s); a re-run with --allow-destructive cannot include them yet (see above).`);
697731
}
698732
if (heldBackNotReRunnable > 0) {
699733
printWarning(

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

Lines changed: 26 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import { Command, Flags } from '@oclif/core';
44
import chalk from 'chalk';
5-
import type { ManagedDriftEntry } from '@objectstack/driver-sql';
65
import {
76
printHeader,
87
printSuccess,
@@ -17,7 +16,6 @@ import {
1716
import {
1817
bootSchemaStack,
1918
collectDriftApplyLimits,
20-
describeWithApplyLimits,
2119
describeDatabaseSource,
2220
describeTelemetryDatabase,
2321
distinctDatabaseDrivers,
@@ -40,7 +38,12 @@ import {
4038
} from '../../utils/unmanaged-tables.js';
4139
import { describeOccupancy, JOURNAL_MODE_SWITCH_NOTE } from '../../utils/sqlite-occupancy.js';
4240
import { describeDriverSqliteFile } from '../../utils/connection-display.js';
43-
import { ceremonyOrganizationColumnDrops, readOrganizationOwnershipMarker } from '../../utils/organization-ownership-drop-gate.js';
41+
import {
42+
DropGateReads,
43+
describeDriftEntries,
44+
describeHeldDrops,
45+
readHeldDrops,
46+
} from '../../utils/apply-drop-gates.js';
4447
import {
4548
resolveTenancyPosture,
4649
collectGlobalUniques,
@@ -97,6 +100,13 @@ import {
97100
* ceremony verified, are read through the two functions `apply` asks, never a
98101
* second list. A verified database, or one with no such drop, gets the plan it
99102
* always got, byte for byte; the `--json` document is unchanged.
103+
*
104+
* [#24958] The same holds for every drop `apply` refuses, not only the
105+
* ceremony's: `sys_account.issuer` while its retirement pre-flight does not
106+
* pass, and `sys_comment.reactions` while no verified run of
107+
* `os migrate comment-reactions --apply` is recorded. Each gate's predicate
108+
* and read live in `utils/apply-drop-gates.ts`, which `apply` asks too; a held
109+
* drop names what it waits for, and the footer names each gate's command.
100110
*/
101111
export default class MigratePlan extends Command {
102112
static override description =
@@ -419,21 +429,20 @@ export default class MigratePlan extends Command {
419429
// but a drop (it runs no SQLite table rebuild). Empty for a driver that
420430
// declares nothing, and then everything below reads as it always has.
421431
const limits = collectDriftApplyLimits(stack, drift);
422-
// [#24925] The `organization_id` drops `os migrate apply` refuses here
423-
// until the organization-ownership ceremony has verified. Empty on a
424-
// verified database and on one with no such drop, and then everything
432+
// [#24925, #24958] The drops `os migrate apply` refuses here, each with
433+
// the gate that refuses it: an `organization_id` drop until the
434+
// organization-ownership ceremony has verified, and the
435+
// `sys_account.issuer` / `sys_comment.reactions` drops until their own
436+
// preconditions hold. Read through the gates' own predicates and reads,
437+
// never a second list. Empty when nothing is held, and then everything
425438
// below reads as it always has.
426-
const held = await ceremonyHeldDrops(stack, drift);
439+
const kernel = stack.kernel as { getService?: (n: string) => unknown };
440+
const held = await readHeldDrops(new DropGateReads(), () => kernel.getService?.call(kernel, 'objectql'), drift);
427441

428442
await renderPendingSchemaWork(pending);
429-
if (limits.size === 0 && held.size === 0) {
430-
renderPlan(drift);
431-
} else {
432-
// Still listed, still counted: it IS drift. Only the remedy changes,
433-
// from a command that cannot perform it here to the reason it cannot.
434-
const byLimit = limits.size === 0 ? (d: ManagedDriftEntry) => d.message : await describeWithApplyLimits(limits);
435-
renderPlan(drift, held.size === 0 ? byLimit : await describeWithCeremonyHolds(held, byLimit));
436-
}
443+
// Still listed, still counted: it IS drift. Only the remedy changes,
444+
// from a command that cannot perform it here to the reason it cannot.
445+
renderPlan(drift, await describeDriftEntries(limits, held));
437446
if (pending.length > 0) printInfo(await summarizePendingSchemaWork(pending));
438447
printInfo(summarize(drift));
439448
if (limits.size === 0 && held.size === 0) {
@@ -444,7 +453,7 @@ export default class MigratePlan extends Command {
444453
// apply; no apply line at all when it can apply nothing listed here.
445454
const appliable = drift.filter((d) => !limits.has(d) && !held.has(d));
446455
const appliableDestructive = appliable.filter((d) => d.category === 'destructive').length;
447-
// [#24925] While the ceremony holds a drop, `apply` refuses every
456+
// [#24925, #24958] While a gate holds a drop, `apply` refuses every
448457
// --allow-destructive run, so it is offered for nothing: what `apply`
449458
// performs now is the rest, and the destructive changes wait.
450459
const appliableNow = held.size === 0 ? appliable : appliable.filter((d) => d.category !== 'destructive');
@@ -453,7 +462,7 @@ export default class MigratePlan extends Command {
453462
? chalk.dim(` (add --allow-destructive for the ${appliableDestructive} destructive change(s) it can apply on this database)`)
454463
: ''));
455464
}
456-
if (held.size > 0) printWarning(describeCeremonyHold(held.size, appliableDestructive));
465+
for (const line of describeHeldDrops(held, drift, limits)) printWarning(line);
457466
const limited = [...limits.keys()].filter((d) => !held.has(d)).length;
458467
if (limited > 0) {
459468
printWarning(
@@ -473,57 +482,3 @@ export default class MigratePlan extends Command {
473482
}
474483
}
475484
}
476-
477-
/**
478-
* [#24925] The entries `os migrate apply` refuses on this database: each
479-
* `organization_id` drop, while the organization-ownership ceremony has not
480-
* verified. Answered by the two functions `apply`'s drop gate asks, through
481-
* the same `objectql` service, so the plan and the refusal cannot disagree:
482-
* `ceremonyOrganizationColumnDrops`, asked of one entry at a time, says which
483-
* entries; `readOrganizationOwnershipMarker` says whether the ceremony
484-
* verified, and answers "not verified" for a database with no `sys_migration`
485-
* table or row. The marker is read only when such a drop is listed, so every
486-
* other plan reads nothing it did not read before.
487-
*/
488-
async function ceremonyHeldDrops(
489-
stack: { kernel: unknown },
490-
drift: readonly ManagedDriftEntry[],
491-
): Promise<ReadonlySet<ManagedDriftEntry>> {
492-
const drops = drift.filter((d) => ceremonyOrganizationColumnDrops([d]).length > 0);
493-
if (drops.length === 0) return new Set();
494-
const kernel = stack.kernel as { getService?: (n: string) => unknown };
495-
const engine = kernel.getService?.call(kernel, 'objectql');
496-
const marker = await readOrganizationOwnershipMarker(engine as never);
497-
return marker.verified ? new Set() : new Set(drops);
498-
}
499-
500-
/**
501-
* [#24925] The {@link renderPlan} line for a held drop: the finding without
502-
* its `--allow-destructive` sentence, then the ceremony as its remedy. Every
503-
* other entry is described by `otherwise`. The driver package is imported at
504-
* the point of use, never statically (`schema-migrate.lazy-driver-import.test.ts`).
505-
*/
506-
async function describeWithCeremonyHolds(
507-
held: ReadonlySet<ManagedDriftEntry>,
508-
otherwise: (d: ManagedDriftEntry) => string,
509-
): Promise<(d: ManagedDriftEntry) => string> {
510-
const { driftFindingWithoutRemedy } = await import('@objectstack/driver-sql');
511-
return (d) => (held.has(d)
512-
? `${driftFindingWithoutRemedy(d)} — a drop the organization-ownership ceremony owns, and it has not verified `
513-
+ 'on this database: the ceremony drops it itself (see below).'
514-
: otherwise(d));
515-
}
516-
517-
/**
518-
* [#24925] The footer line for the held drops: why `--allow-destructive` is
519-
* offered for nothing, and the ceremony's two commands, as `os migrate apply`'s
520-
* refusal names them. `waiting` counts the other destructive entries that run
521-
* could perform once the ceremony has verified.
522-
*/
523-
function describeCeremonyHold(drops: number, waiting: number): string {
524-
return `${drops} organization_id drop(s) above are the organization-ownership ceremony's, and it has not verified `
525-
+ 'on this database, so "os migrate apply --allow-destructive" refuses every run until it has'
526-
+ (waiting > 0 ? ` (the other ${waiting} destructive change(s) above wait for it too)` : '')
527-
+ '. Run "os migrate organization-ownership --out PLAN" to write and review its plan, take a backup, then '
528-
+ '"os migrate organization-ownership --apply --plan-file PLAN --backup-taken"; it drops these columns itself.';
529-
}

0 commit comments

Comments
 (0)