Skip to content

fix(service-analytics): refuse custom-SQL measures on the ObjectQL aggregate path - #12318

Merged
os-trump merged 1 commit into
mainfrom
claude/issue-12209-objectql-custom-sql-measure-refusal
Aug 25, 2026
Merged

fix(service-analytics): refuse custom-SQL measures on the ObjectQL aggregate path#12318
os-trump merged 1 commit into
mainfrom
claude/issue-12209-objectql-custom-sql-measure-refusal

Conversation

@os-trump

Copy link
Copy Markdown
Collaborator

Fixes#12209

What

An authored custom-SQL measure (AggregationMetricTypenumber / string / boolean) used to reach engine.aggregate un-refused on the ObjectQL path: resolveMeasureAggregation forwarded the metric type verbatim as the engine method with the whole SQL expression in field, so driver-sql threw INVALID_QUERY blaming a function key the author never wrote, and the in-memory evaluator answered a silent per-bucket null under the author's own metric name — the #4157 class in its null variant, measured by #12053's probe (an admitted sum returned 300 per bucket where the custom-SQL measure returned null).

This PR adds the INVALID_FIELD / 400 refusal arm (ADR-0112, via invalidMemberError) on the direct path of ObjectQLStrategy, naming the measure the author wrote and its metric type, in the posture of the in-file cross-object twin (ending "…or run on a native-SQL driver"). Option B (lowering the expression through the aggregate AST) is a packages/spec contract question and is deliberately NOT here; per the maintainer's #11833 ruling this is also not closed by widening any local type — no type was touched.

Where the arm sits, and why

Inside resolveMeasureAggregation's declared-measure branch — the one resolver both doors (execute() and generateSql()) call — so /analytics/query and /analytics/sql accept/reject the same set by construction (#10759's invariant). Two deliberate consequences, both pinned:

  • The arm keys on the declared EXPRESSION_METRIC_TYPES partition (ONE source, imported from NativeSQLStrategy, pinned against the spec enum by metric-type-coverage.test.ts), deliberately NOT on a method allowlist. The two read identically on every enum-valid cube; they differ on enum-INVALID drift (e.g. a host-registered cube with type median), which stays the platform's own undeclared-500 tier per dataset-refusal.ts's header instead of being re-blamed on the caller as a 400. A pinned case makes a method-allowlist implementation go red.
  • A custom-SQL measure beside a cross-object dimension now refuses with the custom-SQL message rather than the non-recombinable one (the measure can never run on this engine, cross-object dimension or not, and the one-resolver placement keeps the two doors from forking on attribution). The pre-existing non-recombinable refusal itself is untouched and its exact shipped message is pinned verbatim.

The pins — one fixture, BOTH strategies

The defect existed because NativeSQLStrategy's regression pin measure-expression-sql.test.ts forces objectqlAggregate: false, covering one strategy of two. The new pin measure-expression-both-strategies.test.ts is a shared table: one cube (all six aggregate types + all three expression types + one enum-invalid drift type) driven through the real AnalyticsService routing under both capability profiles, so neither strategy's fixture can hide the other:

  1. ObjectQL profile: each expression measure refused with codeINVALID_FIELDandstatus 400, member = the measure as the author wrote it, parammeasures, cube named, the message naming the measure and metric type — and the engine never reached (calls and sqls both empty). Also on the scalar shape and in a mixed query.
  2. Load-bearing negative: an admitted sum measure is still served and still reaches the engine carrying { field: 'amount', method: 'sum' }; all six aggregates likewise, each carrying its own method. Plus the drift-tier case above — the case that reds a "refuse every method that is not one of the six aggregates" implementation, which passes pin 1.
  3. The cross-object non-recombinable refusal keeps its exact message (full-string equality).
  4. Native profile, same fixture: the same expression measures stay SERVED, emitted verbatim.

Dissolution verification (direction predicted in writing before running)

Predicted: restoring the accepting behaviour (strategy file back at base 5ce5f8c12) turns exactly the six ObjectQL-profile refusal cases red — no error, engine reached — and leaves the other six green. Measured exactly that:

Tests 6 failed | 6 passed (12)
AssertionError: expected undefined to be an instance of Error
at expectCustomSqlRefusal src/__tests__/measure-expression-both-strategies.test.ts:172:19

Mutation was confirmed on disk before reading results (grep of the throw anchor: 1 → 0 after git restore --source=5ce5f8c12; the pin runs the package's own source via relative imports, no dist in the resolution path). Restored via git checkout HEAD -- the strategy file; disk == index == HEAD verified (empty porcelain, empty diff HEAD, byte-diff against HEAD: and : both clean).

Verification at final commit f31671525

  • pnpm --filter @objectstack/service-analytics test — 81 files, 1790 passed (run at f31671525 after the final commit).
  • New pin standalone: 12/12.
  • pnpm --filter @objectstack/service-analytics build (incl. DTS) — clean.
  • Package tsc --noEmit error count is exactly the type-check DEBT ledger's frozen 10, none in the new test file (measured directly, the way the ledger measures; the workspace-wide check:type-check-debt sweep is CI's).
  • Derived gates (scripts/pm/dispatch-gates.mjs, no paths — changed-set self-derived): check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check:page-declaration-shape, check:published-files, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:engine-double-contract, check:cross-package-test-inputs, check:where-matcher, check:type-check-coverage, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-plugin-teardown-shape, docs-audit/check-affected-docs, docs-audit/check-drift-comment, release-rehearsal-clone --self-test — all exit 0 (exit codes captured before any pipe).

