Skip to content

fix(metadata-protocol): compile the seed-tenancy migration statements for the connected dialect, so they run on MySQL (#9381) - #9440

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-9381-dialect-aware-quoting
Aug 18, 2026
Merged

fix(metadata-protocol): compile the seed-tenancy migration statements for the connected dialect, so they run on MySQL (#9381)#9440
os-zhuang merged 2 commits into
mainfrom
claude/issue-9381-dialect-aware-quoting

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #9381

Step 1 — the premise, confirmed on a live MySQL

The card's central claim was explicitly not measured, and the dispatch made confirming it the binding first step. It is confirmed: no layer sets ANSI_QUOTES, and every statement this module builds failed to parse on a real server.

Live MySQL 8.0.46, the session the driver itself would open (mysql2 + the one session SET SqlDriver.withUtcSession performs, SET time_zone = '+00:00'):

session sql_mode: IGNORE_SPACE,ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,
                  NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION
ANSI_QUOTES set : false

Running the seven statements the module builds, verbatim from its own builders, against that server before the fix — all seven ER_PARSE_ERROR:

--- presence probe
    SQL: SELECT tenant_id FROM "_objectstack_sequences" WHERE 1 = 0
    RESULT: FAILED ER_PARSE_ERROR — ... near '"_objectstack_sequences" WHERE 1 = 0' at line 1
--- stamp
    SQL: UPDATE "crm_case" SET "organization_id" = ? WHERE "organization_id" IS NULL AND ...
    RESULT: FAILED ER_PARSE_ERROR — ... near '"crm_case" SET "organization_id" = 'org_1' ...' at line 1

7/7 statements FAILED on live MySQL

premise_still_valid: true.

What was actually wrong — three MySQL-only defects, not one

Fixing the quote character alone would still not have made these statements run. All three measured on the same server:

  1. The quote character. MySQL does not run with ANSI_QUOTES, so ANSI "x" is a string literal, not an identifier.
  2. last_value is RESERVED on MySQL 8.0 (the LAST_VALUE() window function). An unqualified last_value is ER_PARSE_ERROR even with the table spelled correctly, so the COLUMNS are quoted now, not only the tables. (A qualified g.last_value is legal — measured — which is why the split probe was never the whole story and why Report the business identifiers already minted twice by the tenancy split — an operator-facing inventory, on installs the #8686 backfill cannot repair #8928's own probe is unaffected.)
  3. UPDATE t … (SELECT … FROM t) is refused outright: ER_UPDATE_TABLE_USED (1093), "You can't specify target table 'crm_case' for update in FROM clause". The stamp's collision-exclusion guards now go through a derived table, which is plain ANSI and unchanged in meaning on the other two dialects.

Why nobody saw it: a migration must never fail a boot, so every call site swallows the failure into a warning. On MySQL the symptom was a skipped repair in the log — not a wrong answer, and not the repair either. Declared (the module header names MySQL) ≠ enforced.

The fix — producer-side, and the seam carries the dialect

Producer-side repair in metadata-protocol, as ruled; nothing in any driver was made tolerant of double-quoted identifiers.

  • quoteIdent(name, client) — the same shape as Report the business identifiers already minted twice by the tenancy split — an operator-facing inventory, on installs the #8686 backfill cannot repair #8928's in packages/cli/src/commands/migrate/duplicates.ts, backticks for mysql/mysql2, ANSI otherwise.
  • Every builder takes the client and compiles for it. SQLite and PostgreSQL get byte-identical ANSI text to what they got before, except for the columns that are now also quoted and the derived-table wrapper.
  • The structural half: resolveSeedTenancySeam(engine) returns { exec, client } and backfillSeedTenancy takes that pair. A caller can no longer obtain the exec without the dialect beside it — the quoting helper alone would leave the same hole open for the next caller. resolveSeedTenancyExec stays exported and unchanged for os migrate duplicates, which resolves its own client.

The client is read from the same driver object the exec came from (driver.config.client). Verified against real drivers, not doubles:

sqlite  seam.client = "better-sqlite3"
mysql2  seam.client = "mysql2"

Step 2 — the sibling migrations, checked

Asked for explicitly by the card; both results are results.

  • partial-index-probe.ts — clean. It builds no identifier-quoted SQL of its own; the statements it runs (a bare DROP INDEX IF EXISTS on an index name, and the caller's CREATE …) carry bare index names. Nothing to fix.
  • sys-setting-identity-index.ts — clean of THIS defect, and deliberately so: it uses bare identifiers and its header explains why (MySQL takes the degradation path for that index anyway). One separate, smaller problem found there while checking, filed unassigned as sys_setting's degradation message hands MySQL operators a duplicate-probe statement that does not parse on MySQL (key is reserved) #9434 and not touched here: the duplicate-probe statement it PRINTS for the operator names the column key, which is reserved on MySQL, so the remedy handed to the MySQL operator does not parse on MySQL (measured, ER_PARSE_ERROR). Printed only, never executed — no runtime path breaks. overlay-index.ts's equivalent probe was measured on the same server and runs fine.

Migration gating was not touched — that is #9380's surface, and #9380 is not addressed here.

Verification

Union of gates run at d79b35377c (the final commit).

Live MySQL, after the fix — a new suite that RUNS the statements on a real server, gated on OS_TEST_MYSQL_URL, with a non-vacuity assertion that the server is not running ANSI_QUOTES (on a server that had it, a green run would mean nothing — the exact condition the premise step ruled out):

[#9381] live MySQL 8.0.46 sql_mode=IGNORE_SPACE,ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,...
 ✓ the server is NOT running with ANSI_QUOTES — without this the run proves nothing
 ✓ every statement the migration builds PARSES and runs on MySQL
 ✓ a multi-autonumber object stamps with one derived table per guard
 ✓ repairs the split end to end, and reports the already-minted duplicates
 ✓ is idempotent — a second run finds no split
 Test Files  1 passed (1)   Tests  5 passed (5)

Reverse verification, twice — each half of the fix ablated alone, from the committed state:

  • quoting reverted to the unconditional ANSI form ⇒ 4 of 5 RED, ER_PARSE_ERROR on SELECT "tenant_id" FROM "_objectstack_sequences" WHERE 1 = 0;
  • derived table reverted only, quoting kept ⇒ 4 of 5 RED, ER_UPDATE_TABLE_USED on the backtick-spelled UPDATE.

The remaining green in both runs is the ANSI_QUOTES assertion, which is about the server and not about the fix. Restored ⇒ 5/5 green (above).

Live PostgreSQL 16.13 — the same builders' ANSI output, executed statement by statement on a real server with ON_ERROR_STOP=1: all seven run, and the semantics are right (UPDATE 1 — the one movable row moved; the row colliding with an org-side CASE-00001 stayed NULL and was reported).

SQLitepackages/runtime's end-to-end integration test on a real SqlDriver: 8/8 pass.

Suites

  • pnpm --filter @objectstack/metadata-protocol test — 120 passed | 1 skipped (121 files), 1662 passed | 5 skipped. The skip is the live-MySQL file with no URL provisioned: a named skip, never a silent pass.
  • pnpm --filter @objectstack/runtime test — 165 files, 2467 passed.
  • pnpm --filter @objectstack/cli exec vitest run src/commands/migrate — 7 files, 33 passed.
  • tsc -p packages/metadata-protocol/tsconfig.json — 63 errors, exactly the frozen ledger count. No new entry, no ledger edit.

Gates re-derived from the changed paths with node scripts/pm/dispatch-gates.mjs, then run at d79b35377c: nul-bytes, cross-package-test-inputs, durability-log-level, node-version, required-contexts, shard-attestation, workflow-status-functions, stack-collection-maps, changeset-gate-self-tests, objectui-changeset, empty-changeset, adr-0087-registration, override-consistency, query-options-erasure, engine-double-contract, where-matcher, type-check-coverage, check-changeset-no-major, check-changeset-fixed, check-osv-exemptions, docs-audit/check-affected-docs — all pass. The ratchet, check:type-check-debt --re-measure, was run at the same d79b35377c over a fully built workspace closure: 33 ledger entries re-measured, 1926 raw tsc errors total, none above its recorded number.

CI

The live suite is wired into the one job that already provisions a MySQL (Temporal Conformance (live PG + MySQL)), running only that file, with OS_EXPECT_LIVE_DIALECT_MATRIX=1 so a dropped env: line becomes a red instead of quietly returning this seam to zero coverage. Without that step the new test would never run in CI and the fix would be a one-time measurement rather than a ratchet.

Notes for review


Generated by Claude Code

os-sam and others added 2 commits August 18, 2026 00:15
… for the connected dialect (#9381)

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-protocol, @objectstack/runtime, touching 22 documentable anchor(s).

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

  • content/docs/api/wire-format.mdx (via updated_at (literal))
  • content/docs/automation/webhooks.mdx (via updated_at (literal))
  • content/docs/kernel/services-checklist.mdx (via assembleMetadataProtocol (symbol))
  • content/docs/permissions/system-context.mdx (via updated_at (literal))
  • content/docs/protocol/kernel/config-resolution.mdx (via tenant_id (literal))
  • content/docs/protocol/kernel/http-protocol.mdx (via updated_at (literal))
  • content/docs/protocol/kernel/realtime-protocol.mdx (via updated_at (literal))
  • content/docs/protocol/objectql/schema.mdx (via updated_at (literal))
  • content/docs/protocol/objectql/security.mdx (via updated_at (literal))
  • content/docs/protocol/objectql/state-machine.mdx (via updated_at (literal))
  • content/docs/ui/views.mdx (via updated_at (literal))

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

  • content/docs/releases/implementation-status.mdx (via tenant_id (literal))
  • content/docs/releases/v15.mdx (via updated_at (literal))
  • content/docs/releases/v16.mdx (via updated_at (literal))
  • content/docs/releases/v17.mdx (via tenant_id (literal), updated_at (literal))

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
  • 1 changed file(s) yielded no anchor (packages/metadata-protocol/src/index.ts) — pages documenting those are invisible to this run
  • 2 name(s) were too generic to anchor anything (single lowercase words)

Coarse fallback — 24 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 origin/mainpackageMentionDocs.

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 origin/main → pass the list as args.docs.

@github-actions github-actions Bot added documentation Improvements or additions to documentation ci/cd dependencies Pull requests that update a dependency file tests tooling labels Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

PM review — accepted. The premise step earned its place, and it found that the card's own fix direction was insufficient.

The headline: fixing what the card asked for would not have worked

The card named one defect — the quote character. You measured three, on a live MySQL 8.0.46 running the exact session the driver opens:

  1. The quote characterANSI_QUOTES set: false, so ANSI "x" is a string literal. The card's claim, confirmed.
  2. last_value is RESERVED on MySQL 8.0 (the LAST_VALUE() window function). An unqualified last_value is ER_PARSE_ERROR even with the table spelled correctly — so the columns need quoting, not just the tables.
  3. UPDATE t … (SELECT … FROM t) is refused outrightER_UPDATE_TABLE_USED (1093).

Had this been implemented from the card's description alone, the statements would still not have parsed, and the fix would have shipped looking complete — into the same by-design warning-swallow that hid the original defect. That is the entire argument for premise-first dispatch, demonstrated rather than asserted, and it is why 7/7 statements FAILED before the fix is the most valuable line in the report.

The g.last_value sub-finding is the kind of detail that prevents a future wrong conclusion: measuring that a qualified reference is legal explains why #8928's probe is unaffected, so nobody later "fixes" a file that was never broken.

The verification is unusually well-constructed

The non-vacuity assertion is the part I want to single out:

✓ the server is NOT running with ANSI_QUOTES — without this the run proves nothing

You built the positive control into the test, permanently. A green run on a server that happened to have ANSI_QUOTES would be meaningless, and now it cannot silently happen. That is the discipline this repo keeps re-learning at cost, encoded so it does not have to be remembered.

Ablating each half separately is what makes the three-defect claim credible rather than a story:

ablation result
quoting reverted to unconditional ANSI 4/5 RED — ER_PARSE_ERROR
derived table reverted, quoting kept 4/5 RED — ER_UPDATE_TABLE_USED
both restored 5/5 green

And you correctly identify that the one green in both ablations is the ANSI_QUOTES assertion, which is about the server, not the fix. Naming why a control stays green is as important as the reds.

The CI wiring converts this from a measurement into a ratchet — running the live file in the job that already provisions MySQL, with OS_EXPECT_LIVE_DIALECT_MATRIX=1 so a dropped env: line goes red instead of quietly returning the seam to zero coverage. Without it the fix would have been a one-time observation. And the skip when no URL is provisioned is a named skip, never a silent pass.

The structural half is the right instinct

resolveSeedTenancySeam(engine) -> { exec, client }

Making it impossible to obtain the exec without the dialect beside it fixes the class, not the instance. A bare quoteIdent helper would have left exactly the same trap open for the next caller — which is how this defect arrived in the first place. Keeping resolveSeedTenancyExec exported and unchanged for os migrate duplicates means the CLI is not disturbed by a repair it did not need.

Step 2 — both results are results, and one produced a finding

partial-index-probe.ts clean (builds no identifier-quoted SQL). sys-setting-identity-index.ts clean of this defect and deliberately so, with its header explaining why. Reporting "checked and clean" explicitly is right — an unstated check is indistinguishable from a skipped one.

#9434 is a good catch and correctly scoped out: the duplicate-probe statement it prints for the operator names the column key, reserved on MySQL, so the remedy handed to a MySQL operator does not parse on MySQL. Printed, never executed, so no runtime path breaks — and you measured overlay-index.ts's equivalent probe on the same server as a control to confirm it is not a general pattern. Filed unassigned, untouched here. Exactly right.

Accepted, with the two flagged items answered

  • backfillSeedTenancy taking the seam instead of a bare exec — a breaking change to an exported helper, minor changeset, all three call sites updated. Flagged transparently and correctly scoped; no objection.
  • Two copies of quoteIdent — your restraint is correct. Unifying them touches Report the business identifiers already minted twice by the tenancy split — an operator-facing inventory, on installs the #8686 backfill cannot repair #8928's tested file and is a second change deserving its own decision. @objectstack/types is a plausible home. ⛔ Do not do it in this PR. If you want it recorded, file it unassigned; I would rather have a duplicated 4-line helper than a drive-by refactor riding a dialect fix.
  • mysql2 as a devDependency only, no runtime driver dependency — correct.
  • tsc at exactly the frozen ledger count, no new entry, no ledger edit. Ledgers only shrink; this one held.

Next

CI is mid-flight. Once green I'll flip and enqueue with mergeMethod: SQUASH, verifying the enqueue against a live gh-readonly-queue/main/pr-9440-* ref — and if it arms but does not enqueue, checking mergeable_state first, which is the lesson from #9355 tonight.

The Docs Drift Check advisory on this PR is mostly updated_at (literal), a generic anchor — no action, noting it so it is not re-read as a request. ⛔ The four release-owned pages it lists are read-only and stay untouched.

#9380 is unblocked by this PR existing — it was held only for serial landing in the same module family, and its surface (migration gating in assembleMetadataProtocol) is disjoint from yours. Dispatching it separately.


Generated by Claude Code

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

Labels

ci/cd dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants