Skip to content

fix(spec): narrow StrategyContext.executeAggregate aggregations[].method to AggregationFunction - #12937

Merged
os-trump merged 6 commits into
mainfrom
claude/issue-12776-aggregation-method-narrowing
Aug 28, 2026
Merged

fix(spec): narrow StrategyContext.executeAggregate aggregations[].method to AggregationFunction#12937
os-trump merged 6 commits into
mainfrom
claude/issue-12776-aggregation-method-narrowing

Conversation

@os-trump

Copy link
Copy Markdown
Collaborator

Fixes#12776

Maintainer ruling 2026-08-28 (comment 5448715921, option A, census-first): StrategyContext.executeAggregate's aggregations[].method narrows from bare string to the spec's own AggregationFunction (count | sum | avg | min | max | count_distinct, data/query.zod.ts) — the same closed vocabulary IDataEngine.aggregate declares for the identical slot (AggregationNodeSchema.function). One slot, one declaration; the #11833 runtime parse-and-refuse in the bridge stays as defence in depth (untouched, per the ruling).

Census (the ruling's hard precondition — measured BEFORE the narrowing landed, verdict: zero illegal)

Population: repo-wide grep for executeAggregate over all tracked TS (excluding dist), then every hit classified.

Producers (call sites filling method):

  • ObjectQLStrategy.execute main path — values from resolveMeasureAggregation: (a) direct measures emit direct.type after the A custom-SQL measure reaches engine.aggregate un-refused on the ObjectQL path and answers a silent null — the repair #12053's probe scoped #12209EXPRESSION_METRIC_TYPES refusal (number/string/boolean refused), and AggregationMetricType minus those three is exactly the six enum values; (b) the alias path requires membership in the hard-coded six-element list AND candidate.type === type equality; (c) fallback emits the literal count. LEGAL.
  • ObjectQLStrategy.resolveFkAttr (line ~1088) and plugin.ts displayField fetch (line ~508): literal count. LEGAL.
  • Test producers (grep of method: literals across service-analytics, rest, runtime, qa): only count/sum/avg/min/max/count_distinct observed. LEGAL.
  • Documented edge, unchanged by this PR: a cube registered while BYPASSING CubeSchema (host drift) could carry an out-of-enum type through the direct path — resolveMeasureAggregation's own comment declares this deliberately unblocked (the engine's undeclared-500 tier owns it, refusing here would re-blame the caller with a 400). Runtime posture preserved exactly.

