fix: run the declared ledger handoff before core's pre-activate migrations (#470 C15) - #815
Merged
Merged
Conversation
…tions Core runs a plugin's migrations before activate(), but the C11 handoff (ctx.sql.seedLedger) is contractually called inside activate(), before runMigrations. The orderings are inverted, so on the exact upgrade C11 exists for the handoff always arrived after every ledger row was already written and could only report alreadySeeded. skippedNoWitness -- the one alarm C11 was built to raise -- could never fire, and the log line read "0 seeded, 9 already seeded", indistinguishable from a healthy re-run. A plugin cannot fix this from its side: core calls the runner before handing over control, and the witnesses are knowledge only the plugin has. So the plugin declares and core executes. - permissions.sql.handoff names a JSON plan inside the package, of the same shape seedLedger and the operator CLI already accept. - toolPluginRuntime loads it and runs the same seeder -- read-only witness fence, advisory lock, entry validation -- before its own migration runner, logs the report, and raises skippedNoWitness as a WARN naming the files. - The plan is validated at the boundary: zod, strict keys, package-root containment, size cap, typed refusals. A refusal fails the activation and takes the migration runner with it, because running the files anyway would write the very rows the unreadable plan existed to decide on. - ctx.sql.seedLedger is unchanged and stays correct for plugins that manage their own order or run against an older core. plugin-api 1.5.0 -> 1.6.0 (additive: one optional member). Fixes #814; epic #470 C15.
Weegy
added a commit
to byte5ai/omadia-dev-platform
that referenced
this pull request
Aug 21, 2026
…t (0.3.1) (#8) activate() calls ctx.sql.seedLedger before runMigrations, exactly as C11 documented -- but core runs permissions.sql.migrations itself, before activate(), so by the time this plugin got control all nine ledger rows were already written. The handoff could only report alreadySeeded, and skippedNoWitness -- the one alarm it exists to raise -- never fired. The 2026-08-21 acceptance run measured "0 seeded, 9 already seeded" on the exact upgrade the feature was built for, and nothing went red. Not fixable from inside the plugin: core calls the runner before handing over control, and the witnesses are knowledge only this plugin has. - permissions.sql.handoff: handoff-plan.json -- the kernel reads the plan and performs the handoff ahead of its own migration runner. The file is the one already in the ZIP, so an operator can still dry-run the exact plan with plugin-ledger-handoff.mjs before installing. - The activate() seedLedger call stays as the fallback. On a kernel that honours handoff it reports alreadySeeded, which is correct; on an older kernel it is the only thing that performs the handoff at all. - manifest.test.ts pins that handoff names a file the ZIP ships, and that the plan satisfies core's stricter reader: no unknown keys, no duplicates, under the size cap, filenames present in migrations/, and a ledger matching the manifest. Requires @omadia/plugin-api 1.6.0 (byte5ai/omadia#815). Older cores ignore the key and keep the previous behaviour. Refs byte5ai/omadia#814; epic byte5ai/omadia#470 C15.
Cross-family review of #815 found the containment guard and the dry-run key both fail OPEN, in a loader whose entire job is to fail closed. Symlinks defeated path containment. `path.resolve` is lexical and the `stat()` that followed it is not: a package shipping `handoff-plan.json` as a link to a file outside the root, or a directory link whose child path points outside, passed the string check and had its `witnessSql` read and executed. Both escapes were reproduced before the fix. The loader now re-asserts containment against real paths for BOTH the target and the package root — the root too, because on macOS `/var` is really `/private/var` and realpathing only the target would make every tmpdir-based package look like an escape. A missing target still refuses as `unreadable` rather than as an escape, so the operator still hears "the package does not ship this file" for the case they can fix, and a symlink that stays inside the package is still accepted. `"dryRun": true` recreated G7 from one stray key. The kernel honoured it, so the handoff reported and wrote nothing, activation SUCCEEDED, and the migration runner one line later applied every file and wrote the exact ledger rows the plan existed to decide on — the failure C15 exists to remove, restored silently and logged as a success. The operator CLI never read the key at all (it derives dryRun from --apply/--dry-run), so preview mode was only ever the CLI's. The loader now refuses it, typed as `dry-run-declared`; a shared file may still carry `"dryRun": false`. The field is then always false, so it is gone from `HandoffPlan`, from the `seedLedger` call and from the log line rather than left as dead weight. Two fail-closed properties the feature lives or dies on had no test. A witness that fails at the database aborts activation before the runner can run, and a plugin whose grant no longer matches its declared ledger gets no SQL accessor, so neither pre-activate step reaches the database. Both assert on the database and on the step-order trace, not just on the rejection — asserting only the throw would pass even if migrations had already run. Verified: swapping the handoff and migration blocks fails 6 tests, including the order assertion by name; disabling the realpath re-check fails both symlink tests while the inside-the-package symlink test still passes; disabling the dryRun refusal fails its unit and pg tests. 121/121 green with Postgres reachable and 0 skipped, build/typecheck/typecheck:test (ratchet 371, no regressions)/lint clean, api-snapshot current at 1.6.0, decoupling ratchet still reads 0.
…efore-migrations # Conflicts: # docs/CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #814; epic #470 C15.
The defect
Core runs a plugin's migrations before
activate()(toolPluginRuntime.ts, C7/G4 — so "the tables exist" is an invariantactivate()can rely on rather than a race each plugin re-loses in its own way). The C11 ledger handoff (ctx.sql.seedLedger) is contractually called insideactivate(), beforerunMigrations. The two orderings are inverted.On the exact upgrade C11 exists for — donor rows present, tables present — the handoff always arrived after core's runner had written every ledger row, so it could only ever report
alreadySeeded.skippedNoWitness, the one alarm C11 was built to raise, could never fire. Measured in the 2026-08-21 acceptance run:Nothing failed, and that is the problem:
0 seeded, 9 already seededis indistinguishable from a healthy re-run.A plugin cannot fix this from its side. Core calls the runner before handing over control, and the witnesses are knowledge only the plugin has. So the plugin declares and core executes.
The change
permissions.sql.handoff?: string— a path, inside the package, to a JSON plan:{ "entries": [{ "filename": "0001_x.js", "witnessSql": "SELECT to_regclass('public.x') IS NOT NULL" }], "dryRun": false }Same shape
SqlAccessor.seedLedgeraccepts, and the same shape the operator CLI reads.toolPluginRuntimepre-activate step — when the manifest declares it, core loads the plan and runs the handoff throughctx.sql.seedLedgeritself, ahead of its own runner. Going through the accessor rather than re-callingseedPluginLedgerFromDonoris deliberate: it inherits the read-only witness fence, the advisory lock, the entry validation and the grant check, so the two paths cannot drift. TheLedgerSeedReportis logged at info level, andskippedNoWitness.length > 0becomes a WARN naming the files — a count alone gives an operator nowhere to look.platform/pluginHandoffPlan.ts— the plan is plugin-supplied data that reaches core's filesystem and then core's database, on the boot path, before the plugin has run a line of its own code. It is validated at the boundary: zod, package-root containment (with the load-bearing+ path.sep, so<root>-evildoes not pass), a 128 KiB cap, and duplicate-filename rejection. Refusals are typed (PluginHandoffPlanError.reason), because an activation failure reaches an operator through the circuit-breaker and "the plan is invalid" tells them nothing actionable.The schema is strict. The key that makes it matter is
dir:SeedLedgerOptionsaccepts it, so an author could reasonably expect it to work, and silently ignoring it would leave them believing a directory override took effect. It is rejected by name.A refusal fails the activation and takes the migration runner with it. Running the files anyway would write exactly the ledger rows the unreadable plan existed to decide on — i.e. it would reproduce this bug.
ctx.sql.seedLedgeris unchanged. It stays correct for plugins that manage their own order, and it is the right fallback for a plugin that must also run against an older core. Against a core with this change it reportsalreadySeeded, which is what it should report once the work is done.Operator CLI (
plugin-ledger-handoff.mjs) —pluginId/ledger/migrationsDirmay now come from--plugin-id/--ledger/--migrations-dirinstead of the file, so one plan file serves all three readers. Forcing a plugin to ship two would let the file an operator previews drift from the file core runs. Documented in the script header, including that core's reader is the stricter of the two.Advisory-ledger WARN — the plan's
ledger(a CLI field) is reported but never obeyed; core resolves the ledger from the manifest and the grant. When the two disagree, core warns: the operator dry-ran against a different table than the one about to be written.Counter-proof
The defect was order, so asserting the outcome is not enough — a run that seeds nothing and applies everything reaches a correct database too. Each case records the order the two steps logged in and asserts on it.
Swapping the two blocks in
toolPluginRuntimeand re-running:The third failure is worth reading twice: with the order swapped, the suite reproduces G7's exact symptom —
0 seeded, 3 already seeded— which is the string the acceptance run recorded in production. The two cases that still pass are the ones that should: "no handoff declared" and "path escapes the package root" are order-independent by construction.Test matrix
test/pluginHandoffPlan.test.ts(14, no pg) — happy path,dryRunpassthrough, CLI-field tolerance + advisory ledger, escapes-root, absolute path,<root>-evilsibling, missing file, non-JSON, over-cap, emptyentries, blank witness, unknown key (dir), duplicate filename, top-level array.test/toolPluginRuntimeHandoff.pg.test.ts(7, pg) — drives the realToolPluginRuntime.activate()against a real package on disk and a real Postgres, because the failure being fixed is a wiring failure: every unit of this was already correct and tested in isolation, andpluginMigrationHandoffAccessor.pg.test.tswas green the whole time the feature was unreachable.handoffthenmigrationsskippedNoWitnessWARN naming all 3 files; runner then repairshandoffdeclaredmigrationsonly — pre-C15 behaviour, plan file on disk deliberately ignoredPluginHandoffPlanError(malformed), neither step ranPluginHandoffPlanError(escapes-package-root)dryRun: trueFixture names are neutral and suffixed per case: the decoupling ratchet counts the extracted plugin's identifiers in
middleware/test, and the suite writes into the real core donor ledger, so it removes exactly its own rows and never drops the table.Gates
npm run buildnpm run typechecknpm run typecheck:testnpm run lintcheck-core-decoupling.mjsplugin-apiapi:checkThe decoupling ratchet initially went 206 -> 207: the CHANGELOG named the extracted plugin. Reworded to "the first extracted plugin" rather than raising the baseline.
plugin-api 1.5.0 -> 1.6.0
Additive — the snapshot diff is exactly one line:
export interface SqlPermission { readonly migrations?: string; readonly ledger: string; +readonly handoff?: string; }One added optional member on an interface plugins read rather than implement. MINOR by the package's own rule.
Cross-family review (Forge)
Reviewed by Forge (GPT-5.4,
reasoning_effort=high) against the code, not the description — a deliberately different model family from the one that wrote the PR. Branch merged withorigin/main(C13 / #808) first; C14 (#813) has not landed, soscripts/check-core-decoupling.mjsstill exists on main and no ratchet edits were needed here. Merge was clean.Two of the six review questions found real defects. Both fail open, in a loader whose entire job is to fail closed.
Defect 1 — symlinks defeated path containment (fixed)
loadHandoffPlanasserted containment withpath.resolve+startsWith(root + path.sep). That is purely lexical; thefs.statthat followed it is not. Two escapes were reproduced before the fix:handoff-plan.jsonas a file symlink to a target outside the package root, andplans -> /outside) and declaringplans/secret.json.Both passed the string check,
stat().isFile()returned true, and core read and executed the outside file'switnessSql. The file's own comment claimed "nothing else may run against a path that is not inside the package"; that claim was false.The loader now re-asserts containment against real paths for both the target and the package root. Realpathing the root matters: on macOS
/varis really/private/var, so realpathing only the target would make every tmpdir-based package look like an escape. A missing target still refuses asunreadablerather than as an escape, so an operator still hears "the package does not ship this file" for the case they can actually fix, and a symlink that stays inside the package is still accepted — the guard does not over-refuse.Defect 2 —
"dryRun": truerecreated G7 from one stray key (fixed)The kernel honoured a plan-level
dryRun. The consequence on the kernel-run path: the handoff reports and writes nothing, activation succeeds, and the pre-activate migration runner one line later applies every file and writes the exact ledger rows the plan existed to decide on. That is gap G7 restored, silently, and logged as a success — and the pg suite asserted['handoff', 'migrations']for it, enshrining the fall-through as intended.The operator CLI never read the key at all —
plugin-ledger-handoff.mjsderives dry-run from its own--apply/--dry-runflags (dryRun: !args.apply). Preview mode was only ever the CLI's.The loader now refuses
dryRun: true, typed asdry-run-declared; a shared file may still carry"dryRun": false. The field is then always false, so it is gone fromHandoffPlan, from theseedLedgercall and from the log line rather than left as dead weight.Questions that came back clean
.., absolute paths and the sibling-prefix lookalike (<root>-evil) were all already handled; the+ path.sepwas correctly load-bearing. Size cap (128 KiB) enforced offstatbefore the read. JSON parse errors typed asnot-json, schema failures asmalformedwith the offending key named.ctx.sql, which exists only when a grant is present andgrant.ledger === declaredSql.ledger. The kernel goes throughctx.sql.seedLedger, so it inherits the identical read-only witness fence (transaction_read_only+ extended protocol), advisory lock and entry validation. There is no path by which an ungranted plugin seeds a ledger. Previously untested — now locked.runWitnessthrowsSqlMigrationErroron a driver error, a refused write, a multi-command string, a statement timeout or a bad result shape;seedPluginLedgerFromDonorrolls back and rethrows; the runtime'sawaitis not wrapped, so the throw propagates and the migration runner below never runs. Correct as written, and previously untested — a future "defensive"try/catchwould have restored G7 with the suite still green. Now locked.plugin-apiversion — main is at1.5.0;1.6.0on this branch is the correct single bump.api:checkreports the snapshot current.Two regression locks added
Both assert on the database and on the step-order trace, not just on the rejection — asserting only the throw would pass even if migrations had already run.
Mutation proof
The order guarantee is the whole point of this PR, so it was proven by mutation rather than by reading:
runs the handoff BEFORE the migration runnerby its own message ("inverted here is the whole bug")accepts a symlink that stays inside the package rootstill passesdryRunrefusalEvery mutant was killed by exactly the tests that should kill it, and all were reverted.
Verification
npm run build·npm run typecheck·npm run lint— all exit 0npm run typecheck:test— ratchet 371 known errors, no regressions (baseline unchanged).pg.test.tssuites self-skip when it is not, so a skipped run would not have been a passnode scripts/check-core-decoupling.mjs— "Core is free of Dev Platform references", still 0npm run api:check -w @omadia/plugin-api— snapshot up to date at 1.6.0Verdict: MERGE. The ordering fix this PR exists for is sound and now mutation-proven; the two fail-open holes in the loader that guards it are closed, and the two fail-closed properties it depends on are locked by tests.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.