Skip to content

test(driver-sql): measure what MySQL does with a conflict target it cannot honour - #8624

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-8592-mysql-conflict-target-behaviour
Aug 14, 2026
Merged

test(driver-sql): measure what MySQL does with a conflict target it cannot honour#8624
os-zhuang merged 1 commit into
mainfrom
claude/issue-8592-mysql-conflict-target-behaviour

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Part of #8592 — steps 1 (measure) and 3 (make the cell structurally honest), which is the scope triage cut for dispatch. Step 2 (changing MySQL's accept set) is deliberately not here; it is filed as #8621 with its own argument. See "what this PR leaves" at the bottom.

The bug this found on the way in

The card's premise was that the MySQL half could not be measured because no runner has a server. That is wrong, and the correction is the substance of this PR. CI's Temporal Conformance (live PG + MySQL) job attaches a mysql:8.0 service and runs pnpm --filter @objectstack/driver-sql test with OS_TEST_MYSQL_URL and OS_EXPECT_LIVE_DIALECT_MATRIX=1. The server was present and unused.

Unused because the MySQL cell was guarded one way only, with no else:

if(!MYSQL_CELL.available){declareUnprovisionedCell(MYSQL_CELL,'unbacked conflict-target refusal (behaviour never observed)');}
jobOS_TEST_MYSQL_URLwhat the MySQL half did
Test Coreabsentdeclared itself un-run — visible
Temporal Conformancepresentdeclared nothing, measured nothing — invisible

The declaration disappeared exactly when the capability to measure appeared, and OS_EXPECT_LIVE_DIALECT_MATRIX=1 could not catch it because a cell emitting no suite at all is not a skip. This is not a criticism of #8591 — its declaration was correct for the container it was written in.

The measurement

Raised a real server in the dev container rather than inferring: system MySQL 8.0.46 (Ubuntu noble mysql-server, mysqld --daemonize, default_time_zone='+08:00'), driven through the same knex + mysql2 path upsert takes. email is named in conflictKeys and has no unique index; tax_id carries the only unique index.

seed upsert({email:'a@b.com', tax_id:'T-1', title:'first'}, ['email'])
-> RESOLVED. rows=[{id:'VBjOQwQp3uTtewte', email:'a@b.com', tax_id:'T-1'}]
B upsert({email:'other@b.com', tax_id:'T-1', title:'second'}, ['email'])
-> RESOLVED. rows=[{id:'RnSaXzGO69OKkP_D', email:'other@b.com', tax_id:'T-1'}]
ONE row: merged on `tax_id`, across two DIFFERENT `email` values,
and the surviving row's PRIMARY KEY changed.
D seed, then upsert({email:'a@b.com', tax_id:'T-2'}, ['email'])
-> RESOLVED. TWO rows, both email='a@b.com'.

Four facts, now pinned:

  1. No refusal. The identical call is VALIDATION_ERROR / 400 on SQLite and Postgres.
  2. It merges on a key the caller never named — a wrong write with no error. The card's inference was right about this.
  3. It replaces the merged row's primary key.id sits in the merge set, so id = values(id) overwrites the stored row's identity with the nanoid minted for the insert that lost. The inference did not contain this, and it is the sharpest edge — every external reference to the old id dangles silently.
  4. It does not merge on the key it was given — duplicates on email.

So MySQL fails in both directions at once. ⚠️ The new pins are characterizations of a defect and say so loudly in the code; when #8621 lands they go red and must be rewritten to the refusal, not relaxed.

Step 3 — the structural fix

declareDialectCell(cell, matrix, measure) in live-dialect-matrix.testkit.ts is a total function: measure is required, so the half-pair form no longer typechecks. The other six consumers of declareUnprovisionedCell were checked and are already two-way (for … if (!available) … continue), so none of them needed changing.

