Skip to content

fix(metadata-protocol): resolve the raw-SQL driver seam through one execute-first helper - #14119

Merged
zhuangjianguo merged 2 commits into
mainfrom
claude/issue-14083-resolver-order-alignment
Sep 1, 2026
Merged

fix(metadata-protocol): resolve the raw-SQL driver seam through one execute-first helper#14119
zhuangjianguo merged 2 commits into
mainfrom
claude/issue-14083-resolver-order-alignment

Conversation

@claude

@claude claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #14083

Three sites in packages/metadata-protocol resolved a raw-SQL entry point off a driver, in two different orders. They now share one helper and all try execute first, with raw kept as the fallback.

site before after
src/migrations/partial-index-probe.ts (line 94) raw, then execute shared helper
src/migrations/seed-tenancy-backfill.ts (line 347) execute, then raw shared helper
src/protocol.tsensureOverlayIndex (line 5230) raw, then execute shared helper

Line numbers re-derived on this branch's base. Note the third one: the card cited roughly line 5159 and the real location is 5230.

Why this order

IDataDriver (packages/spec/src/contracts/data-driver.ts, line 108) declares

execute(command: unknown, parameters?: unknown[], options?: DriverOptions): Promise<unknown>;

non-optionally, and has never declared raw at all. So execute is not merely the surface the shipped drivers happen to have — it is the only raw-execution surface the contract guarantees, and any driver satisfying the interface has it. This is the 2026-08-07 meta-criterion (one operation, several implementations, inconsistent behaviour, decide by the declaration-bound side) applied a second time; the first application is packages/metadata/src/migrations/driver-exec.ts from #14084, which this follows. The two modules are twins and their headers cross-reference each other.

raw is deliberately kept. Nothing that worked before stops working.

New file: packages/metadata-protocol/src/migrations/driver-exec.ts

A twin of metadata's, not an import of it. @objectstack/metadata is already a declared dependency of metadata-protocol and there is no cycle — so the dependency direction was not the obstacle the card assumed. The live reasons for a twin are:

  • driver-exec.ts is internal to metadata's migrations directory; it is not re-exported from @objectstack/metadata/migrations. Importing it would mean widening that package's published surface to serve three call sites in a sibling package.
  • The only subpath that could carry it is the ./migrations barrel, and ensureOverlayIndex runs on every boot. Putting a migrations barrel on the boot path to save ten lines is the wrong trade.

While there, protocol.ts's stale justification is corrected: it claimed a circular dependency because "metadata already depends on objectql". That is false — @objectstack/metadata does not depend on @objectstack/objectql; objectql depends on both.

Behaviour

