Skip to content

fix(metadata): bind the published migrations to the driver surface IDataDriver declares, and pin it with a real driver - #14084

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-14023-migration-driver-exec-surface
Sep 1, 2026
Merged

fix(metadata): bind the published migrations to the driver surface IDataDriver declares, and pin it with a real driver#14084
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-14023-migration-driver-exec-surface

Conversation

@claude

@claude claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #14023

The defect

All four helpers exported from @objectstack/metadata/migrations guarded on — and drove through — driver.raw(sql, bindings?). No data driver in this repo defines raw. SqlDriver keeps its knex handle protected and declares no raw member, SqliteWasmDriver inherits that, and the only raw( member anywhere outside a test double is an HTTP harness in packages/verify whose 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:

migrateSysNotificationToEvent({ driver, data })  ->  { status: 'error', migrated: 0 }

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. DatabaseLoader calls migrateProjectIdToEnvironmentId(driver) on bootstrap with a real driver, at two call sites, each wrapped in a catch — so the v5.0 project_id -> environment_id forward 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: try execute, fall back to raw, refuse only when neither is present.

execute goes first because it is the surface the contract declares:

// packages/spec/src/contracts/data-driver.ts  --  IDataDriver
execute(command: unknown, parameters?: unknown[], options?: DriverOptions): Promise<unknown>;

non-optional, with bound parameters as the second positional argument — exactly the shape raw(sql, bindings?) was being called in. IDataDriver has never declared raw. raw is 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/spec is 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:293 as the place execute is declared. That line declares IDataEngine.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 by ObjectQL.execute and called that way by service-analytics. The migrations take an IDataDriver, so data-driver.ts governs. The correction strengthens the ruling rather than weakening it: on IDataDriver, execute is required, not optional, so every conforming driver has it.

Why the order had to be chosen rather than copied

metadata-protocol already resolves both surfaces, in opposite orderspartial-index-probe.ts raw-first, seed-tenancy-backfill.ts execute-first, and protocol.ts's ensureOverlayIndex a 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 raw method — including the case that asserts the guard fires. The suite pinned the guard's wording while never once exercising a driver the platform ships. Swapping raw for execute in the helpers and in the doubles would have moved that hole, not closed it.

src/migrations/real-driver-exec-surface.test.ts drives all four migrations through a real SqliteWasmDriver (already a devDependency here, extends SqlDriver, real in-process SQLite, no server), asserting the physical schema rather than the returned status — the returned status is what reported error for years while nothing happened. Its load-bearing case pins the surface reality the file exists for: the real driver has no raw and does have execute, so if that ever moves back, every other case stops proving anything and says so.

database-loader.test.ts bolted raw 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 — it reached past the declared contract, which is why createMockDriver already carries execute without one. Both now observe the mock's own execute, 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 fef41187f unless stated.

Testspnpm --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's execute limb replaced by if (false ...)). Predicted direction: RED, because every execute-only and real-driver case loses its entry point. Observed: Tests 8 failed | 15 passed (23).

  • Mutation proven on disk in both directions before running: removed literal 0 occurrences, injected marker 1, blob 32aaee4c -> e847020a.
  • Restore under trap ... EXIT INT TERM with an absolute path pinned to HEAD (git checkout HEAD -- "$ABS"), proven after: git diff HEAD empty, worktree blob back to 32aaee4c = the HEAD blob, marker absent, limb present.
  • No rebuild leg applies. The subject is reached from the tests through same-package relative imports, which vitest resolves to source — it crosses no package wall through 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: a dist-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/objectstack re-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-contract refused 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 / assertEngineFindOnePredicate from @objectstack/metadata-core), then --write recorded the three new pinned rows — add-only, 15 insertions, no row dropped.
  • check:type-check-debt re-measured @objectstack/metadata at 91 against a shrink-only ledger recording 89. Both new errors were mine — a wrong ([sql]: [unknown]) destructuring annotation against mock.calls, which is any[][]. Removed; back to 89, ledger untouched. Verified with tsc --noEmit --listFiles that 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.

Mergeorigin/main moved mid-flight and #13998's timestamp fix landed in migrate-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 the selectLegacyRows signature change on top. scripts/engine-double-contract.pinned.json auto-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 raw requirement (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.md needed 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-mongodb execute() answer without running the command and without refusing — routed pm:on-hold per the #5499 freeze) and #14083 (metadata-protocol's three resolvers, above).

Generated by Claude Code


Generated by Claude Code

… 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
@github-actions github-actions Bot added the size/l label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata, touching 17 documentable anchor(s).

8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/data-api.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/data-modeling/queries.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/data-modeling/schema-design.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/protocol/kernel/http-protocol.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/protocol/kernel/realtime-protocol.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId), recipient_id (literal, a string literal in migrateSysNotificationToEvent))
  • content/docs/protocol/objectql/query-syntax.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/protocol/objectql/schema.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
  • content/docs/protocol/objectql/types.mdx (via project_id (literal, a string literal in migrateEnvIdToProjectId))
What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json afbf271143715707a96d6255aee08261bf9ac15fpackageMentionDocs.

Which tree this was computed on

This run read content/docs from dde40d4a2bd3683f808e9c1449ed06d05ce092b7 — the merge of head fef41187fce91959fc7546d0f6dc671efc30e070 into base afbf271143715707a96d6255aee08261bf9ac15f, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs afbf271143715707a96d6255aee08261bf9ac15f → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants