Skip to content

fix(service-datasource): read the introspected primary key at the isPrimary/primaryKey seam - #11001

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-10676-external-draft-primary-key
Aug 22, 2026
Merged

fix(service-datasource): read the introspected primary key at the isPrimary/primaryKey seam#11001
os-warren merged 1 commit into
mainfrom
claude/issue-10676-external-draft-primary-key

Conversation

@os-warren

@os-warrenos-warren commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Part of #10676. The seam read is repaired and pinned; the object-draft half of that card is
not discharged here, because measurement turned it into a contract question rather than a
missing read. What remains, and why, is in "Not fixed here" below.

The defect

ExternalDatasourceService is typed against the packages/specIntrospectedColumn
(contracts/schema-diff-service.ts), which spells the flag primaryKey. But plugin.ts hands it
the driver's introspectSchema() result unmodified, and SqlDriver implements the other
IntrospectedColumn — the one in packages/objectql/src/util.ts — which spells it isPrimary and
additionally fills table.primaryKeys.

So the service reads a key no in-tree driver ever sets.

Measured on current origin/main (368e7a06f), against a live in-memory SQLite database declared
primary key (id) — the driver's own output, not a description of it:

{ "name": "id", "type": "varchar", "isPrimary": true, ... } // primaryKey: undefined
{ "name": "customers", "primaryKeys": ["id"], ... }

refreshCatalog fed that undefined into ExternalCatalogSchema, whose primaryKey key is
z.boolean().default(false). The default absorbed it silently, so the persisted external_catalog
recorded every column of every remote table as not being part of the remote key — including the
ones that are. Studio's schema browser and the boot gate read that record.

The repair

One module-local helper, primaryKeyReader(table), reading the union of the three signals
(col.primaryKey, col.isPrimary, table.primaryKeys) rather than picking one, wired into
refreshCatalog.

Union rather than a precedence chain, deliberately: no in-tree producer uses a false to negate a
key another signal asserts — the falses are just "not a key", written by producers that fill exactly
one of the three. A precedence chain would drop a real key whenever the winning signal is the one
its producer left blank, which is the defect being repaired. Where the per-column flag and the
table-level list disagree, the union takes both
; under-reporting a federated table's key costs the
caller its addressing key, and no in-tree consumer treats a column's PK-ness as an exclusive claim.

The extra spellings are read structurally off the value. Nothing in packages/spec or
packages/objectql/src/util.ts is touched — reconciling those two declarations is spec-owned and is
not this PR.

Note that no in-tree driver produces a disagreement today: SqlDriver derives isPrimaryfrom
primaryKeys, so the two always agree — including where both are wrong together. See "composite
keys" below.

Clause ②

The response and record shapes do not change. A field that should always have carried the
introspected value starts carrying it: external_catalog columns already declare primaryKey, and
already emit it on every column — only ever as false. This PR makes the trues true.

⚠️ The dispatch asked me to stop if any consumer keys on the absence of primaryKey. On the
catalog path, none does — the key is non-optional in ExternalCatalogSchema and is always present.
On the object-draft path one does, which is exactly why that half is not shipped here; see below.

Composite keys

The union handles a disagreement, but on SQLite the two signals agree and are both short: a
composite key is truncated upstream in the driver, before this seam ever sees it.
introspectPrimaryKeys filters PRAGMA table_info on row.pk === 1, while SQLite numbers
composite members 1, 2, 3…. Measured on t.primary(['order_id','line_no']): PRAGMA reports
line_no with pk: 2, and the driver returns primaryKeys: ["order_id"]. Filed as #10997;
not repaired here (different package, different lane).

Not fixed here — and why it is a decision, not an omission

generateObjectDraft still drops the key. Restoring it there is blocked on a contract question that
this lane cannot answer, so it is escalated rather than guessed:

A field-level primaryKey — i.e. fields.FIELDNAME.primaryKey, the exact key this generator
renders — is not an authorable spec field key. Measured, with a positive control in the same
tsc program:

  • rendered draft → error TS2353: ... 'primaryKey' does not exist in type ... against the
    ServiceObject annotation the generator itself writes; the identical file with the key removed
    produced no diagnostic.
  • ObjectSchema.safeParseunrecognized_keys at ["fields","id"]; identical definition without
    the key → success: true.

That strict schema is a consumer keying on the absence of primaryKey, so shipping the draft half
under the Clause-② reading above would have been wrong. And there is no other authorable home for it:
ObjectExternalBindingSchema is a strictObject with no key for a remote primary key either.

Filed as #11000 with the three candidate repairs (emit it anyway / reuse externalId /
add an authorable spelling) and the measurements behind each. Reported as needs_decision on
#10676. Note #11000 is reachable today without this PR: the opts.primaryKey path already emits
that key and the existing suite pins it.

Adjacent, untouched: #10712 (the same draft also fails os build on the missing namespace
prefix and absent sharingModel) and #10998 (introspectSchema also omits the contract's
dialect and required introspectedAt).

The pin

The card's own diagnosis of why the suite was blind:

external-datasource-service.test.ts hand-writes its fake schema with primaryKey: true (the
spec-contract spelling), so no test ever feeds it a real driver's isPrimary output.

A pin that hand-wrote the fixed spelling would reproduce that blindness exactly. So
external-introspection-seam.test.ts drives the service off a realSqlDriver.introspectSchema()
against a live in-memory SQLite database, and asserts the producer's spelling directly — if the
driver ever starts emitting the spec spelling, that case reddens and the union read gets re-derived
rather than quietly relaxed.

