Skip to content

feat(objectql): retain and expose external.credentialsRef on datasource definitions - #12806

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-12758-datasource-credentials-ref
Aug 27, 2026
Merged

feat(objectql): retain and expose external.credentialsRef on datasource definitions#12806
os-zhuang merged 4 commits into
mainfrom
claude/issue-12758-datasource-credentials-ref

Conversation

@claude

@claudeclaudeBot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes#12758

Contract-review tier (Clause-②: yes). All evidence below was produced on this branch's final commit afda2d04d.

The measurement that reframed the card, done before any code was written

The card's headline is that a datasource's credentials reference "is dropped on the way in". It is not, and never was — at runtime. A throwaway harness against packages/objectql/src/index.ts registered a def carrying external.credentialsRef through every entry route and read the private index back:

routestored external.credentialsRef
direct registerDatasourceDef callsys_secret:sec_abc123 (and the stored external is the caller's object by reference)
install path, datasources as an ARRAYsys_secret:sec_abc123
install path, datasources as a NAME-KEYED MAPsys_secret:sec_abc123
control — a name never registeredundefined

The writer stores def.external whole rather than rebuilding it key by key, and the install path spreads the manifest def straight through, so nothing on either path is capable of dropping a nested key. The full reading is on the card.

The defect is type-level, and it has two halves. A caller could not get the key in without defeating excess-property checking — the package's own tsc refused a fresh literal with error TS2353: Object literal may only specify known properties, and 'credentialsRef' does not exist in type '{ allowWrites?: boolean | undefined; }' — and having got it in by an as any, nothing could read it back: enumerating the class's prototype chain found twelve datasource-related members and no accessor onto the index at all, its only reader being the private write gate. So the value sat in the map, reachable by no typed producer and no consumer.

⛔ Consequently no test here is phrased as "the reference is no longer dropped". That would pin something that was never true, and no runtime fix was manufactured to make the card's wording right.

The accept/reject boundary change

Refused before, accepted now — calls to ObjectQL.registerDatasourceDef passing a fresh object literal whose external block carries credentialsRef:

engine.registerDatasourceDef({name: 'warehouse',schemaMode: 'external',external: {allowWrites: true,credentialsRef: 'sys_secret:sec_1'}});// was TS2353engine.registerDatasourceDef({name: 'billing',external: {credentialsRef: 'secret/billing/password'}});// was TS2353

Unchanged in both directions. Every shape that compiled before still compiles ({ name } alone; { name, schemaMode, external: { allowWrites } }), and the widening admits no garbage: a def without name, a non-string credentialsRef, an inline password, and the spec's validation block are each still refused. All eight cases are pinned in one file, positive and negative together.

Newly public:ObjectQL.listDatasourceDefs() and the exported type DatasourceDef. Nothing is removed, narrowed or renamed, so there is no breaking-change declaration and no ADR-0087 entry.

⭐ Worth noting for review: credentialsRef is not invention. @objectstack/spec has declared it on ExternalDatasourceSettingsSchema all along (datasource.zod.ts), valid in every schemaMode per #8153, and /docs/data-modeling/external-datasources shows authors writing exactly that key on a code-declared datasource. The engine's public registration method was refusing a key its own docs prescribe. ⛔ packages/spec is untouched here.

What landed

  • registerDatasourceDef now takes the named, exported DatasourceDef, whose external block carries credentialsRef?: string beside allowWrites. Named rather than restated at all three touch points — three copies of one shape is a second de-facto contract that drifts, and the drift it produces is a handle missing from a credentials sweep.
  • ObjectQL.listDatasourceDefs() answers every definition the engine holds, from both entry routes. Deliberately unfiltered: credentialsRef is valid on a managed datasource too, so filtering by schema mode would hide live handles, and under-reporting is the direction that deletes live credentials. Each entry carries a copiedexternal block so a reader cannot reach through the accessor and mutate the write gate's own input.
  • The write gate is untouched. It reads schemaMode + allowWrites; the new key is inert to it.

The runtime change is Array.from over a Map. That is the honest size of it, and it is the point: the card is a widening plus the accessor that was missing.

Tests, and how each was ablation-proven

Predictions were committed empty before any mutation (2de7e6cc6). Every mutation was proven on disk with anchored greps in both directions (injected marker count = 1, deleted text count = 0) before its run was read; every restore was proven by git hash-object matching the HEAD blob plus an empty git diff HEAD; the script carried trap ... EXIT INT TERM with absolute paths. No rebuild leg was needed and this was verified rather than assumed: the pin and the test both import ./enginerelatively, so neither resolves through the package exports field into dist/.

A1 — the type-level assertion, ablated as compile-red rather than test-red. Deleting credentialsRef?: string; from DatasourceDef:

TSC_EXIT=1
src/datasource-def-credentials-ref.pin.ts(58,36): error TS2353: ... 'credentialsRef' does not exist in type '{ allowWrites?: boolean | undefined; }'.
src/datasource-def-credentials-ref.pin.ts(62,45): error TS2353: ... 'credentialsRef' does not exist in type '{ allowWrites?: boolean | undefined; }'.
src/datasource-def-credentials-ref.pin.ts(80,50): error TS2339: Property 'credentialsRef' does not exist on type '{ allowWrites?: boolean | undefined; }'.
VITEST_EXIT=0 Test Files 2 passed (2) Tests 20 passed (20)

⭐ The second line is the load-bearing half: with the type reverted the runtime suite stays entirely green. A runtime test cannot see a compile-time widening, which is why the pin exists — and why it is a .pin.ts and not a .test.ts. packages/objectql/tsconfig.json excludes **/*.test.ts, so a @ts-expect-error written in a test file there is a phantom check. That this file really is inside the program was measured, not assumed: tsc --listFiles counts 1 for the pin, 0 for the test file, and 1 for register-object-authored-shape.pin.ts, the existing file whose docblock establishes this convention and which serves as the positive control.

A2 — the accessor's retention. Making listDatasourceDefs copy only allowWrites out of the stored block: 5 failed / 15 passed — red on both entry routes, on the managed-datasource case and on the defensive-copy case; green on the two write-gate cases, which do not read the reference.

A3 — the write-gate assertion rides the real Gate 3. Forcing dsAllows = true: 2 failed / 18 passed — red on this file's refusal case and on blocks insert when only the object opts in in the pre-existing external-write-gate.test.ts, so the assertion is on the genuine gate and not a local stub. The refusal case asserts the ADR-0112 envelope (code and status, plus the message clause), never a bare toThrow().

The refusal test also pins the widening's inertness to the gate: the definition carries a credentialsRef in every gate case, and both verdicts are unchanged.

Verification, all on afda2d04d

runresult
pnpm --filter @objectstack/objectql testTest Files 246 passed (246) · Tests 4253 passed (4253)
pnpm --filter @objectstack/objectql typecheckexit 0 (this is what proves ObjectQL implements IObjectQLEngine still holds against the wider parameter)
downstream consumer sweep — pnpm --filter '...@objectstack/objectql' run typecheckexit 0, 0 error TS. Direction stated: the ... PREFIX is consumers, i.e. downstream, which is where a contract change lands. 43 packages in the closure, 37 actually ran a typecheck script (verified by counting the echoed script lines, since a zero-match filter exits 0 silently); the 6 without one are cloud-connection, hono, knowledge-ragflow, service-automation, service-knowledge, service-storage
consumer suites naming the method — rest envelope test, specdata-engine contract test, service-datasource (27 files / 585 tests), clisecret-reference-union (20 tests)all pass
pnpm lint (repo-wide eslint . --no-inline-config)exit 0 — run whole, not narrowed
derived gate family — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, 24 path-matched families plus the convention-triggered onesall exit 0
pnpm check:type-check-debt (the ratchet; full closure built first)OK — 31 ledger entr(ies) re-measured in 219.3s, 1570 raw tsc error(s) total, none above its recorded number

⚠️ One family is NOT MEASURED locally, and is reported as neither pass nor fail: node scripts/pm/check-half-states.mjs exits 3, PREREQUISITE NOT MET — the container's GITHUB_TOKEN is a 14-character proxy placeholder, not a GitHub credential, so nothing was swept and no predicate ran. Its self-test half (pnpm check:pm-half-states, 1515 cases) passes. CI runs it with a real token.

Gate results above are read from each gate's own printed verdict line, with the exit code captured before any pipe.

Contract-docs hand-read (not delegated to the drift bot)

The docs-drift bot is blind here by construction and will very likely post "nothing to list": no page in content/docs/ or docs/ mentions registerDatasourceDef at all (0 hits), so a diff changing the emitter shares no token with the pages that state the rule by its inputs.

Positive control first: grepping allowWrites across content/ and docs/ reaches data-modeling/external-datasources.mdx, data-modeling/drivers.mdx, references/data/datasource.mdx, references/data/object.mdx and ADR-0015 — so the search does reach the pages that describe what a datasource declaration may carry.

Shapes searched, not just the token: what a datasource declaration may carry · ADR-0015 and the external-datasource pages · credential-reference and sys_secret pages · claims of the form "the engine keeps / stores / retains / drops ..." · claims about what the write gate reads.

Found: nothing this change falsifies. The prose runs the other way — external-datasources.mdx §4 already instructs authors to put a reference in external.credentialsRef on a code-declared datasource, and references/data/datasource.mdx documents the key as optional, string, valid in every schema mode. Both are generated from or agree with the spec, which is unchanged. So no doc sentence was rewritten, and none needed to be; the widening closes a gap between the docs and the implementation rather than opening one.

Reported rather than edited: neither registerDatasourceDef nor listDatasourceDefs is documented anywhere. Judged not a finding — the docs describe authoring datasources through defineDatasource, and these are engine-side registration APIs consumed by service-datasource, not authoring surface.

Changeset: minor, argued rather than defaulted

The tension is real. Zero runtime behaviour changes, which is the honest case for patch. But the bump describes the contract, not the bytes executed, and this adds public API three ways: a new public method, a newly exported type, and a widened accepted set on an existing public method. A consumer pinning ~ would receive new API under a patch, which misdescribes the release. Nothing is removed, narrowed or renamed, so this is the additive-surface minor, not the launch-window convention for shipping breaking changes as minor.

Scope

The consumer half was NOT wired, and that is a decision with reasons — filed as #12804, blocked on this card. secret-reference-union.ts cannot reach the new accessor (SecretReferenceEngineLike names only two members) and declaredDatasources: undefined is the module's deliberate loud "nobody answered". Making the engine answer instead is a contract decision on a shipped, exported CLI input — the honest shape is plausibly the union of both sources rather than a replacement, since a host can declare datasources the engine never saw. That is not small and local, so per this card's dispatch it becomes a follow-up. ⚠️ Three prose sites in that module now state the old fact and were deliberately left alone: correcting the wording without the behaviour change would leave the module internally inconsistent, and one of the three is the operator-facing gap message. #12804 names all three with line context.

Also filed: #12805IDataEngine.registerDatasourceDef in packages/spec still declares the narrow parameter and has no listDatasourceDefs, so a host typed against the published contract still cannot pass the reference. Fenced out of this card by dispatch; unassigned, for the spec seat.

Both follow-ups are open, unassigned and pm:queue. Neither is closed by this PR.


Generated by Claude Code

…ce definitions
`registerDatasourceDef`'s inline parameter type carried only `name`,
`schemaMode` and `external.allowWrites`, so a caller passing a fresh object
literal with `external.credentialsRef` was refused by excess-property
checking (TS2353) — and the engine exposed no reader onto its datasource
index at all, its only consumer being the private write gate.
Measured before changing anything: nothing stripped the reference at
runtime. The writer stores the caller's `external` object whole, by
reference, and the manifest install path spreads the def straight through,
so the value was already in the index — unreachable to every typed producer
and to every consumer. The defect was type-level, and the fix is a widening
plus the accessor that was missing.
- name the shape as `DatasourceDef` rather than restating it at all three
touch points, and widen it with `external.credentialsRef?: string` — the
key `@objectstack/spec` already declares (`ExternalDatasourceSettingsSchema`),
valid in every `schemaMode` per #8153, so retention rather than invention;
- add `ObjectQL.listDatasourceDefs()`, deliberately unfiltered and returning
copied `external` blocks, so a `sys_secret` reference sweep can see the
handles a datasource declared IN CODE holds — those never reach
`sys_metadata`, so today the host has to remember to pass them in;
- pin the compile-time half in a `.pin.ts`, since the package's tsconfig
excludes `**/*.test.ts` and a `@ts-expect-error` in a test file there would
be a phantom check.
The write gate is untouched: it reads `schemaMode` + `allowWrites` and the
new key is inert to it, which the runtime tests pin in both directions.
Part of #12758
A1 — type-level pin. Delete `credentialsRef?: string;` from `DatasourceDef`.
PREDICTION: `pnpm --filter @objectstack/objectql typecheck` goes RED with
TS2353 on the POSITIVE lines of `datasource-def-credentials-ref.pin.ts`, AND
the vitest run stays GREEN. The second half is the point: a runtime test
cannot see a compile-time widening, which is why the pin exists at all.
A2 — the accessor's retention. Make `listDatasourceDefs` copy only
`allowWrites` out of the stored `external` block.
PREDICTION: vitest goes RED on the read-back cases (both entry routes, the
managed-datasource case, the defensive-copy case) and STAYS GREEN on the two
write-gate cases, which do not read the reference.
A3 — the write gate is really the write gate. Force `dsAllows = true`.
PREDICTION: vitest goes RED on "still refuses a write without the double
opt-in" here AND in the pre-existing `external-write-gate.test.ts`, proving
this file's gate assertion rides the real Gate 3 and not a local stub.
Restore leg for each: `git checkout HEAD -- <absolute path>`, proven by a
`git hash-object` match against the HEAD blob plus an empty `git diff`.
No rebuild is needed on any leg: the pin and the test both import `./engine`
RELATIVELY, so neither resolves through the package `exports` field into
`dist/` — the mutation on disk is the code under test.
Part of #12758
…ning
Argues the bump rather than defaulting it: zero runtime change (the case for
patch) against three additions to public API (the case for minor).
Part of #12758
Brings the branch onto the current type-check-debt ledger sweep (#12798) and
the dispatch-gates hintCovers fix (#12794), so the gate-family derivation below
reads a tree someone is actually on rather than a 5-commit-stale one.
Part of #12758
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/objectql, touching 11 documentable anchor(s). ⚠️1 changed file(s) yielded no anchor (packages/objectql/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/data-modeling/drivers.mdx(via schemaMode (symbol))
  • content/docs/data-modeling/external-datasources.mdx(via schemaMode (symbol), credentialsRef (literal))

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

  • content/docs/releases/v17.mdx(via schemaMode (symbol), credentialsRef (literal))

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
  • 1 changed file(s) yielded no anchor (packages/objectql/src/index.ts) — pages documenting those are invisible to this run
  • 1 anchor(s) matched too much of the corpus to be a work list: ObjectQL (symbol, 64 pages)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 15 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 96dc446c9c19063edfae26ae30ff75143ef0c5b7packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 96dc446c9c19063edfae26ae30ff75143ef0c5b7 → 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 27, 2026
@os-zhuangClaude

Copy link
Copy Markdown
Contributor

Reviewer-of-record audit of the drift check's rows above, including the release-owned one — which the check says is read-only but still audited, so here is the audit.

The guardrail holds. The diff touches five files, none under content/docs/ at all: .changeset/datasource-def-credentials-ref-retained.md, and four under packages/objectql/src/. No release page was edited.

content/docs/releases/v17.mdx — audited, NOT falsified. It was listed on a credentialsRef literal anchor, which is the check working as designed: naming the token is not the same as being made wrong by the diff. All three sites survive this change, for different reasons:

  • :3014 — "DatasourceSchema now accepts external.credentialsRef — and only it — on schemaMode: 'managed' (feat(spec): allow external.credentialsRef (and only it) on schemaMode 'managed' #8588)". That is the spec schema, which this PR leaves untouched.
  • :3787 — a bound credentialsRef reaching the mongo/mysql/postgres clients. Driver-side, unchanged.
  • :589 — a top-level password being pointed at external.credentialsRef. Prescriptive guidance, unchanged.

⇒ nothing to file and nothing to fix on that page.

One adjacent line worth naming rather than passing over, because it points at this card's own follow-up. :3477 records that migrateCredential refuses "a code-defined datasource", among other cases. This PR does not change that refusal and does not make the sentence false — but it is the documented shape of the same gap #12758 exists to close, and it is independent evidence that the direction is right rather than invented. It belongs to the consumer half, filed as #12804.

The two hand-written rows were already covered.external-datasources.mdx and drivers.mdx both appear in this PR's own hand-read (they are named in its positive control), and its conclusion — that the prose already instructs authors to write external.credentialsRef on a code-declared datasource, so the widening closes a docs↔implementation gap rather than opening one — holds against those rows. Bot and hand-read converged, from opposite directions.

⚠️ Residual, stated because the check states it: packages/objectql/src/index.ts yielded no anchor, so pages documenting it are not covered by this run. The hand-read's shape-based sweep is what covers that gap, not this list.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 27, 2026 21:55
@os-zhuang
os-zhuang enabled auto-merge August 27, 2026 21:55
@os-zhuang
os-zhuang added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit 0db5520Aug 27, 2026
34 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-12758-datasource-credentials-ref branch August 27, 2026 22:11
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-zhuang@claude