Skip to content

fix(driver-sql): a PRIMARY KEY is not a unique constraint on the SQLite introspection arm (#11654) - #11827

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-11654-sqlite-pk-unique
Aug 24, 2026
Merged

fix(driver-sql): a PRIMARY KEY is not a unique constraint on the SQLite introspection arm (#11654)#11827
os-warren merged 1 commit into
mainfrom
claude/issue-11654-sqlite-pk-unique

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#11654

SqlDriver.introspectUniqueConstraints reached its answer from a different catalog per dialect, and the catalogs disagreed about primary keys. Postgres and MySQL filter on CONSTRAINT_TYPE = 'UNIQUE', which never matches a primary key. The SQLite arm iterated PRAGMA index_list keyed only on idx.unique === 1, never on origin — and SQLite materialises a non-INTEGER primary key as a unique auto-index, so the key column was reported as unique.

SQLite also disagreed with itself: an INTEGER PRIMARY KEY is a rowid alias for which SQLite creates no auto-index at all, so index_list is empty and such a key was never flagged. The same logical schema produced a different isUnique from the declared type of its key alone.

The SQLite arm now skips origin: 'pk' rows, which closes both gaps in one stroke.

The convention this inherits, and its reason

This is not a new decision — it is the convention #11202 landed (PR #11657), applied one cell over. isUnique means a declared single-column UNIQUE constraint. Primary-key membership already has a lossless face of its own — IntrospectedTable.primaryKeys and IntrospectedColumn.primaryKey — so excluding keys from isUnique drops no fact and leaves the two flags non-overlapping.

Note the direction, because it differs from #11202: the flag being removed here was not false — a primary-key column really is unique — only inconsistent. That is exactly why the exclusion has to be paid for by the face that keeps the fact, and why this PR pins primaryKeys and primaryKey as part of the change rather than only asserting the absence.

The filter is on the INDEX's origin, not on whether the COLUMN is in the key. Those are different changes and only one is correct: a key column that separately carries its own unique index really does have a declared single-column unique constraint and must stay flagged. t_pk_and_idx is that distinction as a pin — it fails under the wrong sibling implementation and passes under this one.

Premise re-measure against origin/main

The card is a lead, not a spec, and the triage fence asked for a re-measure on the post-#11202 ref specifically because PR #11657 touched this exact producer the same day. Re-measured at merge base dce6a019f on embedded better-sqlite3, through the driver itself. The cell was not absorbed; all of the card's readings reproduce.

PRAGMA index_list(t_text) ->
{ name: 'sqlite_autoindex_t_text_2', unique: 1, origin: 'u', partial: 0 }
{ name: 'sqlite_autoindex_t_text_1', unique: 1, origin: 'pk', partial: 0 }
PRAGMA index_list(t_int) -> []
introspectUniqueConstraints(t_text) = ["email","id"] <- 'id' is the PRIMARY KEY
introspectUniqueConstraints(t_int) = []
introspectSchema().tables['t_text']: primaryKeys = ["id"]
column id: primaryKey=true isUnique=true <- the defect
column email: primaryKey=false isUnique=true
introspectSchema().tables['t_int']: primaryKeys = ["id"]
column id: primaryKey=true isUnique=false <- same schema, different answer

premise_still_valid: true. The PG/MySQL half is read from the arms' own SQL (CONSTRAINT_TYPE = 'UNIQUE'); live PG and MySQL are not provisioned in this container, so those cells are named skips locally and run in the Temporal Conformance (live PG + MySQL) job — the same posture #11657 recorded.

Two shapes were measured that the card does not name, and both changed the test design rather than merely confirming it:

Reverse verification

Pinned first against unfixed source, as dispatched. Both legs measured on the full @objectstack/driver-sql suite.

RED (new pins, unmodified arm) — 4 failed / 1972 passed / 112 skipped, quoting the assertions:

× reports the UNIQUE column and NOT the primary-key column
AssertionError: expected [ 'email', 'id' ] to not include 'id'
× `introspectSchema` folds that into `isUnique` — the consumer-visible half
AssertionError: expected true to be falsy
× an INTEGER key and a varchar key now agree — neither is flagged
AssertionError: expected [ 'email', 'id' ] to deeply equal [ 'email' ]
× a WITHOUT ROWID key is not flagged either
AssertionError: expected [ 'email', 'id' ] to deeply equal [ 'email' ]

GREEN (after the filter) — Test Files 127 passed | 8 skipped (135), Tests 1976 passed | 112 skipped (2088).

The four pins that must hold in both directions were green in the RED leg too, which is what makes the red meaningful rather than a broken fixture: the non-vacuity case, the primaryKeys face, the composite key, and the key-column-with-its-own-index case. Exactly 4 tests moved, 1972 → 1976 passed, nothing else.

The fixtures assert the absence is real before asserting it. "a PK column is not flagged" goes green for free on a table whose key never landed, so every cell first makes the database its own witness: a repeated key is rejected and a repeated email is rejected. That also states plainly what the flag's absence does and does not mean — the key column really is unique; what changed is which kind of constraint the flag reports.

Verification

Gate union re-run at the final commit 66d9634df, after the commit, from the worktree. Derivation quoted from the script itself:

dispatch-gates: gate list derived from the tree of 'objectstack-ai/objectstack' at commit 66d9634df
--repo 'objectstack-ai/objectstack' checked against this checkout's 'origin' remote — it holds.
dispatch-gates: change set derived from git — 3 path(s) vs merge base dce6a019f of 'origin/main' and HEAD

14 path-derived families plus 6 convention-triggered (this adds a test file). All 20 exit 0, each captured before any pipe. Selected verdict lines, quoted from the gates themselves:

  • check-driver-conformance: OK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt.
  • check-engine-double-contract: OK — 401 pinned, 133 in the DEBT ledger, 2 exempt.
  • OK: 16 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
  • check-nul-bytes: OK (scanned 6593 text file(s) ... no raw ASCII control bytes).
  • check-where-matcher: 0 silently-wrong and 0 unjudged matcher(s) ... none new.
  • check-changeset-no-major: ✓ This diff introduces no 'major' bump.

Per-package readings, both at 66d9634df (working tree clean at commit time, so the tested tree is the committed tree):

  • @objectstack/driver-sql — 127 files passed, 8 skipped; 1976 tests passed, 112 skipped. typecheck exit 0 (tsc --noEmit echoed, so not a zero-match silent pass).
  • @objectstack/objectql — the consumer check: src/util.test.ts, 1 file / 22 tests passed. This is the suite covering introspectedSchemaToObjects, the flag's one real consumer.

Supporting evidence from the consumer's own fixture, which was not part of the dispatch and is worth a reviewer's eye: packages/objectql/src/util.test.ts already models a key column as { primaryKey: true } with noisUnique, and email as isUnique: true. The consumer's test data has always encoded option A. Nothing in it needed changing, and no fixture triage was required.

Two declared narrowings

Both are declared rather than silently skipped, and CI runs the full farm regardless.

  1. check:type-check-debt --re-measure was not run locally. It re-runs tsc per ledger entry and needs the whole workspace closure built. Its population is read from the gate's own model, not guessed: driver-sql carries no test-typecheck-debt.json and appears nowhere in the gate's DEBT / TEST_DEBT / EXEMPT / UNCHECKED_SOURCE_DEBT / PHANTOM_PIN_DEBT literals (only in its header's history prose, as debt already paid). The structural half, check:type-check-coverage, passed, and the package's own tsc --noEmit is green. The diff adds no file to any ledgered package.

  2. Repo-wide pnpm lint was narrowed to the changed files, and the narrowing is a measurement rather than a skip — all three pieces:

    • Population, read from eslint's own config (ESLint#isPathIgnored / calculateConfigForFile), not from an assumption about which files count: of the 3 changed paths, exactly 2 are in the linted population (6 and 5 rules); the changeset .md is not linted at all.
    • Count, from --format json: 2 files linted, 0 errors, 0 warnings, exit 0.
    • Invariance for untouched files: eslint.config.mjs states in its own text that this repo "never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file". With no cross-file type information in play, this diff cannot move the verdict on any file it does not touch.

    The container was saturated at measurement time (load 4.41 on 4 cores, three waiters queued behind an active build), where a repo-wide scan reliably meets the foreground cap and yields nothing.

Scope

Declared surface, unchanged from the claim: packages/drivers/driver-sql/src/sql-driver.ts (the SQLite arm of introspectUniqueConstraints plus the two TSDoc contract sentences in the same file), one new sibling test file, and the changeset. Nothing else.

H17 fence — both holds name sql-driver.ts as a trigger file, and the diff stays clear of both regions. Neither sqliteCanonicalDatetimeSql, backfillCanonicalDatetimes (#6009) nor insertOnlyUpsertColumns / the upsert region (#8740) appears anywhere in the diff; those symbols live at lines 4007–9630 and 6529 / 7023, while the three hunks are at 3701, 14775 and 14887.

Clause-②: no — driver introspection behaviour, no packages/spec path in the diff.

Drivers extending SqlDriver (driver-turso, driver-sqlite-wasm) inherit the change; both are covered by the package suite above.

One out-of-scope finding, filed unassigned


Generated by Claude Code

…te arm (#11654)
`introspectUniqueConstraints` reached its answer from a different catalog per
dialect, and the catalogs disagreed about primary keys. Postgres and MySQL
filter on `CONSTRAINT_TYPE = 'UNIQUE'`, which never matches one. The SQLite arm
iterated `PRAGMA index_list` keyed only on `unique === 1`, never on `origin` —
and SQLite materialises a non-INTEGER primary key as a unique auto-index, so
the key column was reported.
SQLite also disagreed with itself: an `INTEGER PRIMARY KEY` is a rowid alias
for which no auto-index exists, so `index_list` is empty and the key was never
flagged. The same logical schema produced a different `isUnique` from the
declared type of its key alone.
The arm now skips `origin: 'pk'` rows, which closes both gaps at once
(`WITHOUT ROWID` keys included). This continues the convention #11202 landed:
`isUnique` means a declared single-column UNIQUE constraint. Nothing is lost —
key membership is reported losslessly by `primaryKeys` and
`IntrospectedColumn.primaryKey`, both unmoved.
The test is on the INDEX's origin, not on whether the COLUMN is in the key:
those are different changes and only one is correct. A key column that
separately carries its own unique index really does have a declared
single-column unique constraint and stays flagged — pinned, because it fails
under the wrong sibling implementation.
Measured before the fix on embedded better-sqlite3: `t_text` (varchar key)
reported `['email', 'id']`, `t_int` (INTEGER key) reported `[]`. Four pins go
red against the unfixed arm and green after it; the rest of the package suite
is unmoved (1972 -> 1976 passed, 0 failed).
Claude-Session: https://claude.ai/code/session_01Rxnd8cyFnoU8V5y21PaTsy
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 3 documentable anchor(s).

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

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol))

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

  • content/docs/releases/v17.mdx(via SqlDriver (symbol))

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 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 9 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 e43b18fd9413b5774952eda5b852a54ef423ce2cpackageMentionDocs.

Which tree this was computed on

This run read content/docs from ff9da2fc8f9fa7ad99799aa8393856da655764fd — the merge of head 66d9634df8060a9314fcd7416cd467129a16b0bd into base e43b18fd9413b5774952eda5b852a54ef423ce2c, 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 ff9da2fc8f9fa7ad99799aa8393856da655764fd && git checkout ff9da2fc8f9fa7ad99799aa8393856da655764fd
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e43b18fd9413b5774952eda5b852a54ef423ce2c 66d9634df8060a9314fcd7416cd467129a16b0bd && git checkout -B drift-repro e43b18fd9413b5774952eda5b852a54ef423ce2c && git merge --no-ff 66d9634df8060a9314fcd7416cd467129a16b0bd
node scripts/docs-audit/affected-docs.mjs --json e43b18fd9413b5774952eda5b852a54ef423ce2c

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@os-warren
os-warren marked this pull request as ready for review August 24, 2026 21:39
@os-warren
os-warren added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 7adcd07Aug 24, 2026
32 checks passed
@os-warren
os-warren deleted the claude/issue-11654-sqlite-pk-unique branch August 24, 2026 22:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-warren@claude