No change on any driver this repo ships. No data driver here defines raw: InMemoryDriver, MongoDBDriver and SqlDriver each declare execute and none declares raw, and SqliteWasmDriver and TursoDriver extend SqlDriver. The only raw( members in the tree are two test doubles and packages/verify/src/harness.ts, an HTTP harness whose signature is (path, init).

The flip matters for a host or third-party driver defining both surfaces: it used to be driven through raw at two sites and execute at the third — the same operation on two paths in one process.

Two consequences of routing all three through one helper, named explicitly because they are more than a reordering:

  • Bindings now reach the raw limb. seed-tenancy-backfill.ts's raw fallback was (sql) => driver.raw(sql) — it dropped its params argument entirely. Invisible only because that limb is unreachable on every shipped driver. Same defect class, same file, mechanical, and the correct shape was already pinned by metadata's helper.
  • The capability predicate is now defined as the resolution succeeding, so canRunSql / canRun / the inline check cannot drift away from what actually gets selected.

Both surfaces are now called as (sql, bindings), matching the precedent.

Known limitation, named rather than endorsed

typeof driver.execute === 'function' separates "declares the surface" from "does not", not either from "can actually run SQL". Two shipped drivers satisfy the non-optional declaration and execute nothing. That is a capability-declaration question and is out of scope here — it is tracked on its own card (#14082, which stays open and is not addressed by this PR). The new module's header says so, so the comments read as agreement about the ORDER only.

Tests

New pin src/migrations/driver-exec.test.ts covers all four driver shapes — execute-only, raw-only, both, neither — plus binding pass-through and predicate/resolution agreement. The both row is the only one that can tell the two orders apart; a realistic double would pass under either and pin nothing.

Fixture triage, judged per fixture rather than renamed in bulk:

  • view-definition-active-index.test.ts — asserted raw-first with a both-surface double. That is exactly the branch this PR rules on, so the assertion and its title were updated to the new order.
  • sys-setting-identity-index.test.ts — its doubles offer only raw, so the owner-vs-default question it exists for is untouched; only the call-argument shape changed.

Evidence, all on 7417c3c469:

  • pnpm --filter @objectstack/metadata-protocol test2082 passed, 10 skipped, exit 0.
  • pnpm --filter @objectstack/metadata-protocol typecheck — exit 0, and tsc --listFiles confirms all three edited/new test files are inside the program (3/3), so that green covers them.
  • Repo-wide ESLint (eslint . --no-inline-config) — exit 0, zero findings. Not a narrowed run.
  • Derived gate union (scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack) — 36 families, 35 pass. check-test-completeness with no log argument is NOT MEASURED by the gate's own design ("record this gate as NOT MEASURED; it is not a red"). check:type-check-debt --re-measure: "28 ledger entries re-measured, 1468 raw tsc errors total, none above its recorded number" — no ratchet moved up.

Ablation of the new pin

There is no behaviour difference to ablate — the raw limb is dead on every shipped driver — so the pin itself was ablated instead, predicting the direction first (only the two both-surface assertions should redden).

  • Baseline at HEAD: 34 passed.
  • Leg A, swapping the two limbs in the shared helper: 2 failed | 32 passed — exactly selects execute() — NOT raw() — on a driver that offers BOTH and resolveIndexExec prefers execute(), falls back to raw(). Every single-surface case stayed green.
  • Leg B, restoring partial-index-probe.ts byte-exact from the base commit (the real pre-alignment resolver, not a hand-written mutant): 1 failed | 33 passed.

Each leg proved its mutation on disk in both directions (the injected text present, the removed text absent) plus a changed blob hash; each restore was proved by blob-hash equality against the HEAD blob and an empty git diff HEAD, under a trap ... EXIT INT TERM using absolute paths pinned to HEAD. Both legs reddened without a rebuild, which mechanically confirms the pin resolves the source (a same-package relative import) rather than a stale dist/.

Generated by Claude Code


Generated by Claude Code

…xecute-first helper (#14083)

Three sites resolved a raw-SQL entry point off a driver in two different
orders: `migrations/partial-index-probe.ts` and `protocol.ts`'s
`ensureOverlayIndex` tried `raw` first, `migrations/seed-tenancy-backfill.ts`
tried `execute` first. They now share `migrations/driver-exec.ts`, which tries
`execute` first and keeps `raw` as the fallback.

`execute` goes first because `IDataDriver` declares it non-optionally and has
never declared `raw`, so it is the only raw-execution surface the contract
guarantees. Same reasoning and same order as `@objectstack/metadata`'s
`migrations/driver-exec.ts`; the two headers cross-reference each other.

No behaviour change on any shipped driver: none defines `raw`, so that limb was
unreachable and `execute` already ran at all three sites. The flip matters for a
host or third-party driver defining BOTH, which previously took `raw` at two
sites and `execute` at the third. `raw` is kept.

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/m 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-protocol, touching 7 documentable anchor(s).

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

  • content/docs/concepts/metadata-lifecycle.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))
  • content/docs/releases/v17.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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 — 8 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 9e1b2de73ef5d7ce5f3f665612a12731d3608725packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2cbcf13bdf30b3cf65aec3dcd55456429cc143b6 — the merge of head 6b45e0c4040ca2df690ecbd98861bb11e67de786 into base 9e1b2de73ef5d7ce5f3f665612a12731d3608725, 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 2cbcf13bdf30b3cf65aec3dcd55456429cc143b6 && git checkout 2cbcf13bdf30b3cf65aec3dcd55456429cc143b6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9e1b2de73ef5d7ce5f3f665612a12731d3608725 6b45e0c4040ca2df690ecbd98861bb11e67de786 && git checkout -B drift-repro 9e1b2de73ef5d7ce5f3f665612a12731d3608725 && git merge --no-ff 6b45e0c4040ca2df690ecbd98861bb11e67de786

node scripts/docs-audit/affected-docs.mjs --json 9e1b2de73ef5d7ce5f3f665612a12731d3608725

⚠️ 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 9e1b2de73ef5d7ce5f3f665612a12731d3608725 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…ted by one line

The import added at `packages/metadata-protocol/src/protocol.ts:30` shifts every
line below it by +1, including the `if (context?.isSystem) return data;`
elevation read in `stripReadonlyForInsert` (1736 -> 1737). The census page
still anchored 1736, so `check-system-context-census` reported both halves of
one rot: `[site-without-a-row]` at 1737 and `[anchor-is-not-a-read-site]` at
1736.

Repaired with the gate's own `--fix`, which re-pointed exactly one anchor and
added or deleted no row. Pure line rot: the population is unchanged at 109
elevation read sites.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

CI repair pushed as 6b45e0c404 — docs anchor only, no behaviour change.

Lint & Repo Gates was red on check-system-context-census. Cause was pure line rot owned by this branch: the import added at packages/metadata-protocol/src/protocol.ts:30 shifts every line below by +1, including the elevation read if (context?.isSystem) return data; in stripReadonlyForInsert1736 on origin/main, 1737 here — while content/docs/permissions/system-context.mdx still anchored 1736.

Repaired with the gate's own --fix:

re-anchored content/docs/permissions/system-context.mdx:115
  `metadata-protocol/src/protocol.ts:1736` -> `metadata-protocol/src/protocol.ts:1737`
check-system-context-census --fix: 1 anchor(s) rewritten

Exactly one anchor re-pointed; no row added or deleted, row 21's prose byte-identical. Gate bare, exit captured before any pipe:

check-system-context-census: OK — 109 elevation read sites in 20 packages across 45 files,
all anchored; 145 anchors resolve, 27 declared non-read.     (exit 0)

git diff --stat of the repair commit: content/docs/permissions/system-context.mdx | 2 +-, one file. The same gate also runs green on the merged generation (git merge-tree --write-tree origin/main HEAD at origin/main@556ebc1509, clean, materialised and checked), which is the tree the merge queue rebuilds.

Why the authoring seat's local family run did not catch it: check-system-context-census is absent from the derived gate family for this change set — it declares the page it maintains, never the packages/** subtree it censuses. Filed as #14131.

Generated by Claude Code


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 1, 2026 07:42
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 6e89621 Sep 1, 2026
35 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-14083-resolver-order-alignment branch September 1, 2026 08:08
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/m tests tooling

Projects

None yet

2 participants