Reverse verification — direction predicted before each leg

  • Leg 1a (predicted: red at typecheck, not runtime). Dropped the measure argument: TS2554: Expected 3 arguments, but got 2. Matched.
  • Leg 1b (predicted: the gap returns silently, green). Restored the original one-way block with a live MySQL attached and OS_EXPECT_LIVE_DIALECT_MATRIX=1: 19 passed → 14 passed, still green, tsc exit 0. The five MySQL observation tests vanished with no signal anywhere. That is the defect reproduced exactly.
  • Honest bound on the guard. Leg 1b also shows what the combinator does not catch: a wholesale revert to the one-way form typechecks fine. It makes the half-pair unrepresentable, not the deletion — true of any test, and worth stating rather than overclaiming.
  • Positive control (predicted: unchanged). SQLite and Postgres cells pass identically — 6 + 6 tests, the sweep code untouched by this diff.

Verification

Both live servers were raised locally, so this ran in CI's exact shape rather than a subset — TZ=America/New_York, live PG 16.13 at Asia/Shanghai, live MySQL 8.0.46 at +08:00, OS_EXPECT_LIVE_DIALECT_MATRIX=1:

pnpm --filter @objectstack/driver-sql test
Test Files 98 passed (98)
Tests 2011 passed (2011)
target file, all three cells: 19 passed (19)
tsc --noEmit: clean

Un-provisioned paths also checked both ways: with no URLs the cell reports a named skip; with OS_EXPECT_LIVE_DIALECT_MATRIX=1 and URLs dropped it is a named red.

Gates re-derived against the actual changed paths (scripts/pm/dispatch-gates.mjs) and run: check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:type-check-coverage, check:nul-bytes — all green.

Changeset

skip-changeset. Tests plus a test-only helper — live-dialect-matrix.testkit.ts is not exported from index.ts (verified). No runtime path changed, nothing consumer-visible moved, so a changeset would announce a release that contains nothing.

What this PR leaves

Part of rather than Fixes because the card body's step 2 — decide, and possibly refuse pre-flight — is not done here: triage excluded it from dispatch as a minor needing its own argument, and it is now #8621. Steps 1 and 3 are complete. If the re-scope is taken as authoritative, #8592 can be closed on merge; that call is the PM's, not this PR's.

Findings filed on the way, both unassigned: #8621 (the accept-set decision, with the measurement as its evidence) and #8622 (the primary-key rewrite, which reproduces on SQLite too and is not MySQL-specific). Neither is addressed here.

Generated by Claude Code


Generated by Claude Code

…annot honour (#8592)
The MySQL cell of the unbacked-conflict-target matrix was guarded one way
only -- `if (!cell.available) declareUnprovisionedCell(...)` with no else.
With OS_TEST_MYSQL_URL absent it announced itself un-run; with it PRESENT,
on the one CI job that attaches a live MySQL 8.0, it declared nothing and
measured nothing. The declaration disappeared exactly when the capability
to measure appeared, and OS_EXPECT_LIVE_DIALECT_MATRIX could not catch it
because a cell that emits no suite is not a skip.
Structural fix: `declareDialectCell(cell, matrix, measure)` in the shared
testkit is TOTAL -- `measure` is required, so the one-way form no longer
typechecks. The other six consumers were checked and are already two-way.
Measured on live MySQL 8.0.46 (system mysql-server, mysqld --daemonize),
through the same knex + mysql2 path upsert takes. Table: `email` named in
conflictKeys with no unique index, `tax_id` carrying the only unique one:
- it does NOT refuse -- the identical call is VALIDATION_ERROR/400 on
SQLite and Postgres;
- it MERGES on `tax_id`, the key the caller never named, across two
different `email` values -- a wrong write with no error;
- it REPLACES the merged row's primary key while doing so (`id` sits in
the merge set), which the card's inference did not contain;
- it does NOT merge on `email`, the key it was given -- duplicates.
The new pins are characterizations of a defect and say so; #8621 is the
card that moves MySQL's accept set and will turn them red.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VoxQqG5FiUHZKCST7KDoZC
@vercel