Both directions are pinned: a table with a declared key marks that column and no other; a table
without one still yields a usable catalog entry and promotes nothing, least of all the first
column. Three further cases cover the seam read under a staged disagreement — hand-built on purpose,
and labelled as such in the file, because no live database can currently produce one.

Verification

5cdd4e41f, clean tree.

Ablation — signature predicted in writing before mutating: reduce primaryKeyReader to the
pre-fix read (col.primaryKey === true only) ⇒ exactly 3 failures, all in the new file
(refreshCatalog carries the introspected key…, and both union cases), all expected false to be true; the file's other 3 cases and all 22 other files stay green; totals 522/525.

Observed, on the final bytes: 3 failed | 522 passed (525), 1 failed | 22 passed (23) files,
the three named cases, all AssertionError: expected false to be true. Prediction matched exactly.

Restore proved byte-identical: 5707f16a98f62541e164a1dfe3ef09e28c3936c8 before, mutated
282cb7dfbacf759655198eaf7f6f0fbe22e52b2d, 5707f16a98f62541e164a1dfe3ef09e28c3936c8 after —
restore leg re-run green at 525/525.

No rebuild sits between edit and run, argued from the files: the pin imports the mutated subject
by the relative specifier ../external-datasource-service.js, which vitest resolves to
src/external-datasource-service.ts; packages/services/service-datasource/dist/ does not exist in
this worktree at all; and the package's only vitest alias is anchored on @objectstack/core alone,
which cannot capture the subject. The driver does resolve through its dist/ — built, and
deliberately not mutated, so no leg depends on driver bytes.

checkverdict line
pnpm --filter @objectstack/service-datasource testTest Files 23 passed (23) · Tests 525 passed (525)
pnpm --filter @objectstack/service-datasource typecheckexit 0, no error TS (and --listFiles confirms the new test file is inside that program)
check:changeset-gate-self-tests✓ check-empty-changeset --self-test: 118 assertions …
check:objectui-changeset✓ objectui-range --self-test: all checks passed
check:slot-lookup✓ slot-lookup ratchet holds: 107 unswept site(s) … none new
check:test-source-aliascheck-test-source-alias OK — 72 packages with tests scanned
check:type-source-resolutioncheck-type-source-resolution OK — 76 packages … scanned
check:query-options-erasure✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) … none new
check:type-check-coveragecheck-type-check-coverage: OK — 64/77 workspace packages type-checked
check:engine-double-contractcheck-engine-double-contract: OK — 376 pinned, 133 in the DEBT ledger, 2 exempt
check:where-matcher✓ where-matcher conformance holds: 275 matcher(s) … none new
check:nul-bytescheck-nul-bytes: OK (scanned 6340 text file(s) … no raw ASCII control bytes)
check-adr-0087-registration.mjs✓ … this PR adds no declared-breaking changeset
check-changeset-no-major.mjs✓ This diff introduces no major bump.
check-ci-filter-parity.mjsOK: all 82 declared cross-package glob(s) … are covered
check-empty-changeset.mjs✓ No empty-frontmatter changeset introduced by this diff
check-plugin-teardown-shape.mjs✓ check:plugin-teardown-shape: 57 Plugin implementation(s) …
check-affected-docs.mjsexit 0

Gate union derived on the final commit with a clean tree via node scripts/pm/dispatch-gates.mjs,
no path arguments (exit 0; 11 path-matched families + 5 convention-triggered). Every exit code above
was captured before any pipe.

Declared narrowings, so the gaps are on the record rather than implied:

  1. check:type-check-debt --re-measure was not run — it requires the whole workspace build
    closure, which is the full-farm build this container's shared verify lock is meant to keep out of
    a per-card lane. Partial mitigation, not a substitute: the package's own typecheck is green and
    tsc --listFiles confirms the new test file is inside that program, which is the tsc run that
    gate re-measures for this package. CI runs the gate.
  2. Only the SQLite arm of SqlDriver was executed (better-sqlite3, in-memory). Postgres and
    MySQL were read, not run — no server was reachable. The card claims the defect hits "every SQL
    driver"; that claim is not inherited. What is measured is stronger than a per-dialect sweep for
    this particular seam: the isPrimary assignment is a single dialect-independent site in
    introspectSchema (if (primaryKeys.includes(col.name)) col.isPrimary = true), downstream of the
    per-dialect introspectPrimaryKeys, so the spelling collision cannot vary by dialect. Only the
    composite-key truncation (driver-sql: SQLite introspection reports only the FIRST column of a composite primary key (pk === 1 vs SQLite's 1,2,3… numbering) #10997) is dialect-specific, and only SQLite's arm was measured for it.
  3. The derived change set included packages/create-objectstack/bin/create-objectstack.js, which
    pnpm install mode-flipped (100644 → 100755) in this worktree. Not my edit, deliberately left
    unstaged and out of the commit. It only widened the derived gate set (packages/** families),
    never narrowed it.

#10676 remains open: the object-draft routing decision is still outstanding, so no closing
keyword is used anywhere in this body.

Generated by Claude Code

…rimary/primaryKey seam
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

3 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 7ab286e44aa6626e91e87f9d7d4a7ce881fec34apackageMentionDocs.

Which tree this was computed on

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

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

@os-warren
os-warren marked this pull request as ready for review August 22, 2026 03:51
@os-warren
os-warren enabled auto-merge August 22, 2026 03:51
@os-warren
os-warren added this pull request to the merge queueAug 22, 2026
Merged via the queue into main with commit f76fe42Aug 22, 2026
35 checks passed
@os-warren
os-warren deleted the claude/issue-10676-external-draft-primary-key branch August 22, 2026 04:27
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32551066936 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 5.98s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 79 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

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

Development

Successfully merging this pull request may close these issues.

1 participant

@os-warren