Skip to content

docs(service-datasource): make the primaryKeyReader docblock true again, and record why the isPrimary arm stays - #11206

Merged
os-sam merged 1 commit into
mainfrom
claude/issue-11123-primary-key-reader-truth
Aug 23, 2026
Merged

docs(service-datasource): make the primaryKeyReader docblock true again, and record why the isPrimary arm stays#11206
os-sam merged 1 commit into
mainfrom
claude/issue-11123-primary-key-reader-truth

Conversation

@claude

@claudeclaudeBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Part of #11123 — deliberately NOT a closing keyword.

This lands the half that is executable today: the docblock truth repair (ruled, not re-litigable). The other half — whether the union's isPrimary arm is retired — is escalated as a contract-review decision rather than taken here, with the measurement behind it below. #11123 stays open to carry that decision; closing it on this merge would drop the open question into a closed card.

Comment-only. Zero executable lines change — verified mechanically: every added and removed line in this diff is a comment or blank (git diff -U0 filtered for non-comment +/- lines returns empty). The union read in primaryKeyReader is byte-identical; all three arms stay.

What was stale, and the evidence it is stale

The primaryKeyReader docblock is the one place this seam's reasoning is written down. Re-verified on the merged ref (52a41b72ee) before editing — both card claims hold, plus three the card did not name:

