epic #470 C11: migration handoff — seed the plugin ledger on witnesses, not trust - #806
Merged
Conversation
…(C10) Epic #470 C10 — the flip. The Dev Platform now lives in byte5ai/omadia-dev-platform and installs via Hub/ZIP. Removed: - middleware/src/devplatform/ (62 files) — stores, worker, backends, LLM proxy, pipeline/gates, GitHub App, triggers, routers, wireDevPlatform - middleware/test/devplatform/ (58 files) - middleware/scripts/dev-transcript.ts Adversarial eval (#498): the `brief_delimiter` Tier A probe ran the real `composeBrief` out of src/devplatform/. A probe against a module core no longer ships measures a library, not a deployed defense — so the probe, its five corpus scenarios (direct-injection.jsonl, indirect-injection.jsonl) and their baseline rows leave with it. This is a real coverage reduction and is recorded as such in test/adversarial/README.md, with both ways to close it. Deterministic corpus: 12 scenarios -> 7.
…y (C10) Removed: - middleware/packages/dev-runner-shim/ (23 files) — the in-container agent shim. Never built by `npm run build`, yet index.ts resolved its dist/ at runtime; the extraction removes that inconsistency with it. - middleware/sidecars/dev-runner/ (2), dev-runner-daemon/ (30, dockerode), dev-dind/ (2) - docker-compose.dev-platform.yaml All four now live in byte5ai/omadia-dev-platform, which owns their GHCR publishing, SBOM and signing pipeline.
index.ts (-238 lines): - 15 devplatform imports + the now-dead DeviceFlowStore / ConductorRoleStore - the GitHub-webhook block mounted before express.json - the whole assembly block: assembleDevPlatform / mountDevPlatform, the GitHub-App routers, the three chat orchestrator tools, worker start + SIGTERM/SIGINT hooks, the uiRouteCatalog nav registration, the retention cron and the no-graphPool warning Side effect worth calling out: the `/api/v1/dev-runner/llm/` carve-out sat BEFORE the conductor inbound webhook router, so an express.json ran ahead of that router's route-level express.raw() and short-circuited it (body-parser marks the request `_body`). Deleting the carve-out restores the order the surrounding comments already document: pluginRawBodyMount -> conductor raw router -> global express.json. config.ts (-305 lines): all 43 DEV_*/FLY_* dev-platform schema keys, the `devPlatform` namespace, `buildDevPlatformConfig`, `csvList`, and the CORE_DEV_PREFIXED_KEYS / DevPlatformEnvKey / isDevPlatformEnvKey machinery that existed only to hold those keys out of the top level — `Config` is now `ParsedConfig`. `devPlatformBootRefusals` goes too: the plugin owns both interlocks (pluginConfig.ts, verified), so they became activation refusals rather than vanishing. KEPT: devFlag() (two PUBLIC_MCP_* call sites), DEV_ENDPOINTS_ENABLED, DEV_ENDPOINTS_LOOPBACK_ONLY, FLY_APP_NAME. .env.example: the 15-key block replaced with a pointer to the plugin repo. middleware/package.json: the §5 note — express/pg/zod stay as plugin peerDependencies resolved through the host node_modules symlink.
… (C10) - web-ui/app/admin/dev-platform/ (29 files) — the operator SPA. It ships from the plugin now as a compiled Vite bundle served through the C8 static host at /p/<pluginId>/ui/. - web-ui/app/_components/devjobs/ (6 files) + app/_lib/useDevJobEvents.ts - app/admin/page.tsx — the grid card. The generic `requiresNavFrom` mechanism stays; this was its only user. - app/_lib/i18n-structural.test.ts — the GateInbox.tsx path entry. H3 resolved by omission, as the plugin's ACCEPTANCE-RUN records. chat/page.tsx no longer special-cases `tool.name === 'dev_job_start'`; a dev-job start from the installed plugin now falls through to the generic long-running-task card (`isTaskStartToolName` / TaskChatCard), which is the accepted degradation. i18n: 299 leaf keys per locale removed (adminDevPlatform.* 288, chat.devJob.* 9, admin.index.cards.devPlatform.* 2) = 598 across en+de, plus the four orphaned `i18n-identical-allowlist.json` entries the validator flagged. `npm run i18n:check` OK — 3560 keys; `i18n:literals` translate=0.
… (C10)
publish-images.yml:
- the `dev-runner` and `dev-runner-daemon` matrix entries
- the runner-image supply chain: cosign install, syft SPDX-JSON SBOM,
keyless sign + attest (all guarded on `matrix.name == 'dev-runner'`)
- `id-token: write`, which existed solely for that keyless signing
auto-release.yml / release.yml: the matching caller-side `id-token: write`
grants, which a reusable workflow cannot self-grant and nothing else needs.
The `if: false` npm-provenance job in release.yml keeps its own grant.
byte5ai/omadia-dev-platform owns runner GHCR publishing, SBOM and signing
now. Per implementation.md §2.4 the cosign certificate identity binds to
repo + workflow + ref, so the new signer needs the transition
`--certificate-identity-regexp` landed before it publishes — that is P4's
job in the plugin repo, not core's.
The missing-Dockerfile guard stays: it also computes the version `stamp`
every remaining image consumes; only its dev-runner-daemon justification
comment is gone.
scripts/wave-{implement,verify}.workflow.mjs: the generic wave prompts baked
in "terminal transitions go through finalizeDevJob" — a dev-platform domain
rule in a domain-agnostic prompt, naming a function core no longer ships.
Genericised to "the subsystem's single finalizer"; the rule survives, the
implementor name does not. Same for the `docs/dev-platform/` example path.
`tsc` caught these, which is exactly why C10 has to be one PR. Three LONG_RUNNING_* keys sat INTERLEAVED with the dev-platform keys in config.ts, between DEV_PLATFORM_RUNNER_BASE_URL and DEV_PLATFORM_CLI_BIN, and went out with the block: LONG_RUNNING_SUBAGENT_TOOLS — W2-2 / issue #543, the generic LONG_RUNNING_TASK_STALE_MS `<tool>_start/_status/_list` seam that any LONG_RUNNING_TASK_RETAIN_MS slow tool opts into. Not dev-platform. FLY_APP_NAME went too, despite CORE_DEV_PREFIXED_KEYS documenting in prose that it "describes the host, not the feature, so it must survive the extraction". All four are restored under an explicit heading that says why they are core, so the next person reading config.ts does not have to re-derive it. Also drops the `node:os` import, now unused (it backed the workspace-dir default), and rewords the three comments this PR itself introduced that named the extracted subsystem — C10 must not add coupling references while removing them. Verified: middleware build + typecheck + adversarial/golden tsconfigs green; 6925 pass / 0 fail; typecheck:test ratchet 406 -> 371 (baseline lowered); lint 0 problems. web-ui typecheck green, 717 pass / 0 fail, i18n 3560 keys OK.
…(C10)
Ratchet: **3,300 → 214**, updated consistently in all three places the README
says must agree — `decoupling-baseline.json`, the README "Baseline **214**"
line, and the `acceptance.md` guard row.
Nine of fourteen zones read CLEAN. Every survivor is scheduled, not stranded:
migrations 69 C11 — 0022-0030 stay; core still applies them
middleware/test 62 C13 — fixture strings + the legacy-key regression test
scripts 27 C13 — the ratchet's own pattern list (needs a
self-exclusion before the total can reach 0)
middleware/src 19 C12 — publicPaths (6); rest are comments
web-ui/app 19 C13 — nav test fixtures, two comments
packages 17 C13 — plugin-api CHANGELOG recording the removal
env-example 1 the plugin repo URL, which cannot be reworded
Also: `test-typecheck-baseline.json` 406 → 371, and `package-lock.json`
regenerated (npm left `packages/dev-runner-shim` as an `extraneous: true`
stanza rather than dropping it; `npm ci` verified clean afterwards).
Docs:
- README gains a "C10 — the flip" status section: what left, what was kept and
why, H3 resolved by omission, the adversarial coverage reduction, and the
express.json ordering bug the deletion fixed. The H3 "decision before code"
is struck through — it is answered now.
- plan.md §4.2 gains a C10 note (and flags that its `devJobStepEffect.ts` line
went stale in the other direction: C5 deleted that port as dead code). P4 row
marked shipped.
- docs/CHANGELOG.md: "Dev Platform moved to byte5ai/omadia-dev-platform
(install via Hub/ZIP)" — written for operators, so it leads with how to get
it back, states that no data is touched, and that in-flight jobs survive the
upgrade because C12 has not run yet.
…t trust (C11)
A plugin extracted out of core inherits installations whose schema core already
created, recorded in a core ledger the plugin cannot see. Its own ledger is
empty, so its migration runner would re-apply every file.
The naive handoff copies the donor rows and skips those files. That is correct
on a healthy database and silently destroys one specific installation: rows
present, tables ABSENT — a restore from an older snapshot, a version-skewed
rollback, an operator who dropped a table during an incident. The plugin
activates green and every request 500s, nine steps behind the cause.
So the donor ledger is corroboration and a per-file WITNESS is the decision:
one catalog query that is true only when the schema object that file creates is
actually there.
- `platform/pluginMigrationHandoff.ts` — `seedPluginLedgerFromDonor`. One
transaction under C7's advisory lock and namespace, keyed on the ledger, so a
seed serialises against a migrate and against another seed. `dryRun` rolls the
whole transaction back, including the ledger DDL and anything a witness
touched. Reports `skippedNoWitness` — the files the donor recorded whose
witness is false — because that number is the restore alarm. Contains no
DELETE: the donor rows are the rollback path.
- `ctx.sql.seedLedger({ entries, dryRun })` — the plugin supplies filenames and
witness SQL, core supplies the donor ledger. Matching is by filename STEM, so
a codegen'd `0022_x.js` adopts core's `0022_x.sql`. Witness SQL rather than a
callback: a callback cannot be printed, and the point of `dryRun` is that an
operator reads which query proved which file.
- `pluginLedgerDdl` / `migrationChecksum` / `listMigrationFiles` are now exported
from `pluginMigrations.ts` and used by the seeder, so a seeded row is
byte-identical to one the runner would have written. A wrong checksum would
trip the drift guard one boot later.
- `@omadia/plugin-api` 1.3.0 (additive; `seedLedger` is optional, so a plugin
still activates against a 1.2.0 core). Snapshot regenerated.
- `middleware/scripts/plugin-ledger-handoff.mjs` — operator CLI, dry run by
default, `--apply` the only way to write. Named and written generically: core
may not name the extracted plugin, and the next plugin to leave wants it
unchanged.
Tests: 27 new (25 pg-gated). Six required scenarios plus the counter-proof, a
wiring suite through `createPluginContext`, and a source-level assertion that
the donor ledger name still matches the migrator that creates it. Mutation
check: replacing the witness gate with `donorHasIt ||` fails exactly the three
tests that assert it, case (b) included.
Ratchet held at 214.
Weegy
added a commit
to byte5ai/omadia-dev-platform
that referenced
this pull request
Aug 20, 2026
…5ai/omadia#470) (#6) Slots 0022-0030 are already applied on every installation that ran the Dev Platform inside core, recorded in CORE's ledger. This plugin's ledger starts empty, so `runMigrations()` re-applies all nine. They are idempotent, so that is merely slow on a healthy database — but idempotence is a property of the files, and betting an upgrade on it nine times over is not a plan. `ctx.sql.seedLedger()` (core PR byte5ai/omadia#806, plugin-api 1.3.0) records them as applied instead, and will not take core's word for it: each file needs a WITNESS that the schema object it creates is actually present. The case that makes this necessary is rows present, tables ABSENT — a restore from a snapshot older than the migrations, a version-skewed rollback, an operator who dropped a table during an incident. A handoff that trusted core's rows would activate this plugin green and make every request 500. With witnesses the seed declines, `runMigrations()` applies the files, and that is the repair. - `src/ledgerHandoff.ts` — the nine witnesses. Each proves the LAST object its file creates, because a core migration file ran in one transaction, so the last object exists exactly when the whole file was applied. 0025 is the odd one: it REPLACES a CHECK constraint, so its witness reads the constraint DEFINITION for 'plugin' — presence alone would be true before it ran too. - `activate()` seeds BEFORE migrating, guarded on `ctx.sql.seedLedger` being present: a core older than plugin-api 1.3.0 falls through to the apply loop and says so, rather than refusing to activate on a core that can run it. - A non-empty `skippedNoWitness` is logged as a loud WARNING, not a refusal — the apply loop below is the repair, and the operator needs to know the database is not what they thought. - `handoff-plan.json` + REQUIRED in the ZIP. It is how an operator dry-runs the handoff against production BEFORE installing, with core's `middleware/scripts/plugin-ledger-handoff.mjs`. A ZIP without it installs perfectly and quietly removes the only step that de-risks the upgrade. Tests: 12 new. The entry list must cover exactly the shipped migrations (a missing witness re-applies forever, invisibly); no witness may cast to regclass (it throws on the very case a witness detects); every witness must name an object that appears in its own migration — all nine touch dev_jobs, so proving the wrong one is the easy mistake; the plan file must match the code; and activate() must seed before it migrates, degrade on an old core, and warn on a disagreement. Mutation check: pointing 0029's witness at 0022's table fails exactly the target test.
The floor was 269 and describing a tree that no longer exists. C10 deleted the
Dev Platform and its 26 pg tests left with the code they measured, so that
branch runs `ran=243` and goes red on a guard that is supposed to catch SILENT
skips, not deliberate deletions. A coverage floor that outlives its suites is
not protecting anything; it is just failing.
Two movements, both read off CI rather than guessed:
-26 C10's delete
+21 C11's `pluginMigrationHandoff.pg.test.ts` (14) and
`pluginMigrationHandoffAccessor.pg.test.ts` (7)
243 + 21 = 264, and CI reports exactly 264 — which is simultaneously the check
that nothing else went quiet on the way. Deliberate removal of the covered code
is the one legitimate reason to lower this floor; every other drop is the #565
silent skip it exists to catch, and the comment now says so.
…ansaction (C11)
A witness is SQL the PLUGIN writes and CORE executes on the connection that
holds the handoff transaction and its advisory lock. It ran as
`client.query(text)` with no bind values, which node-postgres sends over the
SIMPLE query protocol — and that protocol accepts multiple commands per
message. Measured against PostgreSQL 16.13:
SELECT true AS ok; COMMIT; DROP TABLE victim
ran all three. The COMMIT ended the seeder's transaction, so the advisory lock
was released mid-seed, `dryRun`'s closing ROLLBACK became a no-op, and the DROP
committed. `DELETE FROM _multi_orchestrator_migrations RETURNING true` needed
no semicolon at all: one row, one column, one boolean, so it passed the shape
check and deleted the donor rows this module documents as never deleted — the
rollback path for the whole extraction.
Two guards, both enforced by PostgreSQL rather than by scanning the string:
- each witness runs inside SAVEPOINT + `SET LOCAL transaction_read_only = on`,
so every write is refused; ROLLBACK TO SAVEPOINT is the way back, because
the GUC cannot be cleared once a statement has taken a snapshot
- `queryMode: 'extended'` forces the extended protocol, where the server
refuses multi-command strings outright
Neither is a semicolon scan: a semicolon inside a legal string literal is still
a valid witness, and a test pins that.
The witness type becomes SQL text rather than a callback. A callback cannot be
fenced — it receives the privileged client — and the shape check existed twice,
once in the accessor and once in the CLI, each with the same hole. Both copies
are deleted; the seeder owns execution, so neither caller can get it wrong.
Also: teardown moved out of `finally` (a throw there discards the witness error
that caused it), plugin-api re-cut as 1.4.0 because #802 took 1.3.0 on main,
and PG_TEST_FLOOR 264 -> 270 for the six new pg tests.
Mutation check: reverting the fence to `client.query(sql)` fails exactly the
five hardening tests and nothing else.
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.
Epic #470 C11 — the migration handoff. Stacked on #804 (C10); retarget to
mainonce that lands.Core's migrations
0022–0030stay in core's ledger and core keeps shipping them. This PR lets the extracted plugin adopt them on an existing installation instead of re-applying them — and makes it prove it may.The failure this exists to prevent
The naive handoff copies the donor ledger's rows into the plugin's ledger and skips those files. On a healthy database that is correct. On one specific database it destroys the installation silently:
The naive seed writes nine rows, the runner applies nothing, the plugin activates green, and every request 500s — nine steps and one deploy behind the cause.
So the donor ledger is corroboration, never authority. Each file carries a witness: a catalog query true only when the schema object that file creates is actually there.
Read down that table and the rule collapses to "the witness decides". That is deliberate, and it is why the donor read is reported rather than obeyed: the interesting number is the disagreement between the two, and
dryRunagainst production surfaces it before anything is written.A file with no witness is never seeded. Absence of proof is not proof, and the fallback costs one idempotent statement.
What ships
middleware/src/platform/pluginMigrationHandoff.ts—seedPluginLedgerFromDonor(). One transaction under C7's advisory lock (PLUGIN_MIGRATION_LOCK_NS, keyed on the ledger), so a seed serialises against a migrate and against a second seed.lock_timeoutandstatement_timeoutare C7's, because a plugin-supplied witness must not hold the ledger lock forever.dryRunrolls the whole transaction back — the ledger DDL and any side effect a witness had included.ctx.sql.seedLedger({ entries, dryRun })— the plugin supplies its filenames and its witness SQL; core supplies the donor ledger (_multi_orchestrator_migrations). A plugin has no field for the donor table, which is what keeps one plugin out of another plugin's migration history. Aplg_-prefixed donor is refused outright.0022_dev_platform.js(codegen'd); core recorded0022_dev_platform.sql. Full-name matching would find nothing and report "no donor rows", which looks exactly like a fresh install.dryRunis an operator reading which query proved which file. The shape is enforced, not coerced: exactly one row, one column, a real boolean. (SELECT count(*)is the tempting wrong witness —1for a table that exists,0for one that exists and is empty, a throw for one that does not.)pluginLedgerDdl,migrationChecksumandlistMigrationFilesare now exported frompluginMigrations.tsand used by the seeder. A seeded row is byte-identical to one the runner would have written; a checksum computed differently would trip the drift guard and turn a successful handoff into a hard activation failure one boot later.@omadia/plugin-api1.2.0 → 1.3.0 (additive:SqlAccessor.seedLedgeris optional,LedgerSeedEntry,SeedLedgerOptions,LedgerSeedReport). Snapshot regenerated; the diff is +21 lines, all additions. A plugin built against 1.3.0 still activates on a 1.2.0 core, where the accessor isundefinedand the idempotent files simply run.node middleware/scripts/plugin-ledger-handoff.mjs --plan <plan.json>. Dry run by default;--applyis the only way to write. It prints the plan against$DATABASE_URLand highlights the disagreement list.Rollback
Uninstalling the plugin and reverting C10 leaves core's migrator consistent, because nothing here writes to core's ledger:
DELETEat all, and a test asserts the donor table is byte-for-byte identical (ids andapplied_at) after a seed. Revert C10 and core's migrator finds its ledger exactly as it left it, applies nothing, and boots.plg_omadia_dev_platform_migrations) is a table only the plugin reads. Uninstall may orphan or drop it (D3: orphan by default); core never looks at it.dev_*tables are never dropped by the handoff. C10 kept the migrations in core for exactly this reason: for one release both sides can apply them, and the files are idempotent.The one irreversible act in this area would be deleting the donor rows — with them gone and core still shipping the files, core's own migrator would re-run all nine on the next boot. That is why the delete does not exist rather than being merely unused.
Tests
27 new tests, 25 of them pg-gated (skip cleanly without a test database, per #572). Fixture names are neutral throughout — the mechanism is generic, and core's ratchet counts the extracted plugin's identifiers in
middleware/test.skippedNoWitness= 9, runner applies 9 and repairs the schemadonorRecordedemptyskippedNoWitnessempty (no disagreement to report)dryRundryRunover an existing ledgeralreadySeededreportedapplied_atbyte-for-byte identicalalreadySeeded9; no duplicate rowsto_regclasson the table returns NULL — the green activation whose every request 500splg_-namespaced donor refused; non-allowlisted identifier refused; a claimed file the package does not ship refused; a missing donor table reads as "no donor rows", not an errorPlus a wiring suite through
createPluginContext(the accessor is absent without the grant; a.jsfile adopts a.sqldonor row; non-boolean witness, empty entries, blank witness and duplicate filename all rejected) and a non-pg test assertingCORE_MIGRATION_DONOR_LEDGERstill matches the migrator that creates that table — a rename there would otherwise make the handoff silently report "no donor rows" forever.Mutation check
Replacing the decision with
const proven = donorHasIt || (witness ? await witness(client) : false)— i.e. trusting the donor row — fails exactly three tests and no others:seeds NOTHING when the donor recorded the files but the schema objects are absent(case b)never seeds a file that has no witness at alldoes not seed when the witness is false, and the runner then applies the file(accessor)The witness gate is load-bearing, not decorative.
One real bug the tests found
The first implementation validated the donor column name with the ledger-table charset rule, whose minimum length is three characters. The column is
id. Every pg case failed identically on the first run; the rule is now split into a charset check for any donor identifier and theplg_-namespace check for the table only.Gates
npm run buildnpm run typechecknpm run typecheck:testnpm run lintnpm testnpm run test:pgnode scripts/check-core-decoupling.mjsThe ratchet cost one round of rewording: the first draft of the test-file headers explained the ratchet using the literal identifiers it counts, which pushed the test zone from 62 to 65. Reworded, not raised.
Note on naming
The working title for the CLI was
scripts/dev-platform-handoff.mjs. It ships asmiddleware/scripts/plugin-ledger-handoff.mjsinstead, for two reasons: a core script named after the plugin is the coupling this epic removes (and C13 pins the ratchet at 0), and the tool is genuinely generic — the plan file carries the plugin id, ledger, directory and entries, so the next subsystem to leave core uses it unchanged. It also lives undermiddleware/scripts/rather than the repo root because that is where the other database-touching operator scripts live and wherepganddist/resolve. Say the word and I will rename it.One extra commit: the Postgres coverage floor
PG_TEST_FLOORwas269and describing a tree that no longer exists. C10 (#804) is red on this check today, atran=243: it deleted the Dev Platform and its 26 pg tests left with the code they measured. A coverage floor that outlives its suites is not protecting anything — it is just failing, and a permanently-red guard is one nobody reads.Re-pinned to 264, which is arithmetic and not a guess:
That the two agree is itself the check that nothing else went quiet on the way. The comment in
ci.ymlnow states the only legitimate reason to lower this floor — the covered code was deliberately removed — so the next drop still reads as the #565 silent skip this guard exists to catch.If #804 lowers it independently, take the higher of the two; 264 is the correct value for C10 + C11 combined.
Cross-family review (Forge)
GPT-5.4 at
reasoning_effort=high, reviewing against the code rather than the prose. Seven checks were asked for; six passed as written. One did not, and it is the one the design leans hardest on.Verdict per check
DELETE/UPDATE/TRUNCATEagainst the donor anywhere in the module — but see the defect, which reintroduced the delete through the witness20bc6563pluginLedgerDdl, samemigrationChecksum, sameINSERT … (filename, checksum). The runner will not re-apply and will not trip driftPLUGIN_MIGRATION_LOCK_NS = 4_420, same as C7, keyed on the ledger; concurrent-seed test passesdryRunwrites nothingledgerExists()false,ledgerRowCount()unchanged), not on the return value--applyis the only writer,DATABASE_URL(or explicit--database-url);check-core-decoupling.mjsheld at 214PG_TEST_FLOORloweringThe defect: witnesses could leave the transaction
A witness is SQL the plugin writes and core executes, on the connection holding the handoff transaction and its advisory lock. Both call sites ran it as
await client.query(witnessSql)— a bare string, no bind values. In node-postgres 8.22 that takes the simple query protocol (Query.requiresPreparation()returnsthis.values.length > 0; an empty array is not enough), and the simple protocol accepts multiple commands per message.Measured against the real PostgreSQL 16.13 test database, not reasoned about:
SELECT true AS ok; COMMIT; DROP TABLE victimCOMMITended the seeder's transaction; theDROPcommitted; the closingROLLBACKwas a no-op. Table gone.DELETE FROM _multi_orchestrator_migrations RETURNING trueSELECT pg_advisory_lock(424242) IS NOT NULLROLLBACKon the pooled connectionThree claims this PR makes in writing did not hold:
dryRunrolls the whole transaction back … nothing survives it." ACOMMITinside a witness makes the finalROLLBACKa no-op. The operator CLI defaults to dry run and is documented as costing "one read-only transaction" — it was neither read-only nor reliably rolled back.COMMITreleasedpg_advisory_xact_lockmid-seed.The fix (
20bc6563)Two guards, both enforced by PostgreSQL rather than by scanning the string — a semicolon inside a legal string literal is still a valid witness, and a test pins that:
SAVEPOINT→SET LOCAL transaction_read_only = on→ witness →ROLLBACK TO SAVEPOINT→RELEASE.DELETEis refused with cannot execute DELETE in a read-only transaction. Rolling back the savepoint is the only route back: PostgreSQL refuses to clear the GUC once a statement has taken a snapshot.queryMode: 'extended'→ the server refuses multi-command strings with cannot insert multiple commands into a prepared statement, before any part executes. (@types/pg8.21 predates the option that pg 8.22 implements, so it reaches the driver through one documented cast; a pg test pins the behaviour against a real server, so a driver or types change that dropped it fails loudly.)MigrationWitnessis now SQL text, not a callback. A callback cannot be fenced — it receives the privileged client. This also removed a real duplication: the shape check existed twice, inmakeSqlWitness(accessor) andwitnessFor(CLI), each carrying the same hole. Both are deleted; the seeder owns execution, so neither caller can get it wrong, and the CLI inherits the guarantee rather than re-implementing it.Two further defects found while verifying the fix:
throwinsidefinallyin the teardown (caught byno-unsafe-finally) would have discarded the witness error that caused the cleanup failure — the exact error an operator most needs. Teardown moved out offinally; both errors are now carried out and reported together.SqlMigrationErrornow passes through unwrapped.Mutation check
Reverting the fence to
await client.query(sql)fails exactly five tests and no others:Every one asserts on database state — canary table still present, donor row count unchanged, ledger row count zero — not merely on the thrown error.
One of these tests initially failed for a reason worth recording: a non-
dryRunseed commits its ledger DDL even when every witness is false, soledgerExists()istruewith zero rows. The assertion was wrong, not the code; it is now a row-count assertion, which is the stronger claim anyway.PG_TEST_FLOOR: 264 → 270The lowering to 264 was justified (C10 removed 26 pg tests) and the guard still catches a silent skip — it fails on
skipped != 0independently of the floor. Re-raised for the six new tests, measured per file rather than inferred:#802 merged while this review was running (
d0a64696, 19:46 UTC) and took1.3.0.mainnow carries plugin-api1.3.0, so this PR's1.3.0would have been a duplicate version — indistinguishable from a silently-changed contract to anything resolving by version. Re-cut as 1.4.0, with the reason recorded in the CHANGELOG.Two things still need a human:
main. Its base is chore(#470): C10 — the flip: delete the Dev Platform from core (~49k LOC) #804 (C10), which is itself behindmain. fix(#470): three core gaps the P5 acceptance run hit — optional_requires, core migration ledger, scoped plugin nav (C9) #802 touched the same four plugin-api files (package.json,CHANGELOG.md,api-snapshot/,src/pluginContext.ts), so conflicts in all four are expected when C10 and then C11 catch up. Resolving them here first would have made the diff against C10 unreviewable.Gates (all re-run after the fix)
npm run buildnpm run typechecknpm run typecheck:testnpm run lintfinallydefect above)npm testnpm run test:pgnode scripts/check-core-decoupling.mjsTODO/FIXME/XXXin touched filesResidual, not fixed
A witness can still call
pg_advisory_lock(), which is read-only-safe and survives the rollback, leaving a session lock on a pooled connection. Not a new capability — a plugin grantedpermissions.sqlalready runs arbitrary SQL throughrunMigrations— and the seeder'sstatement_timeout/lock_timeoutbound the blocking, so it is recorded here rather than papered over. Fixing it properly means running witnesses on a connection that is discarded afterwards, which is a larger change than this PR should carry.Verdict: MERGE, once #804 lands and the plugin-api conflicts against
mainare resolved.