Repository navigation
service-datasource: the primaryKeyReader docblock now describes a producer that no longer exists, and the union read's isPrimary arm has no in-tree producer left #11123
Description
Activity
Blocked-by: #10676
PM note from the
domain:engineseat (session_01RfyXxZ2WPjcjhuXpiQQc3y), verifying thisout_of_scope_findingsentry and porting two things from #11125, which I filed in parallel by mistake and have now closed as a duplicate of this card.① The
Blocked-by:line above is the point of this comment. Everything described here becomes true only once the driver actually lands — PR #11124 is in review, not merged. That line is machine-readable in a comment exactly as in a body, so the unlock scan fires it when #10676 closes, i.e. the moment the driver change merges. Without it this card is a true observation about a future that has not happened yet, and a taker picking it up today would find theisPrimaryarm still has a producer.② ⛔ Do not collapse
table.primaryKeysalong with the dead arm. The union is three signals and only one of them is dying.table.primaryKeysremains live and independent — the driver still emits it, and after #10997 (PR #11104, merged) it is the carrier of composite keys in declared key order, which the per-column boolean cannot express. Collapsing the union tocol.primaryKeyalone would silently drop composite-key handling at this seam. Whatever this lane decides about theisPrimaryarm, that is a separate signal with a separate reason to exist.Also confirming from the engine side, since this card records it: the engine PR touched only the producer-spelling case in
external-introspection-seam.test.tsplus that file's docblock tense, and added one new file.primaryKeyReader, its docblock, its union read, the two staged-disagreement cases and the seconddescribeblock were left untouched — deliberately, and it was an explicit boundary in the dispatch rather than an oversight.
Generated by Claude Code
Triage:
pm:queue,domain:services, Task.primaryKeyReaderdocblock describes a producer that no longer exists and the union'sisPrimaryarm has no in-tree producer left — establish-first: re-verify both claims on the merged ref before editing; if the arm truly has no producer, the removal follows the dead-code discipline (reference-surface check at review).
Generated by Claude Code
{ "issue": 11123, "status": "needs_decision", "branch": "claude/issue-11123-primary-key-reader-truth", "pr": "https://github.com/objectstack-ai/objectstack/pull/11206", "premise_still_valid": true, "clause_2_redeclared": "yes — removing the `isPrimary` arm WOULD change accept/reject behaviour for a reachable out-of-tree producer. Per the claim comment's own instruction I re-declare Clause-② as `yes` and STOPPED before shipping the removal. The PR ships clause ① only; the union read is byte-identical, all three arms intact.", "summary": "Landed clause ① only, as draft PR #11206: a comment-only truth repair of the `primaryKeyReader` docblock in external-datasource-service.ts. Both card staleness claims re-verified on the merged ref before editing and both hold — the producer table's `isPrimary` row (driver now emits `primaryKey`, sql-driver.ts:10151, commit 95437e7d2d) and the `#10997` note (repaired in PR #11104; introspectPrimaryKeys now filters `row.pk > 0` and sorts on the ordinal). Found and repaired three further stale statements the card did not name, all the same defect class: the docblock's closing note claiming the objectql/spec reconciliation was still pending (it landed — util.ts now extends the spec type), the refreshCatalog seam comment at :511 repeating 'a real driver spells this isPrimary / primaryKeys, never primaryKey', and the seam suite's second describe saying SqlDriver derives `isPrimary` from `primaryKeys`. Clause ② honoured: table.primaryKeys untouched. Clause ③ answered and escalated rather than acted on — see open_questions.", "tests": "All on the final commit 57e6a7ccbf. Exit codes captured by redirecting to a file BEFORE any pipe; verdicts quoted are the gates' own printed lines, never a bare $?. Gate union derived on that commit with NO hand-supplied paths: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` — repo assertion held ('checked against this checkout's origin remote — it holds'), 2 paths vs merge base 52a41b72e, yielding 7 path-matched + 6 convention-triggered (test-file kind) families. BUILD: `pnpm --workspace-concurrency=2 --filter '@objectstack/service-datasource^...' build` (upstream-dependency direction, built before judging anything) -> os-verify-lock 'VERDICT command-exit 0'. TYPECHECK: `pnpm --filter @objectstack/service-datasource typecheck` -> 'VERDICT command-exit 0', and the run echoed '> @objectstack/service-datasource@17.1.0 typecheck', so it was not a zero-match no-op. TEST: `pnpm --filter @objectstack/service-datasource test -- --maxWorkers=2` -> 'Test Files 26 passed (26)' / 'Tests 563 passed (563)', 'VERDICT command-exit 0'; 26 = the 26 *.test.ts on disk, so the edited seam file ran. GATES, all exit 0 with their own verdict lines: check:published-files ('69 publishable package(s) ... declare a `files` whitelist'), check:slot-lookup ('ratchet holds ... none new'), check:test-source-alias ('OK — 72 packages with tests scanned'), check:type-source-resolution ('OK — 77 packages with a tsconfig.json scanned'), check-ci-filter-parity.mjs ('OK: all 86 declared cross-package glob(s)'), check-plugin-teardown-shape.mjs ('63 Plugin implementation(s) across 4491 source(s)'), docs-audit/check-affected-docs.mjs (exit 0), check:query-options-erasure ('ratchet holds ... none new'), check:type-check-coverage (exit 0), check:engine-double-contract ('OK — 384 pinned'), check:where-matcher ('conformance holds ... none new'), check:cross-package-test-inputs ('OK: 13 package(s) read outside themselves, all declared'). Full workspace closure built for the ratchet prerequisite: `turbo run build --filter='./packages/*' --filter='./packages/*/*' --concurrency=2` -> 'Tasks: 70 successful, 70 total', 'VERDICT command-exit 0'. NO ABLATION was run — this diff changes zero executable lines, so there is nothing whose removal could redden a test; saying otherwise would be a fabricated leg. DIFF PROVENANCE: the comment-only claim is measured, not asserted — `git diff -U0` filtered to added/removed lines that are not comments or blanks returns EMPTY. Control-byte self-scan on both edited files: `grep -naP '[\\x00-\\x08\\x0b\\x0c\\x0e-\\x1f\\x7f]'` -> no hits.", "declared_narrowing": "ONE, declared rather than silent. `pnpm check:type-check-debt` (the --re-measure ratchet half) is NOT MEASURED locally and is NOT reported as a verdict. Its prerequisite closure WAS built (70/70 above), so this is not a PREREQUISITE-NOT-MET; the first attempt was killed by the container's 10-minute foreground cap (exit 143) and on retry the shared verify lock was held by a parallel agent running that identical gate (os-verify-lock --status: 'holder pid 15018 ... pnpm -s check:type-check-debt --re-measure', still held at 200s), so queueing behind it could not fit a ~10-minute run inside one foreground call. Substitute measurements, all real: (a) its structural sibling check:type-check-coverage is green; (b) the ratchet re-runs tsc PER LEDGER ENTRY, and I enumerated the three ledgers in scripts/check-type-check-coverage.mjs — TEST_DEBT 20 entries, DEBT 13, EXEMPT 1 — and @objectstack/service-datasource is absent from ALL THREE (counts corroborated by the gate's own printed summary), so this package owns no count for the ratchet to move; (c) the package's own typecheck is green; (d) the diff adds no test file and changes no executable line, and touches exactly one package. CI runs the gate regardless.", "open_questions": [ { "question": "Clause ③: is an out-of-tree driver a supported producer of the `isPrimary` shape — i.e. is retiring the union's `isPrimary` arm a dead-code deletion, or a narrowing of accepted input? I established this rather than assuming it, and it comes out as NARROWING. Evidence: (1) the card's in-tree claim is TRUE — a whole-identifier sweep for isPrimary (not the substring; objectql's `isPrimaryKeyField` is unrelated and was excluded) leaves only prose hits, no producer. (2) But the producer population at this seam is OPEN BY DESIGN: 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>`. (3) The retirement shipped as a BREAKING change whose ONLY stated migration channel is the compiler — .changeset/driver-emits-spec-introspection-shape.md: 'TypeScript consumers of the removed member are told by the compiler, precisely and at every site', and its ADR-0087 note repeats 'the channel that reaches every affected consumer is the compiler'. Against a `Promise<unknown>` handle that channel provably never fires, so a host-built driver still emitting the old spelling is reached by NOTHING and would silently lose its remote key. (4) Independently sufficient: the belt's clock has not started. Both the union (#11001, .changeset/external-catalog-introspected-primary-key.md) and the retirement (#11124, .changeset/driver-emits-spec-introspection-shape.md) are STILL UNCONSUMED CHANGESETS at version 17.1.0 — no released version has ever emitted `primaryKey` from this driver, and content/docs/releases/ mentions neither. Removing the belt now would ship the narrowing in the very same release as the break it exists to absorb. (5) No published doc ever told an external author to emit isPrimary: grep over skills/, content/ and docs/ returns zero hits.", "options": [ "A — Keep the arm now (what the PR does); retire it in a later release once the spec-spelling retirement has actually SHIPPED and the belt has had its one-release clock. The docblock now records the belt, its expiry condition and the measurement, so the follow-up is a lookup rather than a re-derivation.", "B — Retire the arm in this PR as dead code. Rejected: it narrows accepted input for a population the retirement's compiler channel cannot reach, and does so before the break it absorbs has shipped.", "C — Retire the arm AND close the gap properly: tighten DatasourceDriverHandle.introspectSchema from `Promise<unknown>` to the spec `Promise<IntrospectedSchema>`, so the compiler channel finally reaches host-built drivers, then drop the belt. Contract-first, but it is a service-datasource contract change well outside this card and needs its own card and gate." ], "recommendation": "A now, C as the follow-up card — and B never on its own. Four axes. REAL BUSINESS NEED: the belt protects a population that is documented and reachable (host-built drivers are the framework's stated extension model) but that I could not show any CURRENT occupant of — so the honest reading is a real-but-unmeasured need, which argues for keeping a zero-cost boolean arm rather than for building anything new; C adds no capability, it only routes an existing contract through the compiler. LONG-TERM SOUNDNESS: C is the contract-first answer and A is its prerequisite — the defect is that a published seam is typed `unknown`, and deleting the belt while leaving `unknown` in place removes the symptom's cushion without touching the cause, which is the workaround shape. AI-WRITTEN-CODE SAFETY: this is the decisive axis. `Promise<unknown>` is exactly the tolerant consumer boundary where an AI-authored driver's wrong spelling is accepted silently and its remote primary key vanishes with no diagnostic; C converts that into a loud compile-time rejection (declared = enforced), while B makes the same mistake silent AND lossy. STARTUP SCOPE DISCIPLINE: A is free — it is the status quo plus a docblock that now explains itself; C is small and bounded (one type on one interface) and should be judged on its own card, not smuggled in here. Note the tension I am not hiding: startup discipline does argue against carrying compatibility for a population with no demonstrated occupant, and if the maintainer weighs that above the safety axis then B becomes defensible — but only AFTER the retirement has actually shipped, never in the same release." }, { "question": "Follow-on, contingent on the above: if the arm is ever retired, the two staged-disagreement cases in external-introspection-seam.test.ts go with it. They are hand-built to stage a disagreement no live in-tree database can produce, and feeding the retired spelling is now the ONLY exercise the isPrimary arm has anywhere in the tree. Should that removal be part of the same card as the arm's retirement?", "options": [ "A — Same card: the cases exist only to pin the arm, so they die with it.", "B — Separate card: keeps the arm's removal diff minimal." ], "recommendation": "A. The test file's docblock now states this explicitly so the coupling cannot be missed; splitting it would leave two cases pinning a code path that no longer exists, which reds nothing and reads as coverage." } ], "files_changed": [ "packages/services/service-datasource/src/external-datasource-service.ts", "packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts" ], "deviations": [ "No changeset; applied the `skip-changeset` label instead (additive POST to /issues/11206/labels, HTTP 200, then READ BACK — labels are ['size/m','tests','skip-changeset'], so it survived the size-labeler's write). The claim comment listed a changeset as in-surface, but nothing published changes: the diff is comment-only, `primaryKeyReader` is module-private and never reaches a .d.ts, and test files sit outside the package's `files` whitelist (check:published-files green). dispatch-gates itself printed 'write one unless this card is docs-only'. This repo rejects the empty-frontmatter form (check-empty-changeset.mjs), so the label is the mechanism, not an omission.", "PR body opens with `Part of #11123`, NOT `Fixes`. Deliberate: clause ③ goes to the decision box, and the dev contract forbids a closing keyword on a card still carrying an open decision — a `Fixes` merge would silently close #11123 and drop the escalation, which inbox filters that read only open issues would never surface. Verified no stray closing keyword anywhere in the body (regex over fix/fixes/fixed/close/closes/closed/resolve/resolves/resolved + #n returns zero hits, negation-proof since GitHub ignores negations).", "Repaired three stale statements the card did not name (the docblock's closing objectql/spec-reconciliation note, the refreshCatalog seam comment at :511, and the seam suite's second describe docblock) under the bounded in-place exemption: same defect class as clause ①, mechanical with the correct form pinned by 95437e7d2d, inside the already-claimed file surface with no other holder, same gate family, no new verification surface. Each is named with its evidence in the PR body, which is the condition for taking the exemption rather than filing them.", "PR body was read back from the API and is byte-identical to the local file; the `<...>`-shaped fragments (`Promise<unknown>`, `Omit<SpecIntrospectedColumn, ...>`) survived the sanitizer, as did the session-URL footer." ], "out_of_scope_findings": [] }
Generated by Claude Code
Maintainer ruling recorded (atomic four-part write)
Ruled 2026-08-23, live PM chat, verbatim: 「10950 不考虑存量，其他接受你的建议」 — this card falls under the accepted recommendations. Ruled: A now + C as an independent follow-up — the landed state (compatibility shim + documented expiry, PR #11001) stands; the contract tightening is filed as #11381 (spec lane, Clause-② yes, release-coordinated); shim removal (B) only after the retirement is published. Nothing further remains on this card — closing completed.
Generated by Claude Code
- added a commit that references this issue
on Aug 23, 2026
Recorded by the engine lane while landing #10676 / #10998, and deliberately not repaired there: the file is the services lane's, and the three-signal union read that PR #11001 shipped is on that lane's own one-release clock. The engine change makes two statements in it false; carving the read is the services seat's decision, not the driver change's.
What moved
SqlDriver.introspectSchema()now emits thepackages/specspelling (primaryKeyon every column, plusdialectandintrospectedAt) and no longer emitsisPrimaryat all. Maintainer ruling, 2026-08-22 (live session, 「同意所有」 item 9 = 驱动侧对齐 spec 契约).What is now stale in
external-datasource-service.tsThe
primaryKeyReaderdocblock, which is the one place this seam's reasoning is written down:Its producer table is wrong. It reads:
SqlDriver(+SqliteWasmDriver, which extends it)isPrimaryprimaryKeysSqlDrivernow spells the per-column signalprimaryKey, so the table records the collision the union read exists to bridge as if it were still live.Its
#10997note is out of date. It says the SQLite composite-key truncation "is upstream of this seam and is not repaired here". driver-sql: SQLite introspection reports only the FIRST column of a composite primary key (pk === 1vs SQLite's 1,2,3… numbering) #10997 merged (PR fix(driver-sql): report every member of a SQLite composite primary key, in key order #11104):introspectPrimaryKeysnow reports every member of a composite key in declared key order. The engine seat flagged this exact sentence as going stale when that PR merged.The
isPrimaryarm of the union now has no in-tree producer. The read still takescol.primaryKey || col.isPrimary || table.primaryKeys.includes(col.name). Nothing in the tree writesisPrimaryany more except two deliberately hand-built fixtures inexternal-introspection-seam.test.tsthat stage a disagreement no live database can produce. That is the belt's clock coming due — collapsing it to the spec spelling is a decision for this lane.What the engine PR did and did not touch in this package
Touched, minimally, and only because leaving it red blocks the merge (authorised at dispatch):
src/__tests__/external-introspection-seam.test.ts— the producer-spelling case, which was written to redden on exactly this change. Its title, its two producer assertions (nowprimaryKey === true/isPrimary === undefined), and the docblock's tense. The staged-disagreement cases and the file's seconddescribeare untouched.Added (new file, no existing behaviour touched):
src/__tests__/external-object-draft-real-introspection.test.ts— the pin the ruling requires:generateObjectDraftdriven off a realSqlDriver.introspectSchema()result.⛔ Not touched:
primaryKeyReaderitself, its docblock, and every consumer of it.Filed unassigned as an observation. No behaviour is broken by any of the above — the union read keeps working; what is wrong is what a reader is told.