Skip to content

fix(analytics)!: AnalyticsServiceConfig.sqlDialect declares its three-name accept set, and an out-of-contract answer is diagnosed once (#16206) - #17496

Merged
os-sales merged 2 commits into
mainfrom
claude/issue-16206-sqldialect-vocab
Sep 10, 2026
Merged

os-sales merged 2 commits into
mainfrom
claude/issue-16206-sqldialect-vocab

Conversation

@claude

@claude claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #16206

AnalyticsServiceConfig.sqlDialect — a published host hook — was typed as free string while only three spellings ever did anything. A host that owned a SQLite datasource and answered the spelling its own stack uses (knex's canonical sqlite3, which driver-sql itself lists in SQLITE_EMIT_CLIENTS alongside better-sqlite3) was read as unknown, and nothing told it. Because sqlDialectFor is tiered "cannot answer, do not block", a wrong answer and no answer were the same answer — the host that tried hardest to help got the residue arm, silently.

Ruled option A (decision batch #88): declare the vocabulary, diagnose a non-empty answer outside it, keep undefined silent and legal, and do not widen the set. That is what this does, and no more.

Re-derived locations — the card's line numbers had rotted twice

Located by TEXT on origin/main @ e758131b3, because the card said analytics-service.ts:606, triage measured :621, and the answer today is a third number:

what card triage measured here
sqlDialect?: (object: string) => string | undefined analytics-service.ts:606 :621 analytics-service.ts:707
const KNOWN_DIALECTS = new Set(...) text-match-sql.ts:196 :196 :196
normalizeSqlDialect :205-:206 :205-:206 :205-:206
sqlDialectFor :218 :218
the host-hook wiring site analytics-service.ts:906
SqlDialectName (read-only, domain:engine) driver-sql/src/schema-drift.ts:123 same
SQLITE_EMIT_CLIENTS (read-only) driver-sql/src/sql-driver.ts:4846sqlite3, sqlite, better-sqlite3

packages/spec is untouched, and the premise that it need not move held: both type names live in this lane's own packages.

What changed

  • text-match-sql.ts — one const tuple ACCEPTED_SQL_DIALECTS is now the single source for both the exported type AcceptedSqlDialect and the runtime KNOWN_DIALECTS membership set, so a future widening cannot land in one and miss the other. unknown is deliberately kept out of it: it is the residue arm, never something a host says. Two predicates join it — isUnrecognisedSqlDialectAnswer (non-empty and outside the set) and asAcceptedSqlDialect (the narrowing the bridge needs).
  • analytics-service.ts — the config hook now declares AcceptedSqlDialect | undefined, with the accept set and both behaviours stated in the docblock. The wiring site wraps the host's answer and emits one warn naming the object, the answer and the accepted set. The answer is passed through untouched: the diagnostic informs, it does not correct and it does not accept a wider vocabulary.
  • normalizeSqlDialect is unchanged — the accept set was NOT widened. That was option B, refused by name.

The level is warn, not error, on AGENTS.md's one question: this is functional degradation (a construct is not enabled and the next person to look finds a slower predicate), not durability degradation — nothing that claims to be persisted fails to land.

"Diagnosed once" — keyed on the ANSWER

The key is the out-of-contract spelling, held in a Set on the service instance. That is the ruling's own granularity ("a non-empty answer outside them is diagnosed once") and it is the failure's identity: one misspelling reaching a thousand objects is one defect with one fix. The object is named in the line so the host has a concrete place to look; it is not part of the key.

Why it is bounded, in the terms #15166 settled: the key set's cardinality is the number of DISTINCT strings the host's own hook can return — a property of the host's code, fixed before the first query runs — and nothing in it grows with traffic. Pinned by measurement, not by argument: 50 laps over two objects and 200 laps over the same two both emit exactly 1 line. A second, different wrong answer is a second failure and gets its own line. The set is per service instance, not module-global, so a second host's identical misconfiguration is not swallowed by the first's.

The shipped bridge cannot reach this path at all: it answers from SqlDriver.dialectName, whose declared return IS the accept set plus unknown.

Reproduction — RED before, GREEN after

The pins were run against the committed tree with the diagnostic call deleted (a delete-shape ablation), and the mutation was proved on disk before the run was allowed to mean anything:

ON-DISK PROOF: marker occurrences 1 -> 0
ON-DISK PROOF: HEAD blob=4f3350d9c278a6f503050de36d31d13f3ccd6861 mutated blob=5f5480e0d2f5b8f0b6780f0f97f49b5f3e624cf2
MUTATED-RUN command-exit=1
     x a host answering knex's `sqlite3` is read as `unknown` AND is told so, once
     x one misspelling reaching many objects and many queries is ONE line
     x a SECOND, DIFFERENT wrong answer is a second failure and gets its own line
     x the line count does not move with TRAFFIC - 4x the queries, the same one line
     x each service instance carries its OWN key set - no module-global residue
      Tests  5 failed | 7 passed (12)

The seven that stayed green under the mutation are the vocabulary pins and the undefined-stays-silent control — correctly, since they assert ABSENCE. Restoration was proven BY STATE, never by an exit code: git hash-object back to 4f3350d9c278a6f503050de36d31d13f3ccd6861 (the HEAD blob), git diff HEAD empty and git status --porcelain empty. Since the predicted direction was RED and the run went RED, the mutation demonstrably reached the code under test; the subject resolves through a relative source import, so no dist/ sits on the resolution path.

The measurement the ruling made a precondition of landing

Driven once, on sql.js, through a host answering sqlite3 — with a host answering sqlite as the discriminating control. The two differ in one character of one string and in nothing else: same cubes, same capabilities, same engine, same shared FILTER_TEXT_ROWS fixture.

=== CASE-EXACT FAMILY - host answers "sqlite3" (unknown arm) vs control "sqlite" ===
  filter                            | contract                        | sqlite3-host              | sqlite-host (control)           | verdict
  {"name":{"$contains":"a_b"}}      | ["7"]                           | ["7"]                     | ["7"]                           | ok
  {"name":{"$contains":"acme"}}     | ["2"]                           | ["1","2"]                 | ["2"]                           | WRONG
  {"name":{"$contains":"ACME"}}     | ["1"]                           | ["1","2"]                 | ["1"]                           | WRONG
  {"name":{"$startsWith":"ACME"}}   | ["1"]                           | ["1","2"]                 | ["1"]                           | WRONG
  {"name":{"$endsWith":"corp"}}     | ["2"]                           | ["1","2"]                 | ["2"]                           | WRONG
  {"name":{"$notContains":"acme"}}  | ["1","3","4","5","6","7","8","9"] | ["3","4","5","6","7","8","9"] | ["1","3","4","5","6","7","8","9"] | WRONG

  case-exact cases driven: 6 - WRONG on the sqlite3 host: 5

Rows 1 and 2 of the fixture are ACME Corp and acme corp. The mechanism, printed from the same run:

  sqlite3-host SQL: ... WHERE name LIKE $1 ESCAPE $2 ...  params ["%acme%","\\"]
  sqlite -host SQL: ... WHERE name GLOB $1 ...            params ["*acme*"]

Five of the six case-exact cases return the wrong rows — every case that discriminates on ASCII case; the sixth carries no cased letter to fold. Note the direction of the negated case: $notContains DROPS row 1 from a set that should contain it, so on the read scope this is not only over-reach in one direction.

This is not "slower", it is "returns the wrong rows" for the population this card is about. It is #15684's fold, live on the arm a sqlite3-answering host lands on. Per the ruling, the grade is the delivering seat's to revisit before this lands, and this PR reports the finding rather than fixing it — closing #15684's unknown arm is that card's business, and doing it quietly inside this one was refused.

One deviation from the dispatched file surface

src/plugin.ts (same package, outside the declared surface) — forced by the declaration, not chosen. SqlDriver.dialectName is a FOUR-name vocabulary whose fourth name is unknown, that driver's own "I cannot say" (what it returns for a client it does not model, mariadb among them per #11756). Passed through verbatim it would arrive at the newly-narrowed hook looking like a considered answer outside the accept set, and every such deployment would carry a warning about a driver behaving correctly. The bridge now translates that residue to undefined, this hook's own spelling for the same thing; the dialect the compilers end up with is unchanged either way. Without it, pnpm --filter @objectstack/service-analytics typecheck is RED (TS2322 at plugin.ts:1060).

driver-sql was read and cited, never edited. src/dimension-labels.ts and the two __tests__ files PR #17470 holds were not touched.

Verification

Measured at f00bd2e50.

  • pnpm --filter @objectstack/service-analytics test2280 passed / 106 files, exit 0 (12 new; every pre-existing case passes unchanged).
  • pnpm --filter @objectstack/service-analytics typecheck — exit 0. Its tsc program reaches 104 files under src/__tests__/, the new suite among them (--listFiles), so the test layer is covered rather than advertised.
  • pnpm --filter '@objectstack/service-analytics^...' build — exit 0 (the dependency closure).
  • Gate families derived from the actual diff (dispatch-gates.mjs --commands, then --ran with exit codes): 61 derived, 58 run green, 3 NOT MEASURED, 0 unrun.
  • pnpm lint (eslint . --no-inline-config, whole repo) — exit 0.
  • node scripts/check-adr-0087-registration.mjs --base origin/main — exit 0; its own verdict line verifies the disposition's four predicates against analytics-service.ts#AnalyticsServiceConfig (interface).

NOT MEASURED: check:dual-build-cjs-loads, check:lean-entry-closure, check:type-check-debt — all three exited 3 (PREREQUISITE NOT MET), one cause: they read built output across the whole workspace and no full dist/ closure exists locally. That is a farm-wide build, and it belongs to CI. Recorded as not measured, never as a pass.

Derivation caveat, stated rather than smoothed: dispatch-gates reports this tree as STALE against a moving origin/main (one file it derives from, scripts/measure-reserved-identity-name-census.mjs, changed in that range). The derived family count was 61 both before and after a fresh fetch; CI on the merged generation is the reading of record.

Acceptance notes

Out of scope, noted and NOT filed:

  • DatasetScopedStrategyContext.sqlDialect (src/strategies/types.ts:97) stays string | undefined deliberately. That is the internal, runtime-honest seam: by the time a compiler asks, the value is whatever the host actually answered, and declaring it narrowly there would be a lie about runtime. The declared vocabulary belongs on the config hook the ruling names. Handed on to whichever PR next moves that file.
  • normalizeSqlDialect still answers unknown for a wrong answer and for no answer alike, and that stays correct at that call site — telling them apart is the contract seam's job, once, not a per-filter-node job. Successor: none needed; the reasoning is in the function's own docblock.

Nothing was filed as a new card from this run: the one finding worth a card is the wrong-rows measurement above, which belongs to #15684 (already open) and to this card's grade, both of which are the dispatching seat's to act on.

Docs drift — re-derived on the bot's own tree, and hand-checked past what it can see

Re-derived on 7a9f32c5ac401febed83434aacc111d63dcd24ba (the merge of f00bd2e50 into c7af6bd6f9), in a detached worktree at that commit — ⛔ not on a worktree cut from an older main, which would be a different tree rather than a wrong row. It reproduces exactly: 12 anchors, 1 hand-written page, 0 release-owned.

content/docs/plugins/packages.mdx — NOT falsified. The anchor is correct: AnalyticsServicePlugin is genuinely in this diff. But the page names it only twice, at :502 and :506, inside one composition example whose options are elided (new AnalyticsServicePlugin({ /* … */ })). The falsifying shape would be a sentence or sample on that page that names an AnalyticsServicePluginOptions member, passes a sqlDialect value, or states the plugin's dialect behaviour; none of the three is present, and this diff moves neither the class, its export, its name, nor its options type.

The emitter-blind half, hand-read because no anchor can ever reach it — a page stating this rule by its INPUTS shares no identifier with the emitter. Searched content/docs/** for sqlDialect, sqlite3 / better-sqlite3, and every dialect occurrence outside releases/, then read the analytics-facing and driver-facing pages by hand:

  • sqlDialect appears in zero pages. The hook is documented nowhere in content/docs/**, and nowhere in this package's README.md either. There is no page that tells a host what to answer, so there is none to falsify.
  • content/docs/data-modeling/drivers.mdx:100 lists sqlite and sqlite3 as accepted spellings — of the driver config's driver key, a driver-owned surface this diff does not touch. It stays true, and it is worth reading as CORROBORATION of the card's premise rather than as drift: the platform's own documentation teaches a host that sqlite3 is the spelling, one layer down.
  • drivers.mdx:76, :86, :356-:411 and packages.mdx:158 are MySQL dialect caveats and knex client names — driver config again, untouched.
  • content/docs/protocol/objectql/query-syntax.mdx:509, :532, :1025-:1026 say "on every dialect" about the $field compiler's backend coverage, not about this hook's accept set. Untouched: the runtime accept/reject behaviour of normalizeSqlDialect is byte-identical before and after.
  • content/docs/references/api/analytics.mdx:92 uses "dialect" in the retired-shim sense, unrelated.

⇒ No page is falsified by this diff. Recorded so nobody re-reads the same set.


Generated by Claude Code


Generated by Claude Code

…an out-of-contract answer

Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW
Co-authored-by: Claude <noreply@anthropic.com>
…t-of-contract answer (#16206)

Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics, touching 12 documentable anchor(s).

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

  • content/docs/plugins/packages.mdx (via AnalyticsServicePlugin (symbol, a top-level class))
What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • 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 — 9 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 c7af6bd6f9d3303a36b46209917a6f68658084e6packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json c7af6bd6f9d3303a36b46209917a6f68658084e6

⚠️ 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 c7af6bd6f9d3303a36b46209917a6f68658084e6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

os-sales commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Contract review (clause ②) — PASS · head f00bd2e50046ead4250d14ac0b894bd79d13fda4

head sha : f00bd2e50046ead4250d14ac0b894bd79d13fda4
base     : main @ c7af6bd6f9d3303a36b46209917a6f68658084e6
card     : #16206 · declaration `Clause-②: yes` (the ruling's own, decision batch #88, comment 5582368838)
surface  : 5 files — 1 changeset, 1 new test, 3 sources, all in packages/services/service-analytics/
reviewed : 2026-09-10T17:52Z · domain:services execution seat · default judgement tier

① Derived judgments — every accept-set and public-surface move the diff makes, named

  1. Four new exported symbols, all in src/text-match-sql.ts: ACCEPTED_SQL_DIALECTS (const tuple), AcceptedSqlDialect (type), isUnrecognisedSqlDialectAnswer (type guard), asAcceptedSqlDialect. Correct as yes — the mechanical floor ("a new exported symbol is always yes") is met four times over, so the declaration is right for a reason independent of the ruling that also named it.
  2. AnalyticsSqlDialect — no set change. 'sqlite' | 'postgres' | 'mysql' | 'unknown'AcceptedSqlDialect | 'unknown', which expands to the identical four names. Neither widened nor narrowed; a re-spelling only.
  3. KNOWN_DIALECTS — no runtime set change. new Set<string>(['sqlite','postgres','mysql'])new Set<string>(ACCEPTED_SQL_DIALECTS), the same three members. ⇒ The runtime accept set is not widened, which is precisely option B and precisely what the ruling refused by name. Correct.
  4. AnalyticsServiceConfig.sqlDialect — the one breaking limb, and it is a NARROWING. (object: string) => string | undefined(object: string) => AcceptedSqlDialect | undefined. The host supplies this function, so its return type is covariant at the composition site: a host whose hook is annotated string stops compiling. That is what the title's ! announces. Nothing reads the annotation at runtime, so no behaviour moves with it. Correct.
  5. The plugin.ts bridge is behaviour-preserving — verified, not accepted on the report's word. It now maps a driver's 'unknown' (and any unmodelled name, 'mariadb' among them) to undefined instead of passing it through. Downstream, normalizeSqlDialect(undefined) and normalizeSqlDialect('unknown') both answer 'unknown', and asAcceptedSqlDialect tests the very same KNOWN_DIALECTS membership normalizeSqlDialect does ⇒ every input compiles to the same construct before and after. The single observable difference is that the shipped bridge no longer trips the new diagnostic, which is the reason it exists.
  6. One new runtime emission, AnalyticsService.diagnoseSqlDialectAnswerprivate, so no public symbol moves. warn not error is the right side of AGENTS.md's one question (a construct is not enabled; nothing that claims to persist fails to land). Its dedupe key is the answer, held per instance, and the bound is pinned by measurement rather than argued: 50 laps and 200 laps both emit exactly 1.
  7. DatasetScopedStrategyContext.sqlDialect deliberately stays string | undefined — correct, and the report's reasoning is the right one: that seam sees what a host actually answered at runtime, and declaring it narrowly there would be a false declaration rather than a contract.
  8. Red line clear, measured. The diff's five paths are all under .changeset/ and packages/services/service-analytics/. packages/spec is untouched; driver-sql (domain:engine) is read and cited, never edited.

② semver grading

  • Changeset: @objectstack/service-analytics: minor — one package moved. Clause-②: yes requires minor or above ⇒ satisfied, and it is the grade the ruling itself named.
  • Breaking-ness is carried where AGENTS.md puts it: the changeset states the FROM → TO (string → the three names or undefined) and the one-line fix (annotate the host's own hook), and carries exactly one ADR-0087 disposition marker. Both are machine-checked, not taken on trust — check:adr-0087-registration exited 0 in delivery and Check Changeset is green on this head across three runs.
  • AGENTS.md sets no major requirement for a breaking change anywhere; the bump level is not this repo's carrier of breaking-ness, the banner and the disposition are. minor stands, and the declaration and the grade agree.

③ Boundary flags — every flag answered, none escalated

Independence pair

  • Implemented-by: branch claude/issue-16206-sqldialect-vocab — a mode:subagent dev has no session of its own, so its branch is its identity.
  • Reviewed-by: session_01ToDPcx9AESFubJkDiFMtKW — the domain:services execution seat.

Stated plainly rather than implied: this is a non-spec seat's Clause-② review, which is self-review at default judgement tier plus the gates, by design — the escalated-tier, context-isolated reviewer is the domain:spec seat's instrument, and this card was measured clear of packages/spec at claim time.

Verdict

PASS. Both carriers are stripped in the same stroke as this record, with a provenance comment citing it. Landing then waits on pre-check ③ only: Lint & Repo Gates and Type Check · workspace were still in_progress at 2026-09-10T17:50Z (25 success / 5 skipped / 2 running, of 32 distinct). ⛔ Not flipped ready and not armed until every check on this head is green — not a required subset.


Generated by Claude Code

@os-sales
os-sales marked this pull request as ready for review September 10, 2026 18:01
@os-sales
os-sales enabled auto-merge September 10, 2026 18:01

Copy link
Copy Markdown
Collaborator

ACCEPT — 2026-09-10T18:01Z, head f00bd2e50046ead4250d14ac0b894bd79d13fda4.

The three landing pre-checks, each measured rather than asserted:

  1. In-seat Clause-② review PASS on record — comment 5623044468 above, in the shape the gate reads (level-2 ## Contract review heading, this head's sha as a code span, a Reviewed-by: line). ①②③ + the independence pair are in it.
  2. Both carriers cleared, machine-readnode scripts/pm/check-clause2-carriers.mjs --pair 17496EXIT 0: the declaration is legible in the fixed spelling, both carriers agree, and a review of record names this head. The needs:contract-review label came off the PR and off card service-analytics: normalizeSqlDialect accepts only the three canonical names, so a host answering knex's own 'sqlite3' / 'pg' / 'mysql2' on the public sqlDialect hook is read as unknown — #11550's shape, one layer up #16206 in the same stroke, both read back; the card kept its assignee.
  3. Every check green on this head, not a required subsetdistinct: 33 | {success: 28, skipped: 5}, NOT GREEN: none. The five skips are the repository's conditional families, and Lint & Repo Gates finished completed / success.

Flipped ready for review and armed. ⛔ Not approved and ⛔ not merged by this seat; the merge queue lands it.

⚠️ One correction the delivery earned and did not get from me at review time, restated here so it travels with the PR: the report placed its wrong-rows finding on #15684 as "already open". #15684 is closed (completed, by merged PR #15790). The finding is card #16206's own — an unrecognised dialect answer lands on the unknown arm, which normalizeSqlDialect's docblock says compiles the pre-#15684 LIKE — and the card was regraded priority:p2 on that measurement, which is the ruling's own stated landing condition. Reporting the finding instead of quietly fixing it inside this PR was the right call.


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 size/l tests tooling

Projects

None yet

2 participants