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

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 orders — partial-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.

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'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.

Merge — origin/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 afbf271143715707a96d6255aee08261bf9ac15f → packageMentionDocs.

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.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 1, 2026 04:38
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 2a18117 Sep 1, 2026
34 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-14023-migration-driver-exec-surface branch September 1, 2026 05:03
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