vercelBot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 14, 2026 2:50am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql.

8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via @objectstack/driver-sql)
  • content/docs/getting-started/glossary.mdx(via @objectstack/driver-sql)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/driver-sql)
  • content/docs/plugins/anatomy.mdx(via @objectstack/driver-sql)
  • content/docs/plugins/packages.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/kernel/lifecycle.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/objectql/query-syntax.mdx(via @objectstack/driver-sql)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/driver-sql)

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.

Advisory only. 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.

@os-zhuangos-zhuang added skip-changeset PR has no user-facing published change; bypasses the changeset gate and removed size/m labels Aug 14, 2026 — with Claude
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — reviewed against the tree at 5718d24f.

Verified independently of the description:

  • The structural fix is total, not conventional.declareDialectCell(cell, matrix, measure) takes measure as a required third parameter, so the half-pair form stops typechecking rather than merely being discouraged. That is the difference between a rule and a guard — Prime Directive chore: version packages #10 territory, and it lands on the right side of it.
  • Test-only.git diff --name-only outside tests/testkit returns nothing. The scope guard triage set held: no accept-set change rode along.
  • The defect pins are labelled as defect pins, loudly, with the instruction that when drivers(sql): MySQL silently merges an upsert on a unique key the caller never named, and rewrites the merged row's primary key — measured, and it is a wrong write with no error #8621 lands they must be rewritten to the refusal, not relaxed to stay green. That instruction is the thing that keeps a characterization pin from silently becoming a contract.
  • Leg 1b is the honest one. Restoring the one-way block with a live MySQL attached went 19 → 14 passed, still green, tsc clean — the five observation tests vanished with no signal anywhere. That is the defect reproduced rather than argued. Equally, stating that the combinator makes the half-pair unrepresentable but not the wholesale deletion is the right kind of bound to declare instead of overclaiming.

On the measurement: two of my own premises were wrong, and the dev's work is what corrected them. I reported this container has "no server binaries of any kind" for MySQL and treated that as not measurable here. The first half was true, the inference was not — mysql-server installs from the distro, and the dev raised MySQL 8.0.46 locally and got the answer. I will stop treating "not currently installed" as "not measurable" when gating cards; that framing already cost this card family one unnecessary hold.

The observation also beat the inference in substance. The card predicted a merge on the wrong key; the measurement additionally found the surviving row's primary key is replaced, and that fact is not MySQL-specific.

Disposition of #8592 — closing it on merge

The PR asks the PM to make this call, correctly. Steps 1 and 3 are the dispatched scope in full, and step 2 was cut from dispatch by triage before this card was claimed, not dropped by the dev — it is now #8621 with the measurement as its evidence. So #8592's scope is complete on merge and I will close it manually, since Part of will not auto-close it. #8621 carries the remainder.

⚠️#8622 is the most consequential thing this run produced

Flagging it for triage priority rather than letting it queue as an ordinary finding. It is not an error-path curiosity: a properly-backed unique: true conflict target — the supported ingest path — silently re-identifies the row it merges into, measured on SQLite and MySQL alike. id rides in the merge set because insertOnlyUpsertColumns covers created_at and auto_number but not the primary key, and it stayed invisible because on the default ['id'] path the clause is a no-op.

Every relationship, audit record and external id mapping pointing at that row dangles, with no error on any dialect. #7011 already accepted exactly this argument for auto_number ("an externally visible business identifier once assigned"); the primary key is the stronger instance. Note it is filed against SQLite too, so it is not gated on anyone having MySQL.

Holding ready + queue until the checks report green.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 14, 2026 03:05
@os-zhuang
os-zhuang added this pull request to the merge queueAug 14, 2026
Merged via the queue into main with commit 424bbd4Aug 14, 2026
33 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8592-mysql-conflict-target-behaviour branch August 14, 2026 03:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-zhuang@claude