fix(mcp)!: an aggregate_records aggregation entry carries its filter to the engine and refuses an undeclared key by name

.changeset/28550-mcp-aggregate-entry-filter.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/mcp': patch
+---
+
+fix(mcp)!: an `aggregate_records` aggregation entry carries its `filter` to the engine and refuses an undeclared key by name (#28550)
+
+Clause-②: yes (narrowing) — the aggregation entry now accepts `filter` (a widening of the tool's input), and refuses any other undeclared entry key, which it stripped and ignored before (a narrowing).
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No schema key of the metadata protocol is added, removed or re-shaped, and no export or type changes: the MCP tool's input closes an entry it already documented, and the key it now accepts is the engine's own `AggregationNodeSchema.filter`. An undeclared key used to be dropped without effect, so there is nothing for a migration to rewrite; the refusal names the key and, for `where` / `filters` / `conditions` / `as`, the spelling to send. -->
+
+**BREAKING** accept-set narrowing at the MCP `aggregate_records` tool (`@objectstack/mcp`), on both the HTTP and the stdio transport.
+
+- **Before.** Each aggregation entry was a non-strict `{ function, field, alias }`. A per-aggregation `filter` was stripped before the engine was asked, so `{"function":"count","alias":"n","filter":{"status":"active"}}` beside an unfiltered count answered the total twice, while the REST query door answered the filtered count for the same body. Any other unknown entry key was dropped the same way, with no error.
+- **Now.** The entry is built from the engine's `AggregationNodeSchema` members (`function`, `field`, `alias`, `filter`) and is closed. The `filter` is forwarded unchanged to `engine.aggregate`, the path REST uses, so both doors give one answer, including the security middleware's 403 `PERMISSION_DENIED` for a filter on a field the caller cannot read. An undeclared entry key (the retired `distinct` included) is refused by name before anything runs, as an undeclared top-level key already was.
+- **Unchanged.** Entries without a filter answer as before; the REST query door is untouched.
+- **Who sees it.** An MCP client that sent an extra key inside an aggregation entry: the call was answered with that key ignored, and it is now refused naming the key.
+
+**The remedy.** Drop the key, or for a per-aggregation condition send it as the entry's `filter`.

packages/mcp/src/mcp-http-tools.aggregation-entry.test.ts

@@ -0,0 +1,182 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `aggregate_records`: each aggregation entry carries its `filter` to the
+ * engine and refuses an undeclared key by name (#28550).
+ *
+ * THE DEFECT. The entry was a plain `z.object({ function, field, alias })`, so
+ * zod's `.strip` default deleted a per-aggregation `filter` before the handler
+ * ran. `{ function: 'count', alias: 'n', filter: { status: 'active' } }` beside
+ * an unfiltered `count` answered the same number twice (5 and 5 on the stock
+ * showcase), while the REST query door answered 2 and 5 for the same body. An
+ * unknown entry key (`bogusKey`) was dropped the same way. The top-level
+ * closure (#16913) never reached inside the array.
+ *
+ * WHAT IS PINNED HERE, at the tool layer over a recording bridge: the entry's
+ * `filter` reaches the bridge byte-for-byte (the bridge hands `aggregations` to
+ * `engine.aggregate`, which applies it, so nothing in this package evaluates
+ * it); a malformed filter gets the engine node's own refusal at the door; an
+ * undeclared entry key is refused by name before the bridge is asked; and the
+ * advertised JSON Schema declares both facts. The MCP-equals-REST answer over a
+ * real engine, and the field-permission refusal, are pinned end to end in
+ * `packages/qa/dogfood/test/showcase-mcp-aggregate-filter.dogfood.test.ts`.
+ *
+ * CONTROLS. Entries without a filter reach the bridge exactly as before, and
+ * the top-level unknown-key posture is unchanged.
+ */
+
+import { describe, it, expect, beforeEach } from 'vitest';
+
+import { MCPServerRuntime } from './mcp-server-runtime.js';
+import type { McpDataBridge } from './mcp-http-tools.js';
+
+interface RecordingBridge extends McpDataBridge {
+  calls: any[];
+}
+
+function makeBridge(): RecordingBridge {
+  const calls: any[] = [];
+  return {
+    calls,
+    async listObjects() { return [{ name: 'showcase_project' }]; },
+    async describeObject(name: string) { return { name, fields: [] }; },
+    async query(object: string) { return { object, records: [] }; },
+    async get() { return null; },
+    async create(object: string) { return { object, id: 'n1' }; },
+    async update(object: string, id: string) { return { object, id }; },
+    async remove(object: string, id: string) { return { object, id, success: true }; },
+    async aggregate(object: string, opts: any) {
+      calls.push([object, opts]);
+      return [{ n: 2, all: 5 }];
+    },
+  };
+}
+
+let nextId = 1;
+
+async function rpc(runtime: MCPServerRuntime, bridge: RecordingBridge, method: string, params?: unknown) {
+  const body = { jsonrpc: '2.0', id: nextId++, method, ...(params === undefined ? {} : { params }) };
+  const req = new Request('http://localhost/api/v1/mcp', {
+    method: 'POST',
+    headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' },
+    body: JSON.stringify(body),
+  });
+  const res = await runtime.handleHttpRequest(req, { bridge, parsedBody: body });
+  return (await res.json()) as any;
+}
+
+async function aggregate(runtime: MCPServerRuntime, bridge: RecordingBridge, args: Record<string, unknown>) {
+  const json = await rpc(runtime, bridge, 'tools/call', { name: 'aggregate_records', arguments: args });
+  const text = json.result?.content?.map((c: any) => c.text).join('\n') ?? json.error?.message ?? '';
+  return { refused: json.result?.isError === true || Boolean(json.error), text: String(text) };
+}
+
+describe('aggregate_records — the aggregation entry (#28550)', () => {
+  let runtime: MCPServerRuntime;
+  let bridge: RecordingBridge;
+
+  beforeEach(() => {
+    runtime = new MCPServerRuntime({ name: 't', version: '1.0.0' });
+    bridge = makeBridge();
+  });
+
+  it('the reported call: an entry `filter` reaches the bridge unchanged, beside an unfiltered sibling', async () => {
+    const aggregations = [
+      { function: 'count', alias: 'n', filter: { status: 'active' } },
+      { function: 'count', alias: 'all' },
+    ];
+    const { refused, text } = await aggregate(runtime, bridge, { objectName: 'showcase_project', aggregations });
+    expect(refused, text).toBe(false);
+    expect(bridge.calls).toHaveLength(1);
+    expect(bridge.calls[0][0]).toBe('showcase_project');
+    // Exactly what was sent: the filter on the first entry, none on the second.
+    expect(bridge.calls[0][1].aggregations).toEqual(aggregations);
+  });
+
+  it('an operator filter reaches the bridge as sent', async () => {
+    const aggregations = [
+      { function: 'sum', field: 'budget', alias: 'big', filter: { budget: { $gte: 1000 }, status: { $in: ['active', 'planning'] } } },
+    ];
+    const { refused, text } = await aggregate(runtime, bridge, {
+      objectName: 'showcase_project',
+      aggregations,
+      groupBy: ['status'],
+      where: { owner: 'u1' },
+    });
+    expect(refused, text).toBe(false);
+    expect(bridge.calls[0][1]).toMatchObject({ aggregations, groupBy: ['status'], where: { owner: 'u1' } });
+  });
+
+  it('CONTROL — entries without a filter reach the bridge exactly as before', async () => {
+    const aggregations = [
+      { function: 'count', alias: 'n' },
+      { function: 'sum', field: 'budget', alias: 'total' },
+    ];
+    const { refused, text } = await aggregate(runtime, bridge, { objectName: 'showcase_project', aggregations });
+    expect(refused, text).toBe(false);
+    expect(bridge.calls[0][1].aggregations).toEqual(aggregations);
+  });
+
+  it('an undeclared entry key is refused by name, and the bridge is never asked', async () => {
+    const { refused, text } = await aggregate(runtime, bridge, {
+      objectName: 'showcase_project',
+      aggregations: [{ function: 'count', alias: 'n', bogusKey: 1 }],
+    });
+    expect(refused).toBe(true);
+    expect(text).toContain('bogusKey');
+    expect(bridge.calls).toHaveLength(0);
+  });
+
+  it('an entry `where` is refused, and the refusal names `filter` as the spelling to send', async () => {
+    const { refused, text } = await aggregate(runtime, bridge, {
+      objectName: 'showcase_project',
+      aggregations: [{ function: 'count', alias: 'n', where: { status: 'active' } }],
+    });
+    expect(refused).toBe(true);
+    expect(text).toContain('where');
+    expect(text).toContain('filter');
+    expect(bridge.calls).toHaveLength(0);
+  });
+
+  it('the retired `distinct` entry key is refused, not dropped', async () => {
+    const { refused, text } = await aggregate(runtime, bridge, {
+      objectName: 'showcase_project',
+      aggregations: [{ function: 'sum', field: 'budget', alias: 's', distinct: true }],
+    });
+    expect(refused).toBe(true);
+    expect(text).toContain('distinct');
+    expect(bridge.calls).toHaveLength(0);
+  });
+
+  it('a filter that is not a filter object is refused at the door by the engine node\'s own shape', async () => {
+    for (const filter of ["status = 'active'", ['status', '=', 'active'], 7]) {
+      const { refused } = await aggregate(runtime, bridge, {
+        objectName: 'showcase_project',
+        aggregations: [{ function: 'count', alias: 'n', filter }],
+      });
+      expect(refused, JSON.stringify(filter)).toBe(true);
+    }
+    expect(bridge.calls).toHaveLength(0);
+  });
+
+  it('CONTROL — an undeclared TOP-LEVEL key is still refused, naming the declared spelling', async () => {
+    const { refused, text } = await aggregate(runtime, bridge, {
+      objectName: 'showcase_project',
+      metrics: [{ function: 'count', alias: 'n' }],
+    });
+    expect(refused).toBe(true);
+    expect(text).toContain('metrics');
+    expect(text).toContain('aggregations');
+    expect(bridge.calls).toHaveLength(0);
+  });
+
+  it('`tools/list` advertises the entry as closed, with `filter` declared', async () => {
+    const json = await rpc(runtime, bridge, 'tools/list');
+    const tool = (json.result?.tools ?? []).find((t: any) => t.name === 'aggregate_records');
+    expect(tool).toBeDefined();
+    const entry = tool.inputSchema?.properties?.aggregations?.items;
+    expect(entry?.additionalProperties).toBe(false);
+    expect(Object.keys(entry?.properties ?? {}).sort()).toEqual(['alias', 'field', 'filter', 'function']);
+    expect(entry?.required).toEqual(expect.arrayContaining(['function', 'alias']));
+  });
+});

packages/mcp/src/mcp-http-tools.ts

@@ -39,6 +39,10 @@
 import { z } from 'zod';
 import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import type { EngineAggregateOptions } from '@objectstack/spec/data';
+// [#28550] The engine's aggregation entry. Each `aggregate_records` entry is
+// built from ITS members, so this door and the REST query door (which parses
+// `FindDataRequestSchema`, whose `aggregations` are this node) judge one shape.
+import { AggregationNodeSchema } from '@objectstack/spec/data';
 // [#23529] The catalogue's spelling of `PERMISSION_DENIED`, the code whose
 // `details` `errorResultFromThrown` withholds. Imported, never hand-spelled.
 import { StandardErrorCode } from '@objectstack/spec/api';
@@ -689,6 +693,56 @@ function strictToolInput<T extends z.ZodRawShape>(
     .strict();
 }
 
+/**
+ * [#28550] One `aggregate_records` aggregation entry: closed, and carrying the
+ * per-aggregation `filter`.
+ *
+ * The entry used to be a plain `z.object({ function, field, alias })`. Zod's
+ * default for an object is `.strip`, and {@link strictToolInput} closes only the
+ * tool's TOP-LEVEL keys, so an entry's `filter` was deleted before the handler
+ * ran: `{ function: 'count', alias: 'n', filter: { status: 'active' } }` answered
+ * the unconditional count, beside the REST query door answering the filtered
+ * one for the same body. Any other unknown entry key vanished the same way.
+ *
+ * Both halves of the fix are the entry's shape, nothing else:
+ *
+ * - **`filter` is declared**, and it is the engine's own member,
+ *   `AggregationNodeSchema.shape.filter` (`FilterConditionSchema`), the slot the
+ *   REST door parses. The handler forwards `aggregations` to the bridge
+ *   unchanged, and every bridge hands them to `engine.aggregate`, which applies
+ *   the filter and runs it through the same refusal doors as `where` (shape,
+ *   unknown field, and the security middleware's field-read check). ⛔ No filter
+ *   is evaluated here: a second evaluator in this package would be a second
+ *   answer to the same question.
+ * - **The entry is strict**, through {@link strictToolInput}, so an undeclared
+ *   entry key is refused by name, as an undeclared top-level key already is.
+ *   The aliases are semantic near-misses for `filter` and `alias`; they are
+ *   named in the refusal and never parsed.
+ *
+ * `function`, `field` and `alias` are the engine's members too. Each keeps the
+ * description this tool advertised before; `.describe` copies the schema and
+ * leaves its parse unchanged.
+ */
+function aggregationEntrySchema() {
+  const node = AggregationNodeSchema.shape;
+  return strictToolInput(
+    {
+      surface: 'this aggregate_records aggregation entry',
+      aliases: { where: 'filter', filters: 'filter', conditions: 'filter', as: 'alias' },
+    },
+    {
+      function: node.function.describe('Aggregation function'),
+      field: node.field.describe('Field to aggregate (omit for count(*))'),
+      alias: node.alias.describe('Result column name'),
+      filter: node.filter.describe(
+        'Optional per-aggregation filter, e.g. {"status":"active"}: only rows matching it feed THIS '
+          + 'aggregation, while the other aggregations in the call still read every row. Same operators '
+          + 'as where.',
+      ),
+    },
+  );
+}
+
 /**
  * The spellings every object-scoped tool shares, split out because they are the
  * same guess wherever an object name is taken.
@@ -1179,17 +1233,12 @@ export function registerObjectTools(
             {
             objectName: z.string().describe('The object/table name'),
             aggregations: z
-              .array(
-                z.object({
-                  function: z
-                    .enum(['count', 'sum', 'avg', 'min', 'max', 'count_distinct'])
-                    .describe('Aggregation function'),
-                  field: z.string().optional().describe('Field to aggregate (omit for count(*))'),
-                  alias: z.string().describe('Result column name'),
-                }),
-              )
+              .array(aggregationEntrySchema())
               .min(1)
-              .describe('Metrics to compute, e.g. [{"function":"sum","field":"amount","alias":"total"}]'),
+              .describe(
+                'Metrics to compute, e.g. [{"function":"sum","field":"amount","alias":"total"},'
+                  + '{"function":"count","alias":"open_count","filter":{"status":"open"}}]',
+              ),
             groupBy: z
               .array(
                 z.union([