Skip to content

fix(driver-sql): introspection trio — error contract (#7332 extended), covering-PK membership, declared column order - #11203

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-11161-introspection-trio
Aug 23, 2026
Merged

fix(driver-sql): introspection trio — error contract (#7332 extended), covering-PK membership, declared column order#11203
os-zhuang merged 3 commits into
mainfrom
claude/issue-11161-introspection-trio

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#11161
Fixes#11162
Fixes#11163

Three defects, three commits (one per issue, separately revertable), one file region: the introspection internals of packages/drivers/driver-sql/src/sql-driver.ts. All live measurements below were taken in this session against PostgreSQL 16.13 (timezone=Asia/Shanghai) and MySQL 8.0.46 (+08:00), process TZ=America/New_York, OS_EXPECT_LIVE_DIALECT_MATRIX=1 — CI's exact Temporal Conformance posture, so no live cell could skip silently.

Commit 1 — #11161: a failed PK/FK/unique read throws instead of reporting absence

Shape inherited from #7332, verbatim.introspectIndexes already carries the ruling: an options object whose only key is onFailure, accepting 'throw' or 'partial', defaulting to 'throw', with the catch rethrowing unless the caller asked for a short read by name. The three siblings (introspectPrimaryKeys, introspectForeignKeys, introspectUniqueConstraints) now carry the identical option shape, identical default, and the same catch comment. No new contract was designed: no new option, no different default, no new error type — the underlying driver error surfaces undecorated. introspectSchema's four per-table reads all take the default; its in-tree callers already handle a throw (introspectColumns in the same loop has never swallowed).

Measured before/after (live PG 16.13):introspectPrimaryKeys on a non-existent relation — before: resolves [] (the undefined_table error swallowed; [] reads downstream as "this table has no primary key"); after: rejects with the Postgres error, SQLSTATE 42P01 on code, relation named in the message. Pinned in sql-driver-introspection-error-contract.test.ts, plus a dialect-independent pool-sabotage leg on all three cells (destroyed pool: before resolves [], after rejects) and positive legs proving onFailure: 'partial' still resolves (empty after a failure, full on a healthy read).

The un-hiding immediately caught a real defect. With the throw in place, 9 live-PG tests across 4 files went red on one root cause: the Postgres arm of introspectUniqueConstraints was invalid SQL all along — it selected c.column_name while the only aliases in scope are tc and ccu, so every execution raised missing FROM-clause entry for table "c", which the old catch converted to []. Live Postgres has therefore never reported a unique constraint through this method. Repaired in the same commit (this is the card's own defect class — a failed read silently converted to absence — with the correct spelling pinned by the FK query one method up): alias corrected to ccu.column_name, and the lookup scoped with tc.table_schema = ANY (current_schemas(false)), the same pin introspectSchema's table listing uses (#9350 pattern), so the newly-working query cannot report a same-named table's constraints from another schema. isUnique is now populated on live Postgres for the first time; the changeset says so.

Changeset: minor, declared BREAKING (calls that resolved now reject), with an ADR-0087 disposition marker in the changeset body (not-required, no-migration-prescription: no authorable metadata key changes shape).

Commit 2 — #11162: covering primary keys stop reporting INCLUDE'd columns as key members (PG-only)

Measured (live PG 16.13), fixture (k1, k2, payload) with CREATE UNIQUE INDEX ... (k2, k1) INCLUDE (payload) promoted via ADD CONSTRAINT ... PRIMARY KEY USING INDEX: catalog reports indkey = '2 1 3', indnatts = 3, indnkeyatts = 2. Before: introspected key k2, k1, payload (both the pre-#11101 and post-#11101 queries agreed on the wrong membership; #11101 changed only order). After: k2, k1 exactly.

Fix is the single bound k.ord at most i.indnkeyatts in the WHERE clause — #11164's ordering machinery (unnest ... WITH ORDINALITY, ORDER BY k.ord) is untouched, and the pin asserts the exact ordered array so membership and order are held simultaneously (the fixture's key is deliberately declared out of column sequence). MySQL has no covering-PK concept and SQLite no INCLUDE, so the suite is PG-only by nature, declared through declareDialectCell (named skip unprovisioned, red under OS_EXPECT_LIVE_DIALECT_MATRIX=1). Catalog-facts leg pins indkey/indnatts/indnkeyatts so a server-side change is localisable. indnkeyatts exists on PG 11+.

Commit 3 — #11163: columns in declared order, from the catalog ordinal

Measured (live MySQL 8.0.46, cold data dictionary right after server install, knex's own unordered information_schema.columns query shape):carrier_code, leg_seq, shipment_id — alphabetical, for a table declared carrier_code, shipment_id, leg_seq. SQLite and PG 16.13 returned declared order for the same table. After: declared order on all three dialects.

Honest note on the pre-fix order: later in the same session, the identical query on the identical server returned declared order — the same statement that measured alphabetical two hours earlier. The row order genuinely flips with dictionary/plan state, which is the issue's own point: the query specifies no order, so the answer is "unspecified" regardless of what a given run shows. Consequence for the pin: the new test is deterministically green under the fix (explicit ordinal ordering), and its red-on-regression sensitivity on MySQL depends on server state — the cold-dictionary state CI's fresh mysql:8.0 service is in on every run is the state measured returning alphabetical. The catalog-facts leg pins the fact the fix rests on (the ordinal is the declared position) independent of that whim, and the reverse-verification table below records exactly which legs went red.

Cost decision the card asked to be explicit about:columnInfo() was not replaced wholesale. It remains the source of the per-column facts (type, nullable, defaultValue, maxLength — each dialect spells them differently and knex already normalises them); only the order comes from a new per-dialect ordinal read (ORDINAL_POSITION on MySQL, ordinal_position scoped exactly as knex scopes columnInfo on PG, PRAGMA table_info's cid on SQLite). The merge reorders but can never drop a column: a name missing from either read is appended in columnInfo() order. Fixtures: the #11101 permutation shape reused (os11163_shipment_legs), plus a second table whose alphabetical order differs in the first position (zone_code first declared, last alphabetically), with a non-vacuity leg pinning both constants against their own DDL text.

Reverse verification (from the committed state, sql-driver.ts restored to origin/main, fixes' tests kept)

Predicted 7 reds, observed 6 — every red is the pinned defect resolving/answering wrongly, every green is a leg designed to survive (partial-opt-in legs pass under the old code because the extra argument is ignored at runtime; non-vacuity and catalog pins do not test the fix):

Also recorded: the first full-suite run with the new throw and without the ccu repair failed 9 tests across 4 live-PG files on the invalid unique-constraints query — the diagnostics-increase direction of the contract change, and the run that discovered the latent defect.

Type-surface verification

onFailure lands in the rebuilt dist declarations (5 occurrences); a scratch tsc probe against the built .d.ts rejects onFailure: 'swallow' with TS2322 naming the 'throw' | 'partial' | undefined vocabulary and accepts the valid spellings — proving the read declarations are the rebuilt ones, not cache. The two in-tree subclasses (driver-sqlite-wasm, driver-turso, consumer direction: downstream packages extending SqlDriver) override none of the four methods and typecheck green after the rebuild.

Verification at aa44f566c (final commit; all runs below on this tree)

  • pnpm --filter @objectstack/driver-sql test with live matrix env: 118 files, 2419 tests, all passed, 0 skipped (vitest summary; both live cells provisioned and executed — verbose re-run of the three new files: 24/24 across sqlite, live postgres, live mysql).
  • pnpm --filter @objectstack/driver-sql typecheck, plus driver-sqlite-wasm and driver-turso typecheck: green (tsc, no output).
  • pnpm lint (full repo eslint scan, --no-inline-config): exit 0 under the verify lock ("VERDICT command-exit 0").
  • node scripts/pm/dispatch-gates.mjs (no paths — change set derived from merge base): 12 path-derived + 5 convention-triggered gates, all run, all green, exits captured before any pipe: check:nul-bytes, check:adr-anchors (named by dispatch by hand), check:changeset-gate-self-tests, check:driver-conformance, check:objectui-changeset, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check-adr-0087-registration, check-changeset-no-major, check-ci-filter-parity, check-empty-changeset, check-plugin-teardown-shape, docs-audit/check-affected-docs, check:query-options-erasure, check:engine-double-contract, check:where-matcher, check:cross-package-test-inputs, and the two type-check ratchets after the full workspace build (check:type-check-coverage "OK — 65/78", check:type-check-debt --re-measure "none above its recorded number").

Out of scope, filed

Generated by Claude Code


Generated by Claude Code

…ead of reporting absence
Extends the #7332 ruling that introspectIndexes already carries to its three
siblings, with the identical option shape and default: onFailure?: 'throw' |
'partial', defaulting to 'throw'. A bare catch {} returning [] converted a
failed read into a positive assertion of absence — and primaryKeys is consumed
as an addressing / upsert-conflict-target key, so downstream code acted on the
wrong answer. introspectSchema's four reads all take the default; every
in-tree caller already handles a throw.
Fixes#11161
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
…UDE'd columns are not key members
A covering primary key (CREATE UNIQUE INDEX ... INCLUDE (payload) promoted via
ADD CONSTRAINT ... PRIMARY KEY USING INDEX) carries its payload columns in
pg_index.indkey; indnkeyatts counts the leading entries that are key members
and was never consulted, so payload was reported as part of the key. Measured
on PostgreSQL 16.13: indkey='2 1 3', indnkeyatts=2. The bound k.ord <=
i.indnkeyatts fixes membership while preserving #11101's declared key ORDER.
Fixes#11162
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
…g ordinal, not columnInfo() key order
knex's columnInfo() is an object keyed by column name with no ORDER BY behind
it; on MySQL 8.0.46 the row order is alphabetical, so the same table
introspected through different dialects returned different columns arrays and
federated-object drafts got their fields alphabetized. The order now comes
from the catalog's ordinal on all three dialects (ORDINAL_POSITION /
ordinal_position / PRAGMA table_info cid); columnInfo() remains the source of
the per-column facts, which knex already normalises per dialect.
Fixes#11163
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 7 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 221 client-bound route-ledger rows — the other 176 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 18d5fec196de76c5054901521d01732f28e5128apackageMentionDocs.

Which tree this was computed on

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

⚠️ 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 18d5fec196de76c5054901521d01732f28e5128a → 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 23, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 23, 2026 03:11
@os-zhuang
os-zhuang added this pull request to the merge queueAug 23, 2026
Merged via the queue into main with commit 9cc1940Aug 23, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11161-introspection-trio branch August 23, 2026 03:25
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment