fix(metadata): bind the published migrations to the driver surface IDataDriver declares, and pin it with a real driver - #14084
Conversation
… declares
All four helpers in `packages/metadata/src/migrations/` guarded on and drove
through `driver.raw(sql, bindings?)`, a method no data driver in this repo
defines. `IDataDriver` declares `execute(command, parameters?, options?)`
non-optionally and has never declared `raw`, so the guard was enforcing a
surface the contract does not have — and refused every driver the platform
ships, quietly, through a returned `{ status: 'error' }`.
A shared resolver (`driver-exec.ts`) now tries `execute` first and falls back
to `raw`, applied uniformly across all four members. The refusal fires only for
a driver offering neither surface, and still states its remedy exactly once.
Adds `real-driver-exec-surface.test.ts`: the suite's every pre-existing case
built a double carrying `raw` — including the one asserting the guard fires —
so it pinned the wording while never exercising a shipped driver. The new file
drives all four migrations through a real `SqliteWasmDriver` on real in-process
SQLite and asserts the physical schema, not the returned status.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
…urface
`database-loader.test.ts` bolted a `raw` method onto its `IDataDriver` mock
through an `as unknown as { raw: unknown }` cast, in the two cases that observe
the post-sync migration. The cast was the tell: `createMockDriver` already
carries `execute` without one, because `IDataDriver` declares it non-optionally
and has never declared `raw`. Both cases now observe the mock's own `execute`.
The overlay-index case gains a non-vacuity assertion first. It asserts that no
statement matched a pattern, which a run issuing no statements at all satisfies
equally well — the state the file was actually in while the migration refused
every driver.
Pins the new file's engine double in the retained ledger (add-only).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
# Conflicts: # packages/metadata/src/migrations/migrate-sys-notification-to-event.ts
…xtures `mock.calls` is `any[][]`, so `([sql]: [unknown])` is not assignable to the callback `some`/`map` expect. Two errors, both mine, both caught by `check:type-check-debt` re-measuring @objectstack/metadata at 91 against a shrink-only ledger recording 89. Back to 89 with the annotations removed; the parameter is inferred as `any[]` and carries no implicit-any. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin dde40d4a2bd3683f808e9c1449ed06d05ce092b7 && git checkout dde40d4a2bd3683f808e9c1449ed06d05ce092b7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin afbf271143715707a96d6255aee08261bf9ac15f fef41187fce91959fc7546d0f6dc671efc30e070 && git checkout -B drift-repro afbf271143715707a96d6255aee08261bf9ac15f && git merge --no-ff fef41187fce91959fc7546d0f6dc671efc30e070
node scripts/docs-audit/affected-docs.mjs --json afbf271143715707a96d6255aee08261bf9ac15f
|
Fixes #14023
The defect
All four helpers exported from
@objectstack/metadata/migrationsguarded on — and drove through —driver.raw(sql, bindings?). No data driver in this repo definesraw.SqlDriverkeeps its knex handleprotectedand declares norawmember,SqliteWasmDriverinherits that, and the onlyraw(member anywhere outside a test double is an HTTP harness inpackages/verifywhose signature is(path, init).So an operator following the ADR-0030 cut-over runbook, which names this call as the supported way to preserve users' existing bell notifications, got:
Quiet in the shape that matters:
status: 'error'is a returned value, not a throw, and the message blamed the caller's driver for lacking a method instead of saying the migration had not run.It was not only an operator-facing path.
DatabaseLoadercallsmigrateProjectIdToEnvironmentId(driver)on bootstrap with a real driver, at two call sites, each wrapped in a catch — so the v5.0project_id->environment_idforward migration threw and was swallowed on every boot.The repair
One shared resolver,
packages/metadata/src/migrations/driver-exec.ts, used by all four members: tryexecute, fall back toraw, refuse only when neither is present.executegoes first because it is the surface the contract declares:non-optional, with bound parameters as the second positional argument — exactly the shape
raw(sql, bindings?)was being called in.IDataDriverhas never declaredraw.rawis kept as a fallback so a host or third-party driver that does define it keeps working: nothing that worked before stops working, and the accepted input set only widens.packages/specis not touched. The declared contract already carries this; the guard was enforcing a surface the contract does not have, and this brings enforcement back to the declaration.A correction to the card, the triage note and the claim comment
All three cite
packages/spec/src/contracts/data-engine.ts:293as the placeexecuteis declared. That line declaresIDataEngine.execute?(command, options?: Record<string, any>)— a different member on a different interface, whose second parameter is an options bag rather than bindings, implemented that way byObjectQL.executeand called that way byservice-analytics. The migrations take anIDataDriver, sodata-driver.tsgoverns. The correction strengthens the ruling rather than weakening it: onIDataDriver,executeis required, not optional, so every conforming driver has it.Why the order had to be chosen rather than copied
metadata-protocolalready resolves both surfaces, in opposite orders —partial-index-probe.tsraw-first,seed-tenancy-backfill.tsexecute-first, andprotocol.ts'sensureOverlayIndexa third, raw-first. One operation, three implementations, two behaviours resolves to the declaration-bound side. That directory's own divergence is recorded in #14083 and is not addressed here.The test finding is the load-bearing half
Every pre-existing case in this directory built its own double carrying a
rawmethod — including the case that asserts the guard fires. The suite pinned the guard's wording while never once exercising a driver the platform ships. Swappingrawforexecutein the helpers and in the doubles would have moved that hole, not closed it.src/migrations/real-driver-exec-surface.test.tsdrives all four migrations through a realSqliteWasmDriver(already a devDependency here, extendsSqlDriver, real in-process SQLite, no server), asserting the physical schema rather than the returned status — the returned status is what reportederrorfor years while nothing happened. Its load-bearing case pins the surface reality the file exists for: the real driver has norawand does haveexecute, so if that ever moves back, every other case stops proving anything and says so.database-loader.test.tsboltedrawonto itsIDataDrivermock through anas unknown as { raw: unknown }cast in the two cases that observe the post-sync migration. The cast was the tell — it reached past the declared contract, which is whycreateMockDriveralready carriesexecutewithout one. Both now observe the mock's ownexecute, and the overlay-index case gains a non-vacuity assertion first: it asserts that no statement matched a pattern, which a run issuing no statements at all satisfies equally well — the state that file was actually in.Verification
Everything below was measured at
fef41187funless stated.Tests —
pnpm --filter @objectstack/metadata exec vitest run --maxWorkers=2:Test Files 39 passed (39) · Tests 639 passed (639).Ablation (on the committed implementation,
driver-exec.ts'sexecutelimb replaced byif (false ...)). Predicted direction: RED, because every execute-only and real-driver case loses its entry point. Observed:Tests 8 failed | 15 passed (23).0occurrences, injected marker1, blob32aaee4c->e847020a.trap ... EXIT INT TERMwith an absolute path pinned to HEAD (git checkout HEAD -- "$ABS"), proven after:git diff HEADempty, worktree blob back to32aaee4c= the HEAD blob, marker absent, limb present.dist. (The one cross-wall import in the new file,@objectstack/driver-sqlite-wasm, is not the ablation subject and is unchanged by the mutation.) The ablation going red is itself evidence the mutation reached executed code: adist-stale ablation stays green.Gates — dependency closure built first, then the full repo (
turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70 successful) so the two ratchets could be measured rather than skipped.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackre-derived after the final commit: 28 families, all run, all exit 0. Exit codes captured before any pipe.Two gates did real work rather than passing on arrival:
check:engine-double-contractrefused the new file's engine double for not routing through the producers' dispatch predicates. Pinned it the way its sibling suite is pinned (assertEngineDeleteDispatch/assertEngineUpdateDispatch/assertEngineFindOnePredicatefrom@objectstack/metadata-core), then--writerecorded the three new pinned rows — add-only, 15 insertions, no row dropped.check:type-check-debtre-measured@objectstack/metadataat 91 against a shrink-only ledger recording 89. Both new errors were mine — a wrong([sql]: [unknown])destructuring annotation againstmock.calls, which isany[][]. Removed; back to 89, ledger untouched. Verified withtsc --noEmit --listFilesthat all five files I added or edited are genuinely inside that tsc program, so the 89 is a reading about them and not a green over source nothing compiled.Merge —
origin/mainmoved mid-flight and #13998's timestamp fix landed inmigrate-sys-notification-to-event.ts. Merged base into head (never rebased); the one content conflict was resolved by keeping that work whole and re-applying only theselectLegacyRowssignature change on top.scripts/engine-double-contract.pinned.jsonauto-merged and was verified by content, not by exit code: 671 base rows + 3 mine + 3 theirs = 677 in the merged tree, zero lost.Scope
#13998's timestamp defect in the same file is untouched — different class, delivered on its own PR, and its data half stays a maintainer floor. No existing-data backfill is written here.
content/docs/releases/is untouched; the changeset is the input to the release notes.The three JSDoc lines that stated the
rawrequirement (drop-projection-tables.ts,migrate-env-id-to-project-id.ts,migrate-project-id-to-environment-id.ts) are corrected.docs/handoff/adr-0030-notification-convergence.mdneeded no edit: it names the call without naming a driver surface, and the step it documents becomes true rather than false with this change.Out-of-scope findings filed unassigned: #14082 (
driver-memory/driver-mongodbexecute()answer without running the command and without refusing — routedpm:on-holdper the #5499 freeze) and #14083 (metadata-protocol's three resolvers, above).Generated by Claude Code
Generated by Claude Code