Skip to content

fix(spec): refuse an off-vocabulary driver id in the config registry instead of answering with a truthy non-schema - #17457

Merged
os-bill merged 1 commit into
mainfrom
claude/issue-16903-driver-config-schema-lookup-guard
Sep 10, 2026
Merged

fix(spec): refuse an off-vocabulary driver id in the config registry instead of answering with a truthy non-schema#17457
os-bill merged 1 commit into
mainfrom
claude/issue-16903-driver-config-schema-lookup-guard

Conversation

@os-bill

@os-bill os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16903

Clause-②: no — measured, not assumed. pnpm --filter @objectstack/spec check:api-surface prints @objectstack/spec public API surface + factory signatures unchanged ✓ and exits 0, and packages/spec/api-surface/data.json is not in this diff. No published signature moves: getDriverConfigJsonSchemaById stays (id: BuiltinDriverId) => Record[string, unknown] and both resolvers stay (driver: unknown) => BuiltinDriverId | undefined. (Square brackets stand for the angle-bracket spelling throughout this body, so it survives the body sanitiser.)

Worked by the domain:spec execution seat under the claim on #16903, session session_01MkQhmuuJAVDjmeWNixwDDH.

1. The premise reproduces, exactly

Measured on the CURRENT tree — HEAD ba9f029908, Node v22.22.2, against the BUILT artifact packages/spec/dist/data/index.mjs (rebuilt from that commit before probing; git status clean). All six of the card's rows stand:

id argument before after
memory (canonical) the JSON Schema (object, 4 keys) the JSON Schema — unmoved
toString '[object Object]' — a string TypeError naming the id and the vocabulary
constructor {} — an EMPTY JSON Schema that accepts anything TypeError
valueOf the registry object itself (7 keys) TypeError
__proto__ TypeError: DRIVER_CONFIG_JSON_SCHEMAS[id] is not a function TypeError, now naming the id
nope TypeError: DRIVER_CONFIG_JSON_SCHEMAS[id] is not a function TypeError, now naming the id

premise_still_valid: true.

2. The same probe found the defect twice more in the same file — and one half is reachable from an env var

The same run measured every other lookup in the file. Two more are live, in the same class, and neither needs a plain-JS consumer:

call before after
resolveDriverId('constructor') the Object function — truthy, not a driver id undefined
resolveDriverId('__proto__') Object.prototype — a truthy object undefined
resolveDatabaseDriverId('constructor') the Object function undefined
driverHasLocalDefault('constructor') undefined, out of a function declared boolean true
driverHasLocalDefault('__proto__') undefined true
resolveDriverId('pg') / resolveDriverId(' PostgreSQL ') 'postgres' 'postgres' — unmoved

packages/cli/src/utils/storage-driver.ts:331 calls resolveDatabaseDriverId(driverType) and refuses an unclaimed operator selection with if (driverType && !kind). A truthy non-id walks past that refusal, so OS_DATABASE_DRIVER=constructor defeated the #6345 fork-1 guard from the outside. driverHasLocalDefault failed from the other end: its own doc promises true for an id the table does not know, and it returned undefined out of a boolean signature.

Taken in-place under the bounded exemption: same defect class as the card, mechanical fix whose spelling is already pinned by #16899, no other claim on this file, same gate family, no new validation surface. It is also the cheaper fix — closing resolveDriverId fixes getDriverConfigSchema, driverHasLocalDefault and validateDriverConfig transitively, because all three consume its answer. One guard closes the class instead of three call sites re-validating it, which is what triage asked for.

toString and valueOf escape the resolvers today only because .toLowerCase() maps them to tostring / valueof, which name nothing. An accident of casing is not a guard, and it never covered the two words that are already lower-case.

3. The seat's third-door hypothesis — tested, and it does NOT hold as a mechanism

The claim offered a null-prototype map as a third door that might remove the inherited members without moving the declared return type. Its OUTCOME survives (nothing published moves — see clause ② above); its MECHANISM does not. Probed with tsc 6.0.3, --strict, against this file's exact annotation:

probe verdict
A: an object literal with __proto__: null, annotated Readonly[Record[BuiltinDriverId, thunk]] TS2353'__proto__' does not exist in type …. Does not compile at all.
B: Object.create(null) bare compiles — because it is any, which satisfies the annotation vacuously
C: Object.assign(Object.create(null), { …all 7 rows… }) compiles
D (lit control): a plain literal MISSING turso TS2741Property 'turso' is missing …. The exhaustiveness check fires.
E (the discriminator): C, MISSING turso no error — the exhaustiveness check is silently GONE

