Commit f20f669
fix(cli): os migrate plan / apply run no app onEnable and no post-declaration host hooks (#21138)
Fixes #21054
Clause-②: yes (widening)
## What was wrong
`os migrate plan` and `os migrate apply` boot the host's stack to read
what it declares. That boot also ran host code that has nothing to do
with declarations:
- the config's `onEnable`, which runs from the `AppPlugin` the migrate
composition builds out of `objectstack.config.ts`. That `AppPlugin` is
not wrapped by `composeForDeclarations`.
- every `kernel:bootstrapped` / `kernel:listening` hook that a host
plugin registers from `init()`. `composeForDeclarations` suppressed
`start()` and nothing else.
`examples/app-crm`'s `onEnable` hooks `kernel:bootstrapped` and reads
`sys_position` / `sys_permission_set`. The plan's composition never
declares those tables. The write guard from the earlier
write-suppression work refuses writes and lets hooks run, so it cannot
stop a read. Every plan therefore printed 6 `DATABASE_ERROR` lines on
stderr and 6 `position binding lookup failed` warnings, on a migrated
file and on an absent one.
## The fix: one point per door, for every app
Host code enters a declaration boot through exactly two doors, and each
is closed where it enters:
1. **The config's `onEnable`.** `AppPlugin` gets a `skipOnEnable`
option, a sibling of the existing `skipSeedData`, which serves the same
commands. With it set, `start()` does not run `onEnable`. It logs
`runtime.onEnable NOT executed …` and reports the skip through
`onEnableWithheld`. The migrate composition sets the option on the app
it builds.
- The option lives in the executor
(`packages/runtime/src/app-plugin.ts`) and not in a stripped copy of the
bundle. The executor is what resolves which object carries the hook
(`bundle.default` before the bundle itself). A copy would re-state that
rule at the CLI call site. The boot would then also log "No
runtime.onEnable function found" about an app that has one.
- A compiled artifact cannot carry `onEnable` at all: it is JSON, and
its runtime module contributes `functions` only. A host plugin that is
itself an `AppPlugin` is already wrapped, so its whole `start()` is
suppressed.
2. **A host plugin's `init()`.** `composeForDeclarations` now forwards
`init` with a context whose `hook()` does not register
`kernel:bootstrapped` or `kernel:listening`. A host that keeps that
context and registers later is declined too.
- The two phases come from the kernel contract
(`IPluginLifecycleEvents`). `kernel:bootstrapped` is for
"reconcile/backfill work that consumes" data. `kernel:listening` comes
after every plugin "has had a chance to register routes / services /
middleware during `kernel:ready`". Both say that registration is over.
- `kernel:ready` is deliberately kept. The contract puts late
registration there, and a host that provisions its tables from a
`kernel:ready` hook is a measured shape whose tables the plan must see.
The write guard still refuses row writes on `kernel:ready`.
- `kernel:shutdown` hooks, data hooks and custom events register as
before.
This repo's own plugins (the data stack, `PlatformObjectsPlugin`, the
guard, and `extraPlugins`) are not host code and are untouched. The plan
still prints the value-shape gate announcement that the engine makes
from its own `kernel:bootstrapped` hook. No in-repo plugin registers a
post-declaration hook from `init()`: all seven sites are in `start()`.
The plan's notes, and `composition.notes` in `--json`, carry one line
naming what was not run. A host with nothing withheld gets no line.
**Stop clause (does the plan need an app hook for its declarations?)**
No. On app-crm, the table list, the pending DDL, the drift and `--json`
(all but `notes`) are identical before and after; see below.
### The earlier design, and how this changes it
The write-suppression card chose to refuse writes at the driver over
neutralising `init()`-registered hooks. One reason was that a log-only
hook should keep running on the plan path. Triage's direction on this
card (comment 5924795251) sets the boundary more narrowly: the
declaration boot does not fire app `onEnable` / `kernel:bootstrapped`
hooks. The guard stays the write guarantee on every phase. Only the two
post-declaration phases are now withheld for host code. The existing
pins that asserted host log-only hooks run on those two phases were
inverted in place, and their writers moved to `kernel:ready` where the
case was about the guard rather than the phase. A new pin keeps the
guard's phase-agnostic property: a writer the composition does not wrap
is refused on all three phases and in `start()`.
## Release grading
`@objectstack/runtime` takes `minor`, and this PR declares `Clause-②:
yes (widening)`. `AppPlugin`, exported from the package root, gains the
optional constructor option `skipOnEnable` (default `false`) and the
read-only getter `onEnableWithheld`. That is an additive widening of a
published surface, which takes at least `minor`. `@objectstack/cli`
stays `patch`. This was re-graded from `patch` / `Clause-②: no` after
the contract review record 5928867906, in the changeset-only commit
`afc44ba5bf`.
## Measured on `examples/app-crm`
`node packages/cli/bin/run.js migrate …` from `examples/app-crm`, with
no `dist/` artifact. Base is `origin/main` `9c8b65aa23`, built. Fix is
`af9ac5ded3`, with runtime and cli rebuilt.
| run | base: `DATABASE_ERROR` (stderr) | base: `position binding lookup
failed` | base: onEnable executed | fix: `DATABASE_ERROR` | fix: lookup
failed | fix: onEnable executed |
|---|---|---|---|---|---|---|
| `plan` on an absent file | 6 | 6 | yes | 0 | 0 | no (withheld, logged)
|
| `plan --json`, absent file | 6 | 6 | yes | 0 | 0 | no |
| `apply --yes` | 6 | 6 | yes | 0 | 0 | no |
| `plan` on the migrated file | 6 | 6 | yes | 0 | 0 | no |
| `plan --json`, migrated file | 6 | 6 | yes | 0 | 0 | no |
- On the base, stderr carries those 6 errors plus 2 "Paged read … NOT
deterministic" warns from the same hook: 8 lines. On the fix, stderr is
empty on every run.
- The plan output does not change. The non-log stdout differs by exactly
one added notes line (`diff` shows 1 line added and 0 removed, for plan
absent, plan migrated and apply). `--json` is identical except
`composition.notes` (2 to 3 entries): `pending` 15/15 (absent) and 0/0
(migrated), `total` 0, `managedTables` 15. `Examined 15 managed
table(s)` holds on both. The absent file is not created.
## Tests
- `@objectstack/runtime` `src/app-plugin.test.ts`: the `skipOnEnable`
pins. The hook is withheld, logged and reported, including when it sits
on `bundle.default`. A bundle with no `onEnable` reports nothing
withheld.
- `@objectstack/cli` unit, `schema-migration-plugins.test.ts`:
- the `init()` context declines the two phases and forwards
`kernel:ready`, `kernel:shutdown`, data hooks and every other member;
- the composed app carries `skipOnEnable`, its `onEnable` does not run,
and the lifecycle names it.
- `@objectstack/cli` integration,
`schema-migration-plugins.declaration-boot-write-guard.test.ts`, using a
real `ObjectKernel`:
- the positive control fires all three phases;
- the fix fires only `kernel:ready` for host code, keeps the teardown,
and leaves an unwrapped platform plugin on all three phases in the same
boot;
- a host that registers later from `kernel:ready` is declined;
- the existing write-guard pins were updated as described above.
- `@objectstack/cli` integration,
`schema-migrate.host-composition.integration.test.ts`, new block for
this card. It uses an app-crm-shaped fixture: a stack with one object, a
named `onEnable` that hooks `kernel:bootstrapped` and reads the two
undeclared tables, and a host plugin with a reading `init()`-registered
`kernel:bootstrapped` hook.
- POSITIVE CONTROL: the same code composed as `serve` composes it prints
the lines.
- CONTROL: `apply`'s confirmed DDL flush still creates the app's table,
and the coverage pass still examines it.
- The plan on the migrated file prints 0 `DATABASE_ERROR`, runs neither
hook, and still runs the host's `kernel:ready` hook.
- The plan on an absent file prints 0 `DATABASE_ERROR` and leaves no
file behind.
Runs:
- At `3781713631`:
- runtime `vitest run --project local`: 297 files, 4252 passed, 5
skipped.
- cli `--project unit`: 240 files, 3422 passed.
- cli `--project integration` over the 11 migrate-related files: 59
passed.
- After merging `origin/main`:
- at `5bd79b1b3c`: runtime `app-plugin.test.ts` 35/35; cli unit file
32/32; write-guard, host-composition and `plan.deferred-reads` 36/36
(integration); `typecheck` (including `check:test-typecheck`) green for
runtime and cli;
- at `6d4ef7c9aa`: host-composition 14/14 and cli `typecheck` green,
after the test-only fix that `check:test-source-alias` asked for;
- the head `dc1c40ec39` differs from `6d4ef7c9aa` by one comment line.
**Gates, at `dc1c40ec39`.**
- `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
--commands` derived 64 commands. All 64 ran with their exit codes
recorded before any pipe, and all 64 exited 0. `--ran` reports "64
derived, 64 run, 0 NOT-MEASURED, 0 UNRUN".
- `check:dual-build-cjs-loads` and `check:i18n-coverage` measured, after
the nine packages they read were built.
- `check:test-source-alias` turned red on the first pass: a dynamic
`import('@objectstack/runtime')` sat inside a test body. Moving it to
module top made it green.
**Gates, at `afc44ba5bf` (changeset-only commit).** The diff from
`dc1c40ec39` is the one changeset file, so the code families keep their
`dc1c40ec39` results.
- The 19 families whose derivation names the changeset path, or that
declare a whole-tree population, ran again: 19 of 19 exited 0.
- `check-changeset-no-major` in event mode with this body: the level
axis is green ("`@objectstack/runtime: minor` … the declared widening is
accounted for"). The control, the same body against `dc1c40ec39` where
runtime was `patch`, exits 1.
- `--ran` over the fresh 19 plus the 45 carried from `dc1c40ec39`: "64
derived, 64 run, 0 NOT-MEASURED, 0 UNRUN".
**Lint, a proven narrowing.**
- Command: `eslint --no-inline-config --format json` over the 7 touched
TypeScript files.
- Result: 7 files, 0 errors, 0 warnings.
- `--print-config` resolves a config for all 7, with 5 to 6 rules and no
`parserOptions.project`.
- The repo's `eslint.config.mjs` enables no type-aware linting, so this
diff cannot move the verdict on any untouched file. The full `pnpm lint`
is left to CI.
**Ablations.** The fix was committed first. Each run went through
`scripts/ablation-replace.mjs`: the anchor moved 1 to 0, the blob
changed, and the restore brought the blob back equal to HEAD with an
empty `git diff HEAD`.
- A1: `POST_DECLARATION_PHASES` emptied in the CLI source.
- Red: unit 2/32. Integration 9/34: the write-guard DEFECT, FIX and
embedder cases, both #21054 kernel cases, the #13332 FIX and R1 cases,
and both #21054 plan cases. The plan cases fail on `DATABASE_ERROR …
'sys_position'` from the host hook.
- Green: every positive control, the phase-agnostic pin and the apply
control.
- A2: `skipOnEnable: true` changed to `false` at the composition.
- Red: the unit compose case, and both #21054 plan cases
(`DATABASE_ERROR` on `sys_position` and `sys_permission_set` from
`onEnable`).
- Green: everything else.
- A3: the executor branch in `app-plugin.ts` neutralised with a planted
marker. The runtime was rebuilt, and `ablation-dist-preflight` found the
marker in 2 built files.
- Red: runtime 2/35 (the two withhold cases), the cli unit compose case,
and both #21054 plan cases.
- Restore leg: rebuilt; `--absent` found the marker in none of the 6
built files and the tree clean. Everything was green again (35, 32, 34).
## Acceptance notes
- **The pin uses an app-crm-shaped fixture, not `examples/app-crm`
itself.** A cli test that reads another package's tree is a
cross-package test input. Declaring it would mean editing
`scripts/cross-package-test-inputs.mjs` and `turbo.json`, which are
outside this card's file surface. The real app-crm is measured by the
CLI runs in the table above.
- **Residue, stated in the module header.** A host that registers a hook
without the context its `init()` received is outside the composition's
reach: through `getKernel()`, or from a service factory, which the
kernel calls with its own context. Its writes still meet the guard.
- **Residue: declarations on a post-declaration phase.** A host that
declares objects from a `kernel:bootstrapped` / `kernel:listening` hook
would lose them from the plan. The contract says registration is over by
then, and no in-repo plugin does it.
- `examples/**` and driver-sql are untouched. `#20821` is not reopened
here; its deferred-DDL demotion is unchanged.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 58a77db commit f20f669
8 files changed
Lines changed: 913 additions & 60 deletions
File tree
- .changeset
- packages
- cli/src/utils
- runtime/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
Lines changed: 245 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
4 | | - | |
| 3 | + | |
| 4 | + | |
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
8 | 12 | | |
9 | 13 | | |
10 | 14 | | |
| |||
694 | 698 | | |
695 | 699 | | |
696 | 700 | | |
| 701 | + | |
| 702 | + | |
| 703 | + | |
697 | 704 | | |
698 | | - | |
699 | 705 | | |
700 | 706 | | |
701 | 707 | | |
702 | | - | |
703 | 708 | | |
704 | 709 | | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
705 | 719 | | |
706 | 720 | | |
707 | 721 | | |
708 | 722 | | |
| 723 | + | |
709 | 724 | | |
710 | | - | |
| 725 | + | |
711 | 726 | | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
712 | 733 | | |
713 | 734 | | |
714 | 735 | | |
| |||
770 | 791 | | |
771 | 792 | | |
772 | 793 | | |
773 | | - | |
774 | | - | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
775 | 797 | | |
776 | | - | |
| 798 | + | |
777 | 799 | | |
778 | 800 | | |
779 | 801 | | |
| |||
786 | 808 | | |
787 | 809 | | |
788 | 810 | | |
| 811 | + | |
| 812 | + | |
| 813 | + | |
| 814 | + | |
| 815 | + | |
| 816 | + | |
| 817 | + | |
| 818 | + | |
| 819 | + | |
| 820 | + | |
| 821 | + | |
| 822 | + | |
| 823 | + | |
| 824 | + | |
| 825 | + | |
| 826 | + | |
| 827 | + | |
| 828 | + | |
| 829 | + | |
| 830 | + | |
| 831 | + | |
| 832 | + | |
| 833 | + | |
| 834 | + | |
| 835 | + | |
| 836 | + | |
| 837 | + | |
| 838 | + | |
| 839 | + | |
| 840 | + | |
| 841 | + | |
| 842 | + | |
| 843 | + | |
| 844 | + | |
| 845 | + | |
| 846 | + | |
| 847 | + | |
| 848 | + | |
| 849 | + | |
| 850 | + | |
| 851 | + | |
| 852 | + | |
| 853 | + | |
| 854 | + | |
| 855 | + | |
| 856 | + | |
| 857 | + | |
| 858 | + | |
| 859 | + | |
| 860 | + | |
| 861 | + | |
| 862 | + | |
| 863 | + | |
| 864 | + | |
| 865 | + | |
| 866 | + | |
| 867 | + | |
| 868 | + | |
| 869 | + | |
| 870 | + | |
| 871 | + | |
| 872 | + | |
| 873 | + | |
| 874 | + | |
| 875 | + | |
| 876 | + | |
| 877 | + | |
| 878 | + | |
| 879 | + | |
| 880 | + | |
| 881 | + | |
| 882 | + | |
| 883 | + | |
| 884 | + | |
| 885 | + | |
| 886 | + | |
| 887 | + | |
| 888 | + | |
| 889 | + | |
| 890 | + | |
| 891 | + | |
| 892 | + | |
| 893 | + | |
| 894 | + | |
| 895 | + | |
| 896 | + | |
| 897 | + | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
| 901 | + | |
| 902 | + | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
| 906 | + | |
| 907 | + | |
| 908 | + | |
| 909 | + | |
| 910 | + | |
| 911 | + | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
| 924 | + | |
| 925 | + | |
| 926 | + | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
| 951 | + | |
| 952 | + | |
| 953 | + | |
| 954 | + | |
| 955 | + | |
| 956 | + | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
| 963 | + | |
| 964 | + | |
| 965 | + | |
| 966 | + | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
| 971 | + | |
| 972 | + | |
| 973 | + | |
| 974 | + | |
| 975 | + | |
| 976 | + | |
| 977 | + | |
| 978 | + | |
| 979 | + | |
| 980 | + | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
| 985 | + | |
| 986 | + | |
| 987 | + | |
| 988 | + | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
| 992 | + | |
| 993 | + | |
| 994 | + | |
| 995 | + | |
| 996 | + | |
| 997 | + | |
| 998 | + | |
| 999 | + | |
| 1000 | + | |
| 1001 | + | |
| 1002 | + | |
| 1003 | + | |
| 1004 | + | |
| 1005 | + | |
| 1006 | + | |
| 1007 | + | |
| 1008 | + | |
| 1009 | + | |
| 1010 | + | |
| 1011 | + | |
| 1012 | + | |
| 1013 | + | |
| 1014 | + | |
| 1015 | + | |
| 1016 | + | |
| 1017 | + | |
| 1018 | + | |
| 1019 | + | |
| 1020 | + | |
| 1021 | + | |
| 1022 | + | |
| 1023 | + | |
| 1024 | + | |
| 1025 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
367 | 367 | | |
368 | 368 | | |
369 | 369 | | |
370 | | - | |
371 | | - | |
372 | | - | |
373 | | - | |
374 | | - | |
375 | | - | |
376 | | - | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
377 | 377 | | |
378 | 378 | | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
379 | 384 | | |
380 | 385 | | |
381 | 386 | | |
| |||
0 commit comments