diff --git a/.changeset/strategy-context-aggregation-method-narrowed.md b/.changeset/strategy-context-aggregation-method-narrowed.md new file mode 100644 index 0000000000..4838f7b965 --- /dev/null +++ b/.changeset/strategy-context-aggregation-method-narrowed.md @@ -0,0 +1,42 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-analytics": patch +--- + +fix(spec): `StrategyContext.executeAggregate` `aggregations[].method` narrows from `string` to `AggregationFunction` (#12776) + + + +**BREAKING** accept-set narrowing on a published contract, landing after the +v17.0.0 cut (the lockstep launch-window convention ships it as `minor`). + +Two spec-declared surfaces described the same slot and disagreed about its +type: `IDataEngine.aggregate`'s `aggregations[].function` is the closed +six-value `AggregationFunction` enum, while the analytics strategy contract's +`StrategyContext.executeAggregate` declared the same value as +`aggregations[].method: string`. The analytics bridge renames one to the +other, so nothing on the analytics side of that seam was compile-checked +against the engine's vocabulary — a strategy author (very often an AI) got +no compile-time help and hit the bridge's runtime refusal instead. + +FROM → TO: + +- `aggregations[].method: string` → + `aggregations[].method: AggregationFunction` + (`'count' | 'sum' | 'avg' | 'min' | 'max' | 'count_distinct'`, the spec's + own enum from `@objectstack/spec/data`). One slot, one declaration. + +Who breaks at compile time on upgrade: + +- external CALLERS of `StrategyContext.executeAggregate` that fill `method` + with a value typed `string` (or a literal outside the six) — the values the + bridge already refused at runtime (#11833) now fail `tsc`. +- external IMPLEMENTORS of `StrategyContext` stay source-compatible: a + handler that accepts `method: string` accepts a superset and remains + assignable to the narrowed member. + +The bridge's runtime parse-and-refuse (#11833) stays as defence in depth. +In-repo, `ObjectQLStrategy`'s aggregation locals now carry the enum +end-to-end (`@objectstack/service-analytics`, runtime behaviour unchanged — +the census measured every reachable producer already emitting enum-legal +values only). diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 03c3227e36..a1e0a773ed 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; -import type { Cube } from '@objectstack/spec/data'; +import type { AggregationFunction, Cube } from '@objectstack/spec/data'; // [#8220] The read-scope provenance mark: `withReadScope` below is one of the // two merge boundaries that stamp it. import { markFilterSubtreeProvenance } from '@objectstack/spec/data'; @@ -172,7 +172,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // returns `null` for a filter that constrains nothing (an empty object), // matching the engine's own "empty filter is vacuous" convention — so a // vacuous measure filter adds no `filter` key rather than an empty one. - const aggregations: Array<{ field: string; method: string; alias: string; filter?: Record }> = []; + const aggregations: Array<{ field: string; method: AggregationFunction; alias: string; filter?: Record }> = []; if (query.measures && query.measures.length > 0) { for (const measure of query.measures) { const { field, method } = this.resolveMeasureAggregation(cube, measure); @@ -969,7 +969,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { private async executeCrossObject( cube: Cube, query: AnalyticsQuery, - aggregations: Array<{ field: string; method: string; alias: string; filter?: Record }>, + aggregations: Array<{ field: string; method: AggregationFunction; alias: string; filter?: Record }>, filter: Record, plan: CrossObjectPlan, ctx: StrategyContext, @@ -1259,7 +1259,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { return member.includes('.') ? member.split('.')[1] : member; } - private resolveMeasureAggregation(cube: Cube, measureName: string): { field: string; method: string } { + private resolveMeasureAggregation(cube: Cube, measureName: string): { field: string; method: AggregationFunction } { const direct = this.lookupMember(cube, measureName, 'measure') as | { sql: string; type: string } | undefined; @@ -1306,7 +1306,13 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } return { field: direct.sql.replace(/^\$/, ''), - method: direct.type === 'count_distinct' ? 'count_distinct' : direct.type, + // The assertion, not a parse: for a CubeSchema-legal cube the type + // partition above leaves exactly the six `AggregationFunction` values. + // An enum-INVALID type (host drift, the comment above) still flows + // through unchecked ON PURPOSE — adding a method allowlist here would + // re-blame the caller with a 400 for OUR bug, so the cast keeps the + // compile-time contract (#12776) without changing that posture. + method: (direct.type === 'count_distinct' ? 'count_distinct' : direct.type) as AggregationFunction, }; } // Accept `${field}_${type}` aliases (e.g. 'amount_sum') for measures whose @@ -1314,16 +1320,18 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // This matches the convention used by clients that build measure names // from (field, function) pairs (e.g. the data-objectstack adapter). const fieldName = measureName.includes('.') ? measureName.split('.')[1] : measureName; - const aggTypes = ['count', 'sum', 'avg', 'min', 'max', 'count_distinct']; + const aggTypes = ['count', 'sum', 'avg', 'min', 'max', 'count_distinct'] as const; for (const type of aggTypes) { const suffix = `_${type}`; if (fieldName.endsWith(suffix)) { const baseField = fieldName.slice(0, -suffix.length); const candidate = cube.measures[baseField]; if (candidate && candidate.type === type) { + // `type` ranges over the six `AggregationFunction` literals and the + // guard just proved `candidate.type` equal to it (#12776) — no cast. return { field: candidate.sql.replace(/^\$/, ''), - method: candidate.type === 'count_distinct' ? 'count_distinct' : candidate.type, + method: type, }; } } diff --git a/packages/spec/src/contracts/analytics-service.ts b/packages/spec/src/contracts/analytics-service.ts index cabd3884f3..e958ee2167 100644 --- a/packages/spec/src/contracts/analytics-service.ts +++ b/packages/spec/src/contracts/analytics-service.ts @@ -2,6 +2,7 @@ import type { AnalyticsQuery, Cube } from '../data/analytics.zod.js'; import type { FilterCondition } from '../data/filter.zod.js'; +import type { AggregationFunction } from '../data/query.zod.js'; import type { PercentScale } from '../data/percent-scale.js'; import type { ExecutionContext } from '../kernel/execution-context.zod.js'; import type { Dataset } from '../ui/dataset.zod.js'; @@ -296,8 +297,14 @@ export interface StrategyContext { * (`AggregationNodeSchema.filter`), which the engine honours on every * driver by lowering in memory when the driver has no native * conditional aggregation. + * + * `method` is the engine's own closed vocabulary + * (`AggregationFunction`, data/query.zod.ts) — the same slot + * `engine.aggregate`'s `aggregations[].function` declares. It was + * `string` until #12776; the bridge's runtime parse-and-refuse + * (#11833) stays as defence in depth behind this compile-time check. */ - aggregations?: Array<{ field: string; method: string; alias: string; filter?: FilterCondition }>; + aggregations?: Array<{ field: string; method: AggregationFunction; alias: string; filter?: FilterCondition }>; filter?: Record; /** * Reference timezone (IANA name) for date bucketing (ADR-0053 Phase 2). diff --git a/packages/spec/src/migrations/entries/semantic/18.strategy-context-aggregation-method-narrowed.ts b/packages/spec/src/migrations/entries/semantic/18.strategy-context-aggregation-method-narrowed.ts new file mode 100644 index 0000000000..74c2dc8761 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.strategy-context-aggregation-method-narrowed.ts @@ -0,0 +1,51 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'strategy-context-aggregation-method-narrowed', + surface: 'StrategyContext.executeAggregate aggregations[].method ' + + '(contracts/analytics-service.ts, exported from @objectstack/spec/contracts) ' + + '- the parameter type, declared as bare string', + replacement: 'AggregationFunction (count | sum | avg | min | max | count_distinct, ' + + 'data/query.zod.ts) - the same closed vocabulary IDataEngine.aggregate already ' + + 'declares for the identical slot (AggregationNodeSchema.function; the analytics ' + + 'bridge renames method to function and forwards). A caller filling method from a ' + + 'string-typed value narrows the value to the enum - typing it ' + + 'AggregationFunction, or parsing with the spec\'s own AggregationFunction zod ' + + 'enum where the value enters from data. Values outside the six were never ' + + 'served: the bridge has parsed-and-refused them at runtime since #11833, and ' + + 'that refusal stays as defence in depth', + reason: + '#12776, maintainer ruling 2026-08-28 (option A, census-first). Two spec-declared ' + + 'surfaces described the same value and disagreed about its type: ' + + 'IDataEngine.aggregate\'s aggregations[].function is the closed six-value ' + + 'AggregationFunction enum while StrategyContext.executeAggregate declared the ' + + 'same slot aggregations[].method: string, so nothing on the analytics side of ' + + 'that seam was compile-checked against the engine\'s vocabulary - an author, ' + + 'very often an AI (ADR-0033), writing an analytics strategy got no compile-time ' + + 'help and could carry any method name all the way to the bridge\'s runtime ' + + 'refusal. One slot now has one declaration. Bookkeeping: this is a TYPE ' + + 'narrowing on a runtime TS interface member - no authorable metadata key, no ' + + 'wire shape and no walked-shape def changed, so nothing lands in ' + + 'RETIRED_KEYS_BY_MAJOR / RETIRED_DEFS_BY_MAJOR and the surface ratchets are ' + + 'expected byte-identical. It is a SEMANTIC entry rather than a D2 conversion ' + + 'because there is no authored document or sys_metadata row for the chain to ' + + 'rewrite: the only consumers are TypeScript call sites, and the compile error ' + + 'is the channel that reaches them. In-repo census at the ruling (hard ' + + 'precondition, measured before the narrowing landed): every implementor and ' + + 'every call site filling method is legal under the enum - ' + + 'ObjectQLStrategy.resolveMeasureAggregation emits only the six post-#12209 ' + + 'refusal, the two literal producers write count, and every test fixture is ' + + 'implementor-side and stays assignable by contravariance.', + acceptanceCriteria: + 'External implementors of StrategyContext stay source-compatible: a handler ' + + 'accepting method: string accepts a superset and remains assignable to the ' + + 'narrowed member. External callers filling method with a string-typed or ' + + 'out-of-vocabulary value fail tsc at the executeAggregate call site on upgrade; ' + + 'the fix is narrowing the value\'s type to AggregationFunction (parsing with ' + + 'the spec enum where it enters from data), never widening a local mirror of ' + + 'the contract. Runtime behaviour is unchanged: the bridge\'s #11833 ' + + 'parse-and-refuse accepts and rejects exactly the same sets before and after, ' + + 'and no stored metadata or document needs editing.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 71b1b31ea3..fe4afa46c7 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -7128,6 +7128,53 @@ const step18: MigrationStep = { + 'of an envelope-level code; constructing an ApiError with a retired spelling ' + 'fails `StandardErrorCode`/`ApiErrorSchema` parse rather than passing silently.', }, + { + id: 'strategy-context-aggregation-method-narrowed', + surface: 'StrategyContext.executeAggregate aggregations[].method ' + + '(contracts/analytics-service.ts, exported from @objectstack/spec/contracts) ' + + '- the parameter type, declared as bare string', + replacement: 'AggregationFunction (count | sum | avg | min | max | count_distinct, ' + + 'data/query.zod.ts) - the same closed vocabulary IDataEngine.aggregate already ' + + 'declares for the identical slot (AggregationNodeSchema.function; the analytics ' + + 'bridge renames method to function and forwards). A caller filling method from a ' + + 'string-typed value narrows the value to the enum - typing it ' + + 'AggregationFunction, or parsing with the spec\'s own AggregationFunction zod ' + + 'enum where the value enters from data. Values outside the six were never ' + + 'served: the bridge has parsed-and-refused them at runtime since #11833, and ' + + 'that refusal stays as defence in depth', + reason: + '#12776, maintainer ruling 2026-08-28 (option A, census-first). Two spec-declared ' + + 'surfaces described the same value and disagreed about its type: ' + + 'IDataEngine.aggregate\'s aggregations[].function is the closed six-value ' + + 'AggregationFunction enum while StrategyContext.executeAggregate declared the ' + + 'same slot aggregations[].method: string, so nothing on the analytics side of ' + + 'that seam was compile-checked against the engine\'s vocabulary - an author, ' + + 'very often an AI (ADR-0033), writing an analytics strategy got no compile-time ' + + 'help and could carry any method name all the way to the bridge\'s runtime ' + + 'refusal. One slot now has one declaration. Bookkeeping: this is a TYPE ' + + 'narrowing on a runtime TS interface member - no authorable metadata key, no ' + + 'wire shape and no walked-shape def changed, so nothing lands in ' + + 'RETIRED_KEYS_BY_MAJOR / RETIRED_DEFS_BY_MAJOR and the surface ratchets are ' + + 'expected byte-identical. It is a SEMANTIC entry rather than a D2 conversion ' + + 'because there is no authored document or sys_metadata row for the chain to ' + + 'rewrite: the only consumers are TypeScript call sites, and the compile error ' + + 'is the channel that reaches them. In-repo census at the ruling (hard ' + + 'precondition, measured before the narrowing landed): every implementor and ' + + 'every call site filling method is legal under the enum - ' + + 'ObjectQLStrategy.resolveMeasureAggregation emits only the six post-#12209 ' + + 'refusal, the two literal producers write count, and every test fixture is ' + + 'implementor-side and stays assignable by contravariance.', + acceptanceCriteria: + 'External implementors of StrategyContext stay source-compatible: a handler ' + + 'accepting method: string accepts a superset and remains assignable to the ' + + 'narrowed member. External callers filling method with a string-typed or ' + + 'out-of-vocabulary value fail tsc at the executeAggregate call site on upgrade; ' + + 'the fix is narrowing the value\'s type to AggregationFunction (parsing with ' + + 'the spec enum where it enters from data), never widening a local mirror of ' + + 'the contract. Runtime behaviour is unchanged: the bridge\'s #11833 ' + + 'parse-and-refuse accepts and rejects exactly the same sets before and after, ' + + 'and no stored metadata or document needs editing.', + }, { id: 'ui-cloud-connection-widgets-unknown-keys-refused', surface: 'page `cloud-connection:panel` / `marketplace:installed-list` components — '