fix(spec): EngineAggregateOptions.groupBy declares the GroupByNodeSchema union the engine already reads (#8032) (#8236)

huangyiirene · claude · web-flow · commit 3f7b4ff4fffa · 2026-08-13T01:38:57.000Z
* fix(spec): declare EngineAggregateOptions.groupBy as the GroupByNodeSchema union (#8032) The engine has always read structured { field, dateGranularity, alias } buckets on this key (date bucketing, credential-aggregation guard); the declaration said string[], so every correct caller had to cast. Catch the declaration up to the enforced contract: - spec: EngineAggregateOptionsSchema.groupBy -> z.array(GroupByNodeSchema), pin tests in both directions (structured buckets parse; plain strings parse byte-identically; bad dateGranularity / field-less bucket rejected) - objectql: engine drops its own groupBy casts; the two landed test casts removed (internal-fields structured-bucket case now type-checks honestly; filter-array-lowering cast narrowed to the deliberately off-contract where slot) - mcp: McpDataBridge.aggregate retyped onto the engine's own EngineAggregateOptions slices - closes function:string vs the six-name enum, dateGranularity:string vs the five-name vocabulary, and distinct?:boolean vs the #6815 retiredKey tombstone; stdio bridge compiles with zero casts No engine/runtime behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123k4cam2jEAkPmbJeoaY3r * docs(spec): regenerate references for the EngineAggregateOptions.groupBy union (#8032) check:generated proved exactly one artifact stale (gen:docs); regenerated that one only. gen:openapi restored after gen:schema's cleanup (untracked, kept for the local rest runs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123k4cam2jEAkPmbJeoaY3r * merge origin/main (os-regen artifacts taken from main; regeneration follows) * docs(spec): re-apply the groupBy union reference rows after os-regen merge (#8032) os-regen-merge took main's side of references/**; gen:docs re-applies the three EngineAggregateOptions.groupBy rows on top of the merged state. check:generated: all 13 artifacts up to date. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123k4cam2jEAkPmbJeoaY3r --------- Co-authored-by: Claude <noreply@anthropic.com>
diff --git a/.changeset/engine-aggregate-groupby-union.md b/.changeset/engine-aggregate-groupby-union.md
@@ -0,0 +1,5 @@
+---
+"@objectstack/spec": minor
+---
+
+`EngineAggregateOptionsSchema.groupBy` now declares the standard `GroupByNodeSchema` union — a bare field name, or a `{ field, dateGranularity?, alias? }` bucket object for date bucketing — the same vocabulary `QuerySchema.groupBy` has always declared (#8032). The engine, driver-mongodb and the in-memory aggregation path have always executed the structured form; the engine-options declaration was the one face still saying `string[]`, so every correct caller had to cast around it. This widens the declared accept-set only: plain-string `groupBy` payloads validate byte-identically and no runtime behavior changes.
diff --git a/.changeset/mcp-bridge-aggregate-declared-types.md b/.changeset/mcp-bridge-aggregate-declared-types.md
@@ -0,0 +1,5 @@
+---
+"@objectstack/mcp": patch
+---
+
+`McpDataBridge.aggregate` now declares its `groupBy` / `aggregations` inputs as the engine's own `EngineAggregateOptions` slices instead of a hand-mirrored copy (#8032). The mirror had drifted in three places: `function: string` against the engine's six-name enum, `dateGranularity?: string` against the `day`/`week`/`month`/`quarter`/`year` vocabulary, and a `distinct?: boolean` the engine retired in `@objectstack/spec` 17 (#6815) — a caller passing `distinct: true` had it silently dropped, and now gets the retirement rejection at compile time instead. Delete the key; a deduplicated count is the `count_distinct` aggregation function. Runtime acceptance is unchanged on every path: the `aggregate_records` tool's zod schema already enforced exactly these shapes at the ingress, and the stdio bridge's engine call no longer needs its two casts.
diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx
@@ -65,7 +65,7 @@ Options for DataEngine.aggregate operations
 | :--- | :--- | :--- | :--- |
 | **method** | `'aggregate'` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `{ context?: object; where?: Record<string, any> \| any; groupBy?: string[]; aggregations?: object[]; … }` | ✅ |  |
+| **query** | `{ context?: object; where?: Record<string, any> \| any; groupBy?: (string \| object)[]; aggregations?: object[]; … }` | ✅ |  |
 
 
 ---
@@ -328,7 +328,7 @@ This schema accepts one of the following structures:
 | :--- | :--- | :--- | :--- |
 | **method** | `'aggregate'` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `{ context?: object; where?: Record<string, any> \| any; groupBy?: string[]; aggregations?: object[]; … }` | ✅ |  |
+| **query** | `{ context?: object; where?: Record<string, any> \| any; groupBy?: (string \| object)[]; aggregations?: object[]; … }` | ✅ |  |
 
 ---
 
@@ -466,7 +466,7 @@ QueryAST-aligned options for DataEngine.aggregate operations
 | :--- | :--- | :--- | :--- |
 | **context** | `{ userId?: string; actor?: string; attributedUserId?: string; email?: string; … }` | optional |  |
 | **where** | `Record<string, any> \| any` | optional |  |
-| **groupBy** | `string[]` | optional |  |
+| **groupBy** | `(string \| { field: string; dateGranularity?: Enum<'day' \| 'week' \| 'month' \| 'quarter' \| 'year'>; alias?: string })[]` | optional | GROUP BY targets (strings or `{field, dateGranularity?}` objects for date bucketing) |
 | **aggregations** | `{ function: Enum<'count' \| 'sum' \| 'avg' \| 'min' \| 'max' \| 'count_distinct'>; field?: string; alias: string; filter?: any }[]` | optional |  |
 | **having** | `any` | optional | HAVING — filter over the aggregated rows (aggregation aliases + groupBy projections); applied engine-side after aggregation |
 | **timezone** | `string` | optional |  |
diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts
@@ -34,6 +34,7 @@
 
 import { z } from 'zod';
 import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
+import type { EngineAggregateOptions } from '@objectstack/spec/data';
 import {
   MCP_OAUTH_SCOPE_DATA_READ,
   MCP_OAUTH_SCOPE_DATA_WRITE,
@@ -80,13 +81,21 @@ export interface McpDataBridge {
    * aggregation through the engine simply omits it and the
    * `aggregate_records` tool is not registered (graceful degradation, same
    * contract as {@link McpActionBridge}).
+   *
+   * `groupBy` / `aggregations` are the engine's own declarations
+   * (`EngineAggregateOptions`, #8032) rather than a hand-mirrored copy: the
+   * tool's zod schema below already enforces exactly these shapes at the
+   * ingress, and a private restatement is where the two had drifted — this
+   * interface used to declare `function: string` against the six-name enum
+   * and a `distinct?: boolean` the engine retired (#6815), silently dropping
+   * any caller who believed it.
    */
   aggregate?(
     object: string,
     opts: {
       where?: Record<string, unknown>;
-      groupBy?: Array<string | { field: string; dateGranularity?: string; alias?: string }>;
-      aggregations: Array<{ function: string; field?: string; alias: string; distinct?: boolean }>;
+      groupBy?: NonNullable<EngineAggregateOptions['groupBy']>;
+      aggregations: NonNullable<EngineAggregateOptions['aggregations']>;
       timezone?: string;
     },
   ): Promise<unknown[]>;
diff --git a/packages/mcp/src/stdio-data-bridge.ts b/packages/mcp/src/stdio-data-bridge.ts
@@ -48,13 +48,9 @@
  */
 
 import type { ExecutionContext } from '@objectstack/spec/kernel';
-import type { EngineAggregateOptions } from '@objectstack/spec/data';
 import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts';
 import type { McpDataBridge, McpObjectSummary } from './mcp-http-tools.js';
 
-/** The engine's own aggregation-node list — see the cast note in `aggregate`. */
-type EngineAggregations = NonNullable<EngineAggregateOptions['aggregations']>;
-
 /** What {@link createStdioDataBridge} needs from the host plugin. */
 export interface StdioDataBridgeDeps {
   /** The ObjectQL engine — the `objectql` service, where RLS/FLS/permissions run. */
@@ -210,27 +206,15 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge
   if (typeof engine.aggregate === 'function') {
     bridge.aggregate = async (object, opts) => {
       const context = await resolvePrincipal();
-      // Two casts, one cause: `McpDataBridge.aggregate` declares a WIDER input
-      // than `EngineAggregateOptions` accepts, and the HTTP path never noticed
-      // because it reaches the engine through `callData`'s untyped `params`.
-      //
-      //  - `groupBy`: the bridge (and the `aggregate_records` tool schema)
-      //    allow `{ field, dateGranularity, alias }` objects; the engine option
-      //    declares `string[]` — while the `timezone` doc three lines below it
-      //    in that same schema describes "groupBy items carrying a
-      //    dateGranularity". The runtime contract is the object form.
-      //  - `aggregations`: the bridge declares `function: string`; the engine's
-      //    `AggregationNode` closes it to the six-name enum. The tool's own zod
-      //    schema already enforces exactly that enum before a value reaches
-      //    here, so the wide spelling is the interface's, never the caller's.
-      //
-      // Casting keeps this transport's request byte-identical to the HTTP one
-      // rather than narrowing the declared tool input on one transport only.
-      // The declaration mismatch itself is filed rather than papered over here.
+      // No casts: `McpDataBridge.aggregate` declares the engine's own
+      // `EngineAggregateOptions` slices since #8032, so the honest call
+      // compiles — the two `as unknown as` casts this line used to carry
+      // existed only because the engine option declared `groupBy: string[]`
+      // while reading structured buckets.
       const rows = await engine.aggregate(object, {
         ...(opts?.where ? { where: opts.where } : {}),
-        ...(opts?.groupBy ? { groupBy: opts.groupBy as unknown as string[] } : {}),
-        aggregations: opts.aggregations as unknown as EngineAggregations,
+        ...(opts?.groupBy ? { groupBy: opts.groupBy } : {}),
+        aggregations: opts.aggregations,
         ...(opts?.timezone ? { timezone: opts.timezone } : {}),
         context,
       });
diff --git a/packages/objectql/src/engine-filter-array-lowering.test.ts b/packages/objectql/src/engine-filter-array-lowering.test.ts
@@ -440,9 +440,15 @@ describe('Door 2 lowers FilterArray to FilterCondition before the driver (#5158)
       .rejects.toMatchObject({ status: 400 });
     await expect(engine.count('deal', { where } as unknown as EngineCountOptions))
       .rejects.toMatchObject({ status: 400 });
+    // Only `where` is off-contract in this call (FilterArray, deliberately —
+    // module note on `asFilterArrayQuery`); since #8032 `groupBy` and
+    // `aggregations` type-check honestly, so the cast is scoped to the one
+    // slot whose contract is being bypassed.
     await expect(engine.aggregate('deal', {
-      where, groupBy: ['stage'], aggregations: [{ function: 'count', field: 'id', alias: 'n' }],
-    } as unknown as EngineAggregateOptions)).rejects.toMatchObject({ status: 400 });
+      where: where as unknown as EngineAggregateOptions['where'],
+      groupBy: ['stage'],
+      aggregations: [{ function: 'count', field: 'id', alias: 'n' }],
+    })).rejects.toMatchObject({ status: 400 });
     await expect(engine.update('deal', { amount: 1 }, { where, multi: true } as any))
       .rejects.toMatchObject({ status: 400 });
     await expect(engine.delete('deal', { where, multi: true } as any))
diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts
@@ -9897,8 +9897,8 @@ export class ObjectQL implements IObjectQLEngine {
       const field = (agg as { field?: string })?.field;
       if (field && field !== '*') referenced.add(field);
     }
-    for (const g of (query?.groupBy as unknown[]) ?? []) {
-      const field = typeof g === 'string' ? g : (g as { field?: string })?.field;
+    for (const g of query?.groupBy ?? []) {
+      const field = typeof g === 'string' ? g : g?.field;
       if (field) referenced.add(field);
     }
 
@@ -9934,7 +9934,7 @@ export class ObjectQL implements IObjectQLEngine {
         ast: {
             object,
             where: query.where,
-            groupBy: query.groupBy as any,
+            groupBy: query.groupBy,
             aggregations: query.aggregations,
             // ENFORCED since #4286 (step 3). On the ast so the FLS predicate
             // guard walks its references (predicate-guard.ts) and a future
@@ -9961,11 +9961,11 @@ export class ObjectQL implements IObjectQLEngine {
         // supported we can push the aggregate down to the driver; otherwise
         // we fall back to driver.find() + in-memory bucketing so the result
         // remains correct on partial-support dialects (e.g. SQLite + week).
-        const groupByItems = Array.isArray(query.groupBy) ? (query.groupBy as any[]) : [];
+        const groupByItems = Array.isArray(query.groupBy) ? query.groupBy : [];
         const granularityCaps: Record<string, boolean> | undefined =
             drv?.supports?.queryDateGranularity;
         const structuredItems = groupByItems.filter((g) => typeof g !== 'string');
-        const allStructuredSupported = structuredItems.every((g: any) => {
+        const allStructuredSupported = structuredItems.every((g) => {
             if (!g?.dateGranularity) return true; // plain {field} object is fine
             return granularityCaps?.[g.dateGranularity] === true;
         });
@@ -9977,7 +9977,7 @@ export class ObjectQL implements IObjectQLEngine {
         // matching rows are fetched), but bucketing runs uniformly in JS so a
         // row near a tz day-boundary lands identically on every driver.
         const tz = query.timezone;
-        const hasDateBucket = structuredItems.some((g: any) => !!g?.dateGranularity);
+        const hasDateBucket = structuredItems.some((g) => !!g?.dateGranularity);
         const tzRequiresInMemory = !!tz && tz !== 'UTC' && hasDateBucket;
         if (typeof drv.aggregate === 'function' && allStructuredSupported && !tzRequiresInMemory) {
             // HAVING is engine-owned (#4286): applied AFTER aggregation, over
diff --git a/packages/objectql/src/internal-fields.test.ts b/packages/objectql/src/internal-fields.test.ts
@@ -25,7 +25,7 @@
 import { describe, it, expect, beforeEach } from 'vitest';
 import { ObjectQL, type EngineReadOptions } from './engine.js';
 import { collectInternalReadFields, SECRET_MASK } from './secret-fields.js';
-import type { EngineAggregateOptions, ServiceObject } from '@objectstack/spec/data';
+import type { ServiceObject } from '@objectstack/spec/data';
 
 // ---- minimal stub driver (equality-only WHERE) ----------------------------
 // Rows leave the driver as COPIES, as a real driver's do — see the note in
@@ -366,16 +366,15 @@ describe('#7728: the `internal` field flag omits a value from the generic data p
 
     it('rejects the flagged field as a structured {field} groupBy bucket', async () => {
       await seedThree();
-      // `as unknown as` names the contract being bypassed rather than erasing
-      // it: `EngineAggregateOptions.groupBy` is declared `string[]`, while the
-      // engine reads structured `{ field, dateGranularity }` buckets too — so
-      // this is deliberately off-contract input, and the guard must walk that
-      // second spelling as well. (`as any` here would grow the #4918 ratchet.)
+      // The structured bucket form is ON-contract since #8032
+      // (`EngineAggregateOptions.groupBy` is the standard GroupByNodeSchema
+      // union) — this case type-checks honestly and pins that the guard walks
+      // the second spelling, not just the string form above.
       await expect(
         ctx.engine.aggregate('itest_api_key', {
           aggregations: [{ function: 'count', alias: 'n' }],
           groupBy: [{ field: 'key' }],
-        } as unknown as EngineAggregateOptions, SYSTEM),
+        }, SYSTEM),
       ).rejects.toThrow(/key/);
     });
 
diff --git a/packages/spec/src/data/data-engine.test.ts b/packages/spec/src/data/data-engine.test.ts
@@ -476,6 +476,53 @@ describe('EngineAggregateOptionsSchema', () => {
     expect(options.groupBy).toHaveLength(1);
     expect(options.aggregations).toHaveLength(2);
   });
+
+  // #8032 — `groupBy` is the standard GroupByNodeSchema union, the same
+  // vocabulary as `QuerySchema.groupBy`. The engine has always read the
+  // structured `{ field, dateGranularity }` bucket form (date bucketing,
+  // and the credential-aggregation guard walks it); the declaration used to
+  // say `string[]`, so every correct caller had to cast. These pin the
+  // declaration to the enforced contract, in both directions.
+  it('accepts structured { field, dateGranularity, alias } groupBy buckets (#8032)', () => {
+    const result = EngineAggregateOptionsSchema.safeParse({
+      groupBy: ['region', { field: 'closed_at', dateGranularity: 'quarter' }, { field: 'owner_id', alias: 'owner' }],
+      aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
+      timezone: 'Asia/Shanghai',
+    });
+    expect(result.success).toBe(true);
+    if (result.success) {
+      expect(result.data.groupBy).toEqual([
+        'region',
+        { field: 'closed_at', dateGranularity: 'quarter' },
+        { field: 'owner_id', alias: 'owner' },
+      ]);
+    }
+  });
+
+  it('keeps the plain-string groupBy form validating byte-identically', () => {
+    const result = EngineAggregateOptionsSchema.safeParse({
+      groupBy: ['status', 'category'],
+      aggregations: [{ function: 'count', alias: 'n' }],
+    });
+    expect(result.success).toBe(true);
+    if (result.success) expect(result.data.groupBy).toEqual(['status', 'category']);
+  });
+
+  it('rejects a groupBy bucket whose dateGranularity is not in the vocabulary', () => {
+    const result = EngineAggregateOptionsSchema.safeParse({
+      groupBy: [{ field: 'closed_at', dateGranularity: 'decade' }],
+      aggregations: [{ function: 'count', alias: 'n' }],
+    });
+    expect(result.success).toBe(false);
+  });
+
+  it('rejects a groupBy bucket that names no field', () => {
+    const result = EngineAggregateOptionsSchema.safeParse({
+      groupBy: [{ dateGranularity: 'day' }],
+      aggregations: [{ function: 'count', alias: 'n' }],
+    });
+    expect(result.success).toBe(false);
+  });
 });
 
 describe('EngineCountOptionsSchema', () => {
diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts
@@ -2,7 +2,7 @@
 
 import { z } from 'zod';
 import { FilterConditionSchema } from './filter.zod';
-import { SortNodeSchema, QuerySchema, FullTextSearchSchema, FieldNodeSchema, AggregationNodeSchema, QUERY_CURSOR_REMOVED, QUERY_DISTINCT_REMOVED } from './query.zod';
+import { SortNodeSchema, QuerySchema, FullTextSearchSchema, FieldNodeSchema, AggregationNodeSchema, GroupByNodeSchema, QUERY_CURSOR_REMOVED, QUERY_DISTINCT_REMOVED } from './query.zod';
 import { retiredKey } from '../shared/retired-key';
 import { ExecutionContextSchema } from '../kernel/execution-context.zod';
 
@@ -326,8 +326,15 @@ export const DataEngineDeleteOptionsSchema = lazySchema(() => BaseEngineOptionsS
 export const EngineAggregateOptionsSchema = lazySchema(() => BaseEngineOptionsSchema.extend({
   /** Filter conditions (WHERE) — standard QueryAST `where` */
   where: z.union([z.record(z.string(), z.unknown()), FilterConditionSchema]).optional(),
-  /** Group By fields */
-  groupBy: z.array(z.string()).optional(),
+  /**
+   * GROUP BY targets — standard {@link GroupByNodeSchema}, same as
+   * `QuerySchema.groupBy`: a bare field name, or a
+   * `{ field, dateGranularity?, alias? }` bucket object for date bucketing.
+   * The engine has always read both spellings (#8032 caught the declaration
+   * up to the enforced contract); the string form stays the canonical
+   * short-hand and validates unchanged.
+   */
+  groupBy: z.array(GroupByNodeSchema).optional().describe('GROUP BY targets (strings or `{field, dateGranularity?}` objects for date bucketing)'),
   /**
    * Aggregation definitions — uses standard AggregationNodeSchema (`function` key).
    * e.g. [{ function: 'sum', field: 'amount', alias: 'total' }]

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@objectstack/spec": minor
 +---
++
 +`EngineAggregateOptionsSchema.groupBy` now declares the standard `GroupByNodeSchema` union — a bare field name, or a `{ field, dateGranularity?, alias? }` bucket object for date bucketing — the same vocabulary `QuerySchema.groupBy` has always declared (#8032). The engine, driver-mongodb and the in-memory aggregation path have always executed the structured form; the engine-options declaration was the one face still saying `string[]`, so every correct caller had to cast around it. This widens the declared accept-set only: plain-string `groupBy` payloads validate byte-identically and no runtime behavior changes.