Implementors (functions assigned into StrategyContext.executeAggregate): the plugin auto-bridge (which already parses method with the enum at runtime, from #11833), analytics-service.ts's config wiring, and roughly 45 test fakes across service-analytics, rest (3 files) and runtime (1 file). All accept method at string, any, or ignore the options — a handler accepting a superset stays assignable to the narrowed member (parameter contravariance), so implementors are source-compatible by construction. Verified empirically: zero fixture-side TS errors appeared (below).

Measured downstream impact (baseline-controlled)

tsc --noEmit on @objectstack/service-analytics was run twice against rebuilt spec dist — once with the BASE contract (mutation confirmed on disk and in dist, restore proven by empty git diff HEAD), once narrowed. The error sets differ by EXACTLY two lines: TS2322 at the strategy's two ctx.executeAggregate call sites, whose local aggregations annotations still said string. 10 pre-existing test-hygiene errors are byte-identical in both runs (that package has no typecheck script, so no CI gate reads them; noted for the PM as a finding candidate).

Those two errors are the narrowing working, and this PR carries the mechanical consumer fix in objectql-strategy.ts (a DECLARED deviation from the dispatch file surface, judged the anticipated mechanical-breakage allowance): the two local aggregations annotations and resolveMeasureAggregation's return type now carry the enum. The alias path proves it cast-free via the existing candidate.type === type guard (the six-element list becomes as const and the guard-proven-equal loop variable is returned); the direct path takes one type assertion with prose keeping the documented host-drift posture. Runtime behaviour byte-identical — after the fix the tsc error set is again identical to baseline (10 pre-existing, 0 new, 0 gone).

ADR-0087 (per the gate's own verdict, as ruled)

The gate refused not-required because the changeset carries a FROM-TO prescription, and demanded registration. Registered: semantic entry strategy-context-aggregation-method-narrowed (entries/semantic/18.…, registry regenerated by gen:migration-registry, self-check green). Changeset is breaking-grade prose on a minor bump per the launch-window convention (check-changeset-no-major green), FROM-TO stated, on @objectstack/spec (minor) and @objectstack/service-analytics (patch).

Verification

Union at ed2bd44dd (post-merge of main at 750fff5): spec build + typecheck green; spec tests 440 files / 11646 passed; service-analytics build green, tests 83 files / 1805 passed, tsc reading identical to baseline; consumer readings — rest fixture tests 3 files / 54 passed plus check:test-typecheck OK ("test layer compiles"), runtime fixture test 1 file / 11 passed (that fixture types options as any; no gate typechecks runtime tests — stated as not-measured, safe by contravariance). Gate families derived by dispatch-gates.mjs at the branch tree: 26 matched families run green, plus changeset-conditional families and the migrations-triggered check:spec-changes / check:upgrade-guide / check:query-options-erasure — all green. check:api-surface: measured UNCHANGED (gen produced zero diff — the dispatch expectation that it would move is falsified; the surface tracker does not fingerprint this inline options member). Two gates PREREQUISITE NOT MET locally, no reading either way, CI owns them: check-dev-prereqs (37 unrelated app packages have no local dist in this partial-build worktree) and check-half-states (no GitHub API credential in the container). After main moved a second time (docs/PM-tooling only, no path intersection), merged again and re-ran the ratchet set at 7ed016475: api-surface, authorable-surface, migration-registry, strictness-ledger, spec-changes, upgrade-guide, adr-0087, no-major, empty-changeset, nul-bytes — 10/10 green; test suites not re-run for that merge (incoming paths touch neither package — declared narrowing).

Clause-② applies (path limb and content limb): this PR stays DRAFT for the contract-review chain; the review chain owns enqueue.

Session: https://claude.ai/code/session_01LpRNHxWZgSUgVnFT9mQQo4

Generated by Claude Code


Generated by Claude Code

…hod to AggregationFunction
Maintainer ruling 2026-08-28 (option A, census-first). The engine contract
(IDataEngine.aggregate -> AggregationNodeSchema.function) and the analytics
strategy contract described the same slot with two types: a closed six-value
enum on one side, bare string on the other. One slot, one declaration:
method now carries the spec's own AggregationFunction
(count | sum | avg | min | max | count_distinct). The #11833 runtime
parse-and-refuse in the bridge stays as defence in depth.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LpRNHxWZgSUgVnFT9mQQo4
…ategy's aggregation locals
The spec narrowing surfaces exactly two TS2322s at the strategy's
ctx.executeAggregate call sites (measured against a BASE-spec baseline:
error sets differ by only these two). The local aggregations annotations
and resolveMeasureAggregation's return type now carry the enum; the alias
path proves it by the existing equality guard (no cast), the direct path
asserts it with prose keeping the documented no-allowlist posture for
host-drift cubes. Runtime behaviour unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LpRNHxWZgSUgVnFT9mQQo4
…entry)
The adr-0087 gate's own verdict: a changeset carrying a FROM->TO
prescription must register its migration. Semantic entry (D3): a TS
interface member has no authored document or sys_metadata row to rewrite,
so the ledger entry plus the compile error are the upgrade channel.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LpRNHxWZgSUgVnFT9mQQo4
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/service-analytics, @objectstack/spec, touching 5 documentable anchor(s).

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

  • content/docs/ai/natural-language-queries.mdx(via count_distinct (literal))
  • content/docs/data-modeling/queries.mdx(via count_distinct (literal))
  • content/docs/kernel/contracts/data-engine.mdx(via count_distinct (literal))
  • content/docs/protocol/objectql/query-syntax.mdx(via count_distinct (literal))
  • content/docs/ui/dashboards.mdx(via count_distinct (literal))

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

  • content/docs/releases/v15.mdx(via count_distinct (literal))
  • content/docs/releases/v17.mdx(via count_distinct (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
  • 5 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 — 126 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 6228c1f0ef3aad1946763b543e9fb207b8bf7de2packageMentionDocs.

Which tree this was computed on

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

⚠️ 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 6228c1f0ef3aad1946763b543e9fb207b8bf7de2 → 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 tooling labels Aug 28, 2026
@os-trump
os-trump marked this pull request as ready for review August 28, 2026 12:48
@os-trump
os-trump enabled auto-merge August 28, 2026 12:48
@os-trump
os-trump added this pull request to the merge queueAug 28, 2026
Merged via the queue into main with commit d028b37Aug 28, 2026
41 checks passed
@os-trump
os-trump deleted the claude/issue-12776-aggregation-method-narrowing branch August 28, 2026 13:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mtooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

finding: StrategyContext.executeAggregate declares aggregations[].method as string while the engine contract declares the six-value AggregationFunction

2 participants

@os-trump@claude