Changeset

@objectstack/service-analyticspatch: states plainly what stops being served and for whom — on ObjectQL-path deployments a custom-SQL measure now answers a loud 400 instead of a 200 full of per-bucket nulls; native-SQL behaviour unchanged. No authorable key removed, no ADR-0087 disposition required (not declared-breaking).

This PR is DRAFT and carries needs:contract-review (it changes contract accept/reject behaviour on an authorable surface); it awaits the contract-review chain — do not flip ready or arm auto-merge.


Generated by Claude Code

…gregate path
A measure whose AggregationMetricType is number/string/boolean declares a raw
SQL expression as its whole computation. ObjectQLStrategy.resolveMeasureAggregation
forwarded the metric type verbatim as the engine method with the expression in
field, so driver-sql threw INVALID_QUERY blaming a function key the author
never wrote, and the in-memory evaluator answered null for every bucket through
its switch default. The prior fix for this class landed on NativeSQLStrategy
only, and its regression pin forces objectqlAggregate: false, so it covered one
strategy of two.
The ObjectQL path now refuses such a measure with INVALID_FIELD / 400 (ADR-0112
via invalidMemberError), naming the measure the author wrote and its metric
type, in the posture of the in-file cross-object refusal twin. The arm sits in
the one resolver both doors call, so /analytics/query and /analytics/sql
accept/reject the same set by construction. It is keyed on the declared
EXPRESSION_METRIC_TYPES partition (one source shared with NativeSQLStrategy),
deliberately not on a method allowlist: an enum-invalid drift type stays the
platform's own undeclared-500 tier instead of being re-blamed on the caller.
New pin drives one fixture through BOTH strategies: refusal envelope on the
ObjectQL profile, expressions still served verbatim on the native profile, all
six admitted aggregates still reaching the engine carrying their own methods,
and the cross-object non-recombinable refusal keeping its exact message.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ⚠️1 changed file(s) yielded no anchor (packages/services/service-analytics/src/strategies/native-sql-strategy.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-analytics/src/strategies/native-sql-strategy.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 177: 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; 107 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 — 8 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 783111d25227b7f907f2a0059797056e0fb78e8apackageMentionDocs.

Which tree this was computed on

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

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

@os-trumpClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract-review verdict: PASS (#12209)

Reviewed by the contract-review chain at CONTRACT_REVIEW_TIER. Tier fuse, machine reading this sub-round: get_session.external_metadata.last_served_model = claude-fable-5.

Reviewed: the refusal arm and partition-export patches in full, the pin design with its dissolution record, and the lineage on #12209 (the #12053 probe, the #11833 no-type-widening constraint, the #4157 recurrence history).

Conformance: the arm sits in the one resolver both doors call (#10759's invariant held by construction), throws invalidMemberError with member/param/cube in the twin's envelope and remedy posture, names the measure the author wrote and its metric type — ending exactly the mis-attribution this card is about. It keys on the shared EXPRESSION_METRIC_TYPES partition imported from NativeSQLStrategy (one source, spec-enum-pinned, with the exporter-side comment closing the fork risk) — and the enum-invalid drift case staying the platform's 500 tier is a distinction sharper than the brief, correctly refusing to re-blame the caller for host drift, and pinned so an allowlist implementation reds. Zero spec, zero deletions, option B untouched. The both-profiles shared-table pin is the piece that stops #4157's third recurrence: neither strategy's fixture can hide the other again.

The two judgment calls the seat put to this tier, both ratified:

  1. The attribution move (custom-SQL beside a cross-object dimension now gets the custom-SQL message): correct — the measure can never run on this engine regardless of dimensions, the partition check belongs first, and the single-resolver placement is what keeps the two doors from forking on attribution. Pinned, and the pre-existing non-recombinable message is full-string-pinned untouched.
  2. patch: ratified. The served behaviour was wrong by construction — per-bucket null under the author's own metric name, or a 400 blaming a key the author never wrote — so no correct behaviour a host relied on narrows; a diagnosis replacing a wrong answer is a fix, not a surface change. The changeset states what stops being served and for whom, plainly.

Landing completed by the chain in the same stroke per the maintainer's standing instruction (2026-08-25, verbatim: 「审核通过你应该直接负责合并吧,还要等原始的项目经理吗」): clearing needs:contract-review on this PR and #12209, flipping ready, arming the queue — the queue waits for the in-flight CI.


Generated by Claude Code

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.

A custom-SQL measure reaches engine.aggregate un-refused on the ObjectQL path and answers a silent null — the repair #12053's probe scoped

2 participants

@os-trump@claude