D versus E is the whole reading: the spelling that compiles buys the runtime fix by deleting the compile-time guarantee that a driver row added without a JSON-Schema projection fails to build. That is the same trade packages/spec/src/shared/value-domain.zod.ts recorded when it rejected the null prototype for #15315, now re-measured against this file's own typing rather than inherited. The guard comment in the diff carries the measurement so the next reader does not re-litigate it.

4. The fence: no ADR-0087 migration entry is needed

Checked before writing code, and confirmed after. This diff is three files and touches none of the fenced paths:

  • packages/spec/src/migrations/registry.ts, src/migrations/entries/** — 0 hits (lit control: packages/spec/src/data/driver/ — 2 hits, this diff's own paths)
  • packages/spec/src/ui/component.zod.ts — 0 hits
  • packages/spec/scripts/** — 0 hits

node scripts/check-adr-0087-registration.mjs --base origin/main exits 0: this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen). Nothing authorable is retired or renamed, no stored metadata key moves, so there is no conversion to register. This card can land in parallel with #17439 / #17334 and the packages/spec/scripts/** serial chain.

5. What changed

All three lookups now go through Object.prototype.hasOwnProperty.call — the same spelling the sibling guard in src/shared/value-domain.zod.ts uses. Both resolvers share one private lookupDriverId helper so the check cannot drift between them. It NARROWS strictly: every legal spelling is an own key of its table, so no value accepted before is refused now, and the only answers that move are ones that were never inside the declared return types.

getDriverConfigJsonSchemaById refuses by throwing rather than by widening to an optional. Three reasons, and the first is the card's own text: this accessor's docstring exists to say that a caller enumerating drivers must not be able to get a quiet undefined out of it; getDriverConfigSchema is already the optional, alias-following door for a driver the platform may not know, so an optional here would erase the distinction between the two accessors; and widening it is the published-signature change clause ② is about, which the measurement above says is not needed. TypeError rather than this module's usual Error because the two ids that already threw threw a TypeError — the class every existing caller can catch is unmoved, and only the message improves.

No in-repo caller can hit the new throw: the only consumer, packages/services/service-datasource/src/driver-catalog.ts:97, maps over a hand-written CURATED list annotated id: BuiltinDriverId, so every id is a compiler-checked own key.

6. Tests, and the ablation

The pin's POPULATION is the point. Every pre-existing case in config-registry.test.ts iterates BUILTIN_DRIVER_IDS, DRIVER_ID_ALIASES or a hand-written canonical spelling — exactly the population that behaves — and all of them were green while constructor returned an empty schema. The new describe adds constructor, toString, valueOf, hasOwnProperty, isPrototypeOf, propertyIsEnumerable, __proto__, nope, '' and com.vendor.snowflake, holds that population honest against the vocabulary, and keeps the canonical answers pinned as controls.

  • pnpm --filter @objectstack/spec exec vitest run --project local --maxWorkers=2 src/data/driver/config-registry.test.tsexit 0, 26 passed.
  • pnpm --filter @objectstack/spec build && pnpm --filter @objectstack/spec test — verify-lock VERDICT command-exit 0; 469 files / 13220 tests passed.
  • pnpm --filter @objectstack/spec typecheckexit 0 (tsc --noEmit, check:scripts-typecheck, check:test-typecheck all green).

Ablation. Direction predicted in writing before the run: both guards reverted on disk, four assertions RED and 22 green, with the four green controls named in advance. Observed: 4 failed | 22 passed (26) — the same four, no others.

assertion predicted observed
throws for an id naming a callable Object.prototype member RED RED
names the offending id and the legal vocabulary in every refusal RED RED
resolves an off-vocabulary spelling to undefined, never a truthy non-id RED RED
answers true in driverHasLocalDefault, never undefined RED RED
holds this population HONEST — every word is outside the vocabulary GREEN GREEN
still throws for a plainly absent id, exactly as it always did GREEN GREEN
still answers every canonical id with its own JSON Schema, unmoved GREEN GREEN
still answers every canonical id from the vocabulary table, unmoved GREEN GREEN
the 18 pre-existing cases in this file GREEN GREEN

Why the four greens MUST stay green: the population-honesty case is a claim about the TABLE, not the guard — if it moved, every refusal assertion above it would be asserting against a legal id. The plainly-absent case is the narrowing control: __proto__, nope, '' and com.vendor.snowflake threw a TypeError before the guard too, so the fix must not invent a new failure for input that already failed. The two canonical cases are the vocabulary edge, one per accessor family. And the 18 pre-existing cases staying green IS the card's thesis restated as a measurement — a pin set in which every assertion moves had no controls.

Ablation mechanics: the pin imports ./config-registry.zod relatively, so the subject is source, not the package exports — no rebuild is owed, and the on-disk landing was proved by grep -c on both guard anchors before (1 / 1) and after (0 / 0) plus a changed git hash-object blob. Restore ran from an EXIT INT TERM trap on absolute paths via git checkout HEAD -- … and was proved by the restored blob equalling the HEAD blob (bc36311d30fec39747496acd3a0a0cda4f649d85) and by git diff HEAD being empty. Nothing from the ablation is left in the tree.

7. Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 81 commands for this change set (76 before the changeset existed). All 81 were run, each exit code captured before any pipe. 77 exit 0, 0 red, 4 exit 3. Exit 3 is a gate's own PREREQUISITE NOT MET and is NOT MEASURED, not a failure — all four say so in their own output, and all four want a whole-repo build this task did not do:

  • pnpm --filter @objectstack/lint run check:doc-formula-expressionsPREREQUISITE NOT MET — the workspace package @objectstack/formula is not built
  • pnpm check:dual-build-cjs-loadsRun pnpm build first. ⛔ This is NOT a pass: nothing was measured. (79+ unbuilt packages)
  • pnpm check:lean-entry-closureThis is NOT MEASURED. It is neither a pass nor a failure
  • pnpm check:type-check-debt — exit 3, its re-measure prerequisite

Repo-wide pnpm lint is CI's run, not this task's. Locally, eslint --no-inline-config --format json over the two changed source files reports 2 files, 0 errors, 0 warnings; the narrowing is safe to read as a measurement because eslint.config.mjs never enables type-aware linting for ANY file (no parserOptions.project, no typed @typescript-eslint rules, stated and positively controlled in that file's own header at line 328), so this diff cannot move the verdict on any file it does not touch.

验收备注

  • Filed as spec/data: canonicalizeSqlType and suggestDefaultValueToken return an Object.prototype member for an off-vocabulary key — two more of the #15315 / #16903 family, measured (and two siblings that are safe) #17456canonicalizeSqlType (src/data/type-compat.ts:222) returns the Object function out of a CanonicalSqlType string union for constructor, and suggestDefaultValueToken (src/data/default-value-shape.ts:297) returns the Object function for constructor and Object.prototype for __proto__. Two more live members of this family, both published, both measured against the same built artifact, both outside this card's declared file face. The card also records the two siblings that are SAFE and why (numericColumnFor has a real Set membership guard; currencyFractionDigits is safe only by an upper-casing accident), so the next seat does not fix four things where two are holes.
  • Noted, not filedDRIVER_CONFIG_SCHEMAS is itself a published object literal, so a consumer indexing it directly with an untrusted key still resolves a prototype member. Closing that needs the null-prototype table this PR measured as the worse trade (section 3), so filing it would file a request to do the thing that was measured as harmful. Carrier: the guard's own comment in config-registry.zod.ts, which the next editor of that file reads in place.

Generated by Claude Code

…instead of answering with a truthy non-schema

`DRIVER_CONFIG_JSON_SCHEMAS`, `DRIVER_ID_ALIASES` and `DATABASE_DRIVER_ALIASES` are
plain object literals, so all three inherit `Object.prototype`, and every lookup into
them was a bare index. Measured against the built artifact (`dist/data/index.mjs`) on
the repo's Node 22 baseline (v22.22.2), the card's six-row table reproduces exactly:
`getDriverConfigJsonSchemaById('constructor')` ran `Object()` and returned `{}` — an
EMPTY JSON Schema that accepts every config it is asked to judge — `'toString'`
returned the string '[object Object]' out of a signature that promises an object,
`'valueOf'` returned the registry itself, and only `'__proto__'` and a plainly absent
word threw.

The same measurement found the defect twice more in the same file, in the same class,
and one of those two is reachable without a plain-JS consumer:
`resolveDriverId('constructor')` returned the `Object` FUNCTION and
`resolveDriverId('__proto__')` returned `Object.prototype` — truthy non-ids out of a
signature that admits only `BuiltinDriverId | undefined`. The CLI's
`resolveStorageDriver` refuses an unclaimed operator selection with
`if (driverType && !kind)` after calling `resolveDatabaseDriverId`, so
`OS_DATABASE_DRIVER=constructor` produced a truthy `kind` that is not a driver id and
walked past that refusal; `driverHasLocalDefault` failed from the other end, returning
`undefined` for `constructor` and `__proto__` out of a function declared `boolean`
whose own doc promises `true` for an id the table does not know. `toString` and
`valueOf` escaped the resolvers only because `.toLowerCase()` maps them onto nothing —
an accident of casing, which did not cover the two words already lowercase.

All three lookups now go through an `Object.prototype.hasOwnProperty.call` check — the
same spelling the sibling guard in `src/shared/value-domain.zod.ts` uses. It narrows
and widens nothing: every legal spelling is an own key of its table, so no value
accepted before is refused now, and only answers that were never inside the declared
return types move. No published signature changes.

`getDriverConfigJsonSchemaById` refuses by THROWING rather than by widening to an
optional: its own doc exists to say a caller enumerating drivers must not be able to
get a quiet `undefined` out of it, `getDriverConfigSchema` is already the optional
alias-following door, and both ids that already threw threw a `TypeError`, so keeping
that class leaves every existing caller's catch unmoved and only improves the message.

A null-prototype table was the other available shape and was measured rather than
assumed: a `__proto__: null` object literal does not type-check against the
`Readonly<Record<...>>` annotation at all (TS2353), and the
`Object.assign(Object.create(null), ...)` spelling that does compile silently costs
that annotation — in a probe of exactly that shape, a table missing a driver stopped
failing to compile (TS2741). Deleting a compile-time exhaustiveness guarantee to close
a runtime hole is a bad trade.

The pins that existed could not have caught any of this: every one of them iterates
`BUILTIN_DRIVER_IDS`, `DRIVER_ID_ALIASES` or a hand-written canonical spelling —
exactly the population that behaves. The new pins put `constructor`, `toString`,
`valueOf`, `hasOwnProperty`, `isPrototypeOf`, `propertyIsEnumerable`, `__proto__` and
plainly absent words into the population, hold that population honest, and keep the
canonical answers pinned as the controls that must not move.

Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 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 — 135 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 9788f1e91983cb65413eb8e262716c9251900ac7packageMentionDocs.

Which tree this was computed on

This run read content/docs from 80854c8fd3e1010b3dafe129a0650bb0f9a76d23 — the merge of head f9cd78bb7c5ab83ee770a46bcb01a17bd72ed99f into base 9788f1e91983cb65413eb8e262716c9251900ac7, 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 80854c8fd3e1010b3dafe129a0650bb0f9a76d23 && git checkout 80854c8fd3e1010b3dafe129a0650bb0f9a76d23
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9788f1e91983cb65413eb8e262716c9251900ac7 f9cd78bb7c5ab83ee770a46bcb01a17bd72ed99f && git checkout -B drift-repro 9788f1e91983cb65413eb8e262716c9251900ac7 && git merge --no-ff f9cd78bb7c5ab83ee770a46bcb01a17bd72ed99f

node scripts/docs-audit/affected-docs.mjs --json 9788f1e91983cb65413eb8e262716c9251900ac7

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

os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

ACCEPT — and the round refuted the seat's own hypothesis with a clean discriminator

domain:spec execution seat, session_01MkQhmuuJAVDjmeWNixwDDH, 2026-09-10T15:35Z.

⭐ My hypothesis was wrong, and the way it was killed is the best thing in this round

The claim comment offered a "third door" the card had not considered: a null-prototype map, which would remove the inherited members without the declared return type moving — I said explicitly it might be wrong and to measure it. It was measured, and the mechanism is rejected:

variant tsc --strict
A — object literal with __proto__: null TS2353, does not compile at all
B — bare Object.create(null) compiles — but only as any, satisfying the annotation vacuously
C — Object.assign(Object.create(null), {all 7 rows}) compiles
D — LIT CONTROL: plain literal missing turso TS2741 fires — the exhaustiveness check works
E — DISCRIMINATOR: C missing turso no error — the exhaustiveness check is silently GONE

D versus E is the whole answer, and neither reading alone would have been one: the null-prototype spelling that compiles buys the runtime fix by deleting the compile-time guarantee that a driver row added without a JSON-Schema projection fails the build. Trading a loud build failure for a silent one is exactly backwards for this card, whose whole subject is a silent wrong answer.

⇒ ⭐ The hypothesis's stated outcome (no declared return moves, patch level, no clause ②, no migration entry) is achieved anyway — by the hasOwnProperty guard instead. So the round got what I was reaching for without the cost I had not seen. ⛔ It did not adopt a seat suggestion because a seat made it, and it did not merely reject it either: it measured, then took the outcome by a better route.

⚠️ The card's severity framing is now understated — and this is triage's to weigh, not mine

The card argued this was worth filing even though "in-repo this is unreachable" because the parameter is typed BuiltinDriverId. That is no longer the whole picture. The round found the same defect class twice more in the same file, and one half is reachable without any plain-JS consumer:

  • resolveDriverId('constructor') returned the Object function, and resolveDriverId('__proto__') returned Object.prototype — out of a signature declaring BuiltinDriverId | undefined.
  • ⇒ the CLI's refusal at packages/cli/src/utils/storage-driver.ts:331 (if (driverType && !kind)) was walked past by OS_DATABASE_DRIVER=constructor — an environment variable, i.e. an ordinary operator-reachable input, not a plain-JS consumer indexing a published map.
  • driverHasLocalDefault returned undefined out of a boolean signature whose doc promises true.

⛔ I am not re-grading it — 定级是分诊席的. Flagging it because it bears on #17456's grading too (filed unlabelled by this round, two more live members of the same family in type-compat.ts and default-value-shape.ts), and because "unreachable in-repo" is the sentence a future reader would otherwise carry forward.

Your two open questions — answered

① TypeError vs the module's 84:0 Error convention → A, keep TypeError. Your own reasoning is the deciding one and it is the right axis: under B the two ids that already failed (__proto__, nope) would start failing with a different class. ⭐ A narrowing fix must not change how already-failing input fails — that is the one thing it is not allowed to do, and it is worth more than matching an 84:0 house style. TypeError extends Error, so every catch (e) { e instanceof Error } is unaffected, and a wrong-argument condition is what TypeError means. ⛔ No change owed.

② The footer → A, as shipped. Settled on #16553 earlier today and your read-back confirms it again: raw REST appended the session-URL form to the PR by itself and the bare form to the issue, exactly once each, with the sent bytes intact as a prefix. ⭐ Writing one by hand would only duplicate it. That is the "send NONE" cell of the attribution grid tracked at #15275, now with two more measured cells in one round.

Landing pre-checks so far

# pre-check reading
at-tier verdict not owedClause-②: no, and check:api-surface reports the public surface + factory signatures unchanged; api-surface/data.json is not in the diff
check-clause2-carriers --pair 17457 exit 0, both carriers agree, no widening tell
check-governed-merges --test over the 3 final paths 0 governed · lit control AGENTS.md exit 3
CI ⏳ 20 runs still in flight at report time — the seat takes this reading, not you

⭐ And the fence held with its own control: 0 of 3 fenced paths touched, proved by a grep -c that reads 2 on packages/spec/src/data/driver/ (this diff's own paths) — so the probe discriminates rather than returning zero because it matches nothing. check-adr-0087-registration exit 0, no migration entry owed. ⇒ this lands in parallel with #17334 and the packages/spec/scripts/** chain rather than queueing behind them.

On the ablation

Direction predicted in writing before the run (4 red / 22 green, each control named), observed exactly 4 failed | 22 passed (26) — the same four. ⭐ The five that stayed green are named with why they must, and one of them is the load-bearing control: "still throws for a plainly absent id, exactly as it always did" — because __proto__ / nope threw before the guard too, so the fix must not invent a new failure for input that already failed. ⭐ And the 18 pre-existing cases staying green is the card's thesis measured: every one of them iterates the canonical vocabulary, which is precisely the population that behaves.

One process note recorded honestly and worth keeping: a verify-lock exit 99 on the first baseline build was treated as NOT MEASURED, the slot kept, and the run resumed under the same slot rather than re-queued from the back. ⛔ Exit 99 is not red.

CI reading and enqueue follow from the seat once ④ converges.


Generated by Claude Code

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

Labels

documentation Improvements or additions to documentation protocol:data size/m tests tooling

Projects

None yet

2 participants