#Stale statementEvidence it is false today
1Producer table: SqlDriver spells the per-column signal isPrimary95437e7d2d (#11124). sql-driver.ts:10151 now does if (primaryKeys.includes(col.name)) col.primaryKey = true; and emits no isPrimary
2The #10997 note: the SQLite composite-key truncation "is upstream of this seam and is not repaired here"Repaired in PR #11104. introspectPrimaryKeys filters row.pk > 0 and sorts on the ordinal, so it reports every composite member in declared key order
3Closing note: reading the extra spellings structurally is needed "because reconciling objectql/src/util.ts with the spec contract is a spec-owned change"That reconciliation happened — objectql/src/util.ts now extends Omit<SpecIntrospectedColumn, …>
4refreshCatalog seam comment: "a real driver spells this isPrimary / primaryKeys, never primaryKey"Same evidence as row 1
5Seam suite, second describe: "SqlDriver derives isPrimary FROM primaryKeys"Same evidence as row 1 — it derives primaryKey

Rows 4 and 5 were not named on the card. They are the same defect class (stale prose asserting the retired producer spelling), mechanical, pinned by the same commit, inside the file surface already claimed, and add no verification surface — so they were repaired in place rather than left as a second card that would go stale against this one. Naming them here is the condition for that.

What did NOT change, and why

The union read is untouched.col.primaryKey || col.isPrimary || table.primaryKeys — all three arms.

The isPrimary arm: why it is not a dead-code deletion

The card's claim — no in-tree producer — is true. Swept whole-identifier (\bisPrimary\b), every surviving non-test hit is prose; objectql's isPrimaryKeyField merely contains the substring and is unrelated. But in-tree is not the same population as supported, and two independent measurements say removal would narrow accepted input:

  1. The producer population at this seam is open by design, and the retirement's migration channel cannot reach it.contracts/datasource-driver-factory.ts states the framework "deliberately ships no universal driver-by-id registry — concrete drivers are constructed by the host stack", and types the handle as introspectSchema?(): Promise<unknown>. The retirement shipped as a BREAKING change whose stated migration channel is the compiler: "TypeScript consumers of the removed member are told by the compiler, precisely and at every site" (.changeset/driver-emits-spec-introspection-shape.md). Against a Promise<unknown> handle that channel never fires. A host-built driver still emitting the old spelling is reached by nothing today and would silently lose its remote key if the arm went.
  2. The compatibility belt's clock has not started. The union (fix(service-datasource): read the introspected primary key at the isPrimary/primaryKey seam #11001, .changeset/external-catalog-introspected-primary-key.md) and the retirement (fix(driver-sql): introspectSchema() emits the spec introspection contract — primaryKey, dialect, introspectedAt (#10676, #10998) #11124, .changeset/driver-emits-spec-introspection-shape.md) are both still unconsumed changesets at 17.1.0. No released version has ever emitted primaryKey from this driver. Removing the belt now would ship the narrowing in the same release as the break it exists to absorb.

So the arm is kept, and the docblock now records it as a deliberate compatibility belt with the measurement behind it, so the next reader does not re-derive this. Retiring it is a contract-review decision, not a cleanup — flagged back to the PM rather than taken here.

Its only exercise anywhere in the tree is the staged-disagreement pair in external-introspection-seam.test.ts; the suite docblock now says so, and says those two cases go with the arm if it is ever retired.

No changeset — skip-changeset

Nothing published changes. primaryKeyReader is module-private (never in a .d.ts), and test files are outside the package's files whitelist (check:published-files green). This PR releases nothing, which is what the label is for; this repo rejects the empty-frontmatter form (check-empty-changeset.mjs).

Verification — all on the final commit 57e6a7ccbf

Gate union derived on that commit with no hand-supplied paths: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (repo assertion held; 2 paths vs merge base 52a41b72e).

checkverdict line
pnpm --filter '@objectstack/service-datasource^...' buildVERDICT command-exit 0
pnpm --filter @objectstack/service-datasource typecheckVERDICT command-exit 0
pnpm --filter @objectstack/service-datasource testTest Files 26 passed (26) · Tests 563 passed (563)
pnpm check:published-files✓ check:published-files — 69 publishable package(s) …
pnpm check:slot-lookup✓ slot-lookup ratchet holds … none new
pnpm check:test-source-aliascheck-test-source-alias OK — 72 packages with tests scanned
pnpm check:type-source-resolutioncheck-type-source-resolution OK — 77 packages … scanned
node scripts/check-ci-filter-parity.mjsOK: all 86 declared cross-package glob(s) …
node scripts/check-plugin-teardown-shape.mjs✓ check:plugin-teardown-shape: 63 Plugin implementation(s) …
node scripts/docs-audit/check-affected-docs.mjsexit 0
pnpm check:query-options-erasure✓ query-options-erasure ratchet holds … none new
pnpm check:type-check-coverageexit 0
pnpm check:engine-double-contractcheck-engine-double-contract: OK — 384 pinned …
pnpm check:where-matcher✓ where-matcher conformance holds … none new
pnpm check:cross-package-test-inputsOK: 13 package(s) read outside themselves, all declared

Full workspace closure built for the ratchet gate: turbo run build --filter='./packages/*' --filter='./packages/*/*'Tasks: 70 successful, 70 total, VERDICT command-exit 0.

One declared narrowing.pnpm check:type-check-debt (the --re-measure ratchet half) is NOT MEASURED locally — its prerequisite closure was built, but the run was killed by the container's 10-minute foreground cap (exit 143) on the first attempt and the lock was held by a parallel agent running the same gate on the retry. Not reported as a verdict. What is measured in its place: its structural sibling check:type-check-coverage is green; the ratchet's population is the three ledgers in scripts/check-type-check-coverage.mjs, and service-datasource is in none of them (enumerated: TEST_DEBT 20 entries, DEBT 13, EXEMPT 1 — absent from all three), so there is no ledger count of this package's for the ratchet to move; the package's own typecheck is green; and the diff adds no test file and changes no executable line. CI runs the gate regardless.


Generated by Claude Code

…in after the driver aligned to spec
The `primaryKeyReader` docblock is the one place this seam's reasoning is
written down, and two of its statements went false when the driver was
aligned to the `packages/spec` introspection contract (`95437e7d2d`,
#10676/#10998) and when the SQLite composite-key truncation was repaired
(#10997, PR #11104).
Comment-only. No executable line changes.
- The producer table said `SqlDriver` spells the per-column signal
`isPrimary`. It emits `primaryKey` and no `isPrimary` at all.
- The `#10997` note said the SQLite composite-key truncation "is upstream of
this seam and is not repaired here". It was repaired;
`introspectPrimaryKeys` now reports every member of a composite key in
declared key order.
- The closing note said reading the extra spellings structurally was needed
"because reconciling `objectql/src/util.ts` with the spec contract is a
spec-owned change". That reconciliation has happened.
- The `refreshCatalog` seam comment repeated the stale claim that "a real
driver spells this `isPrimary` / `primaryKeys`, never `primaryKey`".
- The seam suite's second `describe` said `SqlDriver` derives `isPrimary`
FROM `primaryKeys`; it derives `primaryKey`.
The union read is UNCHANGED — all three arms stay. `table.primaryKeys` is
live and independent: after #10997 it is the only signal here that carries a
composite key in declared key order, which a per-column boolean cannot
express. The `isPrimary` arm is now recorded as a deliberate compatibility
belt rather than a bridge to a live producer, with the measurement that
decided it, so the next reader does not re-derive it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

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 — 0 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 21756b3254ed61a6c38baa46642bd003952194b4packageMentionDocs.

Which tree this was computed on

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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

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

Labels

size/mskip-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-sam@claude