diff --git a/.changeset/calldata-batch-arm-retired.md b/.changeset/calldata-batch-arm-retired.md new file mode 100644 index 0000000000..bf3ec215d1 --- /dev/null +++ b/.changeset/calldata-batch-arm-retired.md @@ -0,0 +1,34 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): `callData` no longer has a `batch` arm that answers a silent, empty success (#5856) + +`callData`'s `action === 'batch'` arm returned `{ object, results: [] }` — an +HTTP 200 whose body a consumer cannot tell apart from "the batch ran and matched +nothing" — while opening no transaction and writing nothing. It was the only arm +in that function answering an unimplemented action with **success**: every other +unhandled action throws `400 Unknown data action: …`, and `aggregate` throws +`503` when the engine cannot serve it. Retry, idempotency and audit logic all +read a 200 + empty result set as one successful empty operation. + +Nothing could reach it, and that is the point: its safety lived **upstream**, in +a route table that happens not to spell `batch`, not in any guard of its own — +the ADR-0115 Evidence 5 / #4451 shape, where one route-table extension silently +turns a dormant branch into a live "successfully did nothing". Every entry point +was enumerated before removal (`/data` compares `parts[1]` against the literal +`'query'` and otherwise reads it as a record id; the MCP bridge, the actions +domain and `invokeBusinessAction` pass literals; the declarative endpoint +executor is bounded by `ApiEndpointSchema.objectParams.operation`, a closed enum +of find/get/create/update/delete; and `callData` is not part of this package's +export surface), so the arm is removed under ADR-0049 enforce-or-remove rather +than converted to a 501 nobody would ever receive. + +**Behaviour on every live path is unchanged** — no reachable request produced +that response. What changed is the answer waiting for the first caller who ever +does spell `batch`: a loud `400 Unknown data action: batch`, identical to any +other unknown action, instead of a silent success. Batching itself is untouched +and keeps its single owner: `@objectstack/rest`'s `registerBatchEndpoints` +mounts both `POST /batch` (atomic, cross-object) and `POST /data/:object/batch` +(per-object, ADR-0119) — which is exactly why a host serving only the +dispatcher reports `capabilities.transactionalBatch: false` (#5672). diff --git a/packages/runtime/src/action-execution-calldata-batch-retired.test.ts b/packages/runtime/src/action-execution-calldata-batch-retired.test.ts new file mode 100644 index 0000000000..8abb67b858 --- /dev/null +++ b/packages/runtime/src/action-execution-calldata-batch-retired.test.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5856 — `callData` has no `batch` arm, and `batch` is refused like every + * other action it does not serve. + * + * The removed code was three lines: + * + * ```ts + * if (action === 'batch') { + * // Batch operations — not yet supported via direct service dispatch + * return { object: params.object, results: [] }; + * } + * ``` + * + * It was the ONLY arm in `callData` that answered an unimplemented action with + * SUCCESS. Every other unhandled action throws `400 Unknown data action: …`, + * and `aggregate` throws `503` when the engine cannot serve it — this one + * returned an HTTP 200 whose body is shaped exactly like a batch that ran and + * matched nothing, having opened no transaction and written nothing. Retry, + * idempotency and audit all read that as one successful empty operation. + * + * ## Why deleting it changed no online behaviour — the enumeration + * + * Nothing could reach the arm, and its unreachability lived UPSTREAM of it + * (ADR-0115 Evidence 5 / #4451: "the slot exists, nobody registers it"), which + * is why removal is the fix rather than a comment. Every entry point into + * `callData`, on `main` at the time of the fix: + * + * | entry point | what it passes as `action` | + * |---|---| + * | `domains/data.ts` (`/data`) | the literals `query` / `get` / `create` / `update` / `delete`; `parts[1]` is compared against `'query'` and otherwise read as a record **id**, never as an action | + * | `domains/mcp.ts` (MCP bridge, `run_action`) | the literals `query` / `get` / `aggregate` / `create` / `update` / `delete` | + * | `domains/actions.ts` + `invokeBusinessAction` | the literal `get` | + * | `endpoint-executor.ts` (declarative endpoints, bound in `dispatcher-plugin.ts`) | one literal per `ObjectOperation`, and that type is `ApiEndpointSchema.objectParams.operation` — a CLOSED enum of find/get/create/update/delete | + * | outside this package | nothing: `callData` is not re-exported from `packages/runtime/src/index.ts` | + * + * The two structural halves of that table are pinned below (the `/data` route + * table, and the endpoint vocabulary) so a future re-wiring has to face them. + * + * ## What this suite pins + * + * 1. `batch` is refused with the SAME `{ statusCode: 400, message }` shape as + * any other unknown action — on a deployment WITH the `protocol` slot and + * on one WITHOUT it, since the removed arm sat past both paths; + * 2. the actions `callData` really serves are untouched (positive control); + * 3. the two upstream facts that made the arm unreachable. + * + * Reverse verification (direction predicted BEFORE running, then measured — + * see the PR): restoring the three lines turns case 1 RED in the ordinary + * direction — the call RESOLVES `{ object: 'task', results: [] }` instead of + * rejecting, so every `rejects` assertion in `describe('batch is refused …')` + * fails with "promise resolved instead of rejected". Cases 2 and 3 stay green + * under the restore: they describe the paths the arm never sat on, which is + * the same claim the enumeration above makes. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ApiEndpointSchema } from '@objectstack/spec/api'; + +import { callData, type ActionExecutionDeps } from './action-execution.js'; +import { HttpDispatcher, type HttpProtocolContext } from './http-dispatcher.js'; + +const EC = { userId: 'u1', isSystem: false, positions: [], permissions: [] } as any; +/** [#5155] Every service lookup resolves off the REQUEST's kernel. */ +const REQ = { request: {} } as HttpProtocolContext; +const SCHEMA = { name: 'task', fields: { title: { name: 'title', type: 'text' } } }; + +// --------------------------------------------------------------------------- +// Harnesses — the same row set behind both deployments +// --------------------------------------------------------------------------- + +function rows() { + return [{ id: 'r1', title: 'one' }]; +} + +/** The read surface both harnesses share. No write verb is defined: this suite + * never writes, and a double that declares one it does not need is a contract + * to keep in sync for nothing (`check:engine-double-contract`'s subject). */ +function engine(store = rows()) { + return { + // `registry` is what `HttpDispatcher.getObjectQLService` requires before + // it will hand the service to `callData` — not decoration. + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + find: async (_o: string, bag: any) => { + const id = bag?.where?.id; + return id == null ? [...store] : store.filter((r) => r.id === String(id)); + }, + findOne: async (_o: string, opts: any) => store.find((r) => r.id === String(opts?.where?.id)) ?? null, + } as any; +} + +/** No `protocol` slot — every verb takes `callData`'s ObjectQL fallback. */ +function fallbackHarness() { + const ql = engine(); + const services: Record = { + metadata: { getObject: async () => ({ name: 'task', fields: {} }) }, + objectql: ql, + }; + const deps: ActionExecutionDeps = { + resolveService: (async (_c: HttpProtocolContext, name: string) => services[name]) as any, + getObjectQL: async () => ql, + }; + return { deps, ql, services }; +} + +/** Protocol-first, with the REAL `@objectstack/metadata-protocol` occupant. */ +function protocolHarness() { + const ql = engine(); + const services: Record = { + metadata: { getObject: async () => ({ name: 'task', fields: {} }) }, + protocol: new ObjectStackProtocolImplementation(ql), + objectql: ql, + }; + const deps: ActionExecutionDeps = { + resolveService: (async (_c: HttpProtocolContext, name: string) => services[name]) as any, + getObjectQL: async () => ql, + }; + return { deps, ql, services }; +} + +const DEPLOYMENTS: Array<[string, () => { deps: ActionExecutionDeps }]> = [ + ['without the protocol slot (ObjectQL fallback)', fallbackHarness], + ['with the protocol slot', protocolHarness], +]; + +/** Capture a rejection as plain data so two of them can be compared. */ +async function rejection(p: Promise) { + try { + const resolved = await p; + return { rejected: false as const, resolved }; + } catch (e) { + return { rejected: true as const, error: e as any }; + } +} + +// --------------------------------------------------------------------------- +// 1. `batch` is refused, and refused like everything else unknown +// --------------------------------------------------------------------------- + +describe('batch is refused with the unknown-action answer (#5856)', () => { + it.each(DEPLOYMENTS)('%s → 400 Unknown data action: batch', async (_label, harness) => { + const { deps } = harness(); + await expect( + callData(deps, REQ, 'batch', { object: 'task' }, undefined, undefined, EC), + ).rejects.toEqual({ statusCode: 400, message: 'Unknown data action: batch' }); + }, 60_000); + + it('answers `batch` in the SAME shape as any other unknown action', async () => { + // The claim the issue is about, stated as an identity rather than as a + // literal: `batch` is no longer a special case of anything. Only the + // action name may differ between the two rejections. + const { deps } = fallbackHarness(); + const forBatch = await rejection(callData(deps, REQ, 'batch', { object: 'task' }, undefined, undefined, EC)); + const forOther = await rejection(callData(deps, REQ, 'frobnicate', { object: 'task' }, undefined, undefined, EC)); + + expect(forBatch.rejected).toBe(true); + expect(forOther.rejected).toBe(true); + expect(Object.keys(forBatch.error).sort()).toEqual(Object.keys(forOther.error).sort()); + expect(forBatch.error.statusCode).toBe(forOther.error.statusCode); + expect(forBatch.error.message.replace('batch', 'X')).toBe(forOther.error.message.replace('frobnicate', 'X')); + }, 60_000); + + it('never answers a 200 whose body reads as "the batch ran and matched nothing"', async () => { + // The was-red assertion in its narrowest form. `{ results: [] }` is + // indistinguishable from a real empty batch, which is what made this + // worse than a 501: nothing downstream can tell the two apart. + for (const [, harness] of DEPLOYMENTS) { + const outcome = await rejection( + callData(harness().deps, REQ, 'batch', { object: 'task' }, undefined, undefined, EC), + ); + expect(outcome.rejected).toBe(true); + expect(outcome).not.toMatchObject({ resolved: { results: [] } }); + } + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 2. Positive control — the actions `callData` DOES serve are untouched +// --------------------------------------------------------------------------- + +describe('the served actions still answer (positive control)', () => { + it.each(DEPLOYMENTS)('%s → query lists, get reads', async (_label, harness) => { + const { deps } = harness(); + const list: any = await callData(deps, REQ, 'query', { object: 'task', query: {} }, undefined, undefined, EC); + expect(list.object).toBe('task'); + expect(list.records).toEqual([{ id: 'r1', title: 'one' }]); + + const one: any = await callData(deps, REQ, 'get', { object: 'task', id: 'r1' }, undefined, undefined, EC); + expect(one).toMatchObject({ object: 'task', id: 'r1', record: { id: 'r1', title: 'one' } }); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 3. The two upstream facts that made the arm unreachable +// --------------------------------------------------------------------------- + +describe('nothing upstream can spell `batch` (#5856 enumeration)', () => { + it('the dispatcher’s `/data` domain declines `/data/:object/batch` — it routes only `query`', async () => { + // `handleDataRequest` compares `parts[1]` against the literal 'query' + // and otherwise reads it as a record id, so this POST matches no branch + // and the domain DECLINES it (`handled: false`). This is the upstream + // constraint the removed arm was relying on for its safety. + // + // Note what this does NOT say: `POST /data/:object/batch` is a real + // endpoint — `@objectstack/rest` mounts it (`registerBatchEndpoints`, + // `rest-server.ts`), together with the cross-object `POST /batch`. + // That is the point of route-ownership rule 1: batching has one owner, + // and a host that wants it mounts REST. What is pinned here is that + // THIS domain is not a second owner of the same path. + const h = fallbackHarness(); + const resolve = (name: string) => + name === 'objectql' ? h.ql + : name === 'metadata' ? h.services.metadata + : name === 'auth' ? { api: { getSession: async () => ({ user: { id: 'u1' } }) } } + : undefined; + const kernel: any = { getService: resolve, getServiceAsync: async (n: string) => resolve(n) }; + const dispatcher = new HttpDispatcher(kernel); + + const res: any = await dispatcher.dispatch('POST', '/data/task/batch', { operations: [] }, {}, { request: {} } as HttpProtocolContext); + expect(res.handled).toBe(false); + + // The sibling that IS routed, so the assertion above cannot pass by the + // whole domain being broken. + const served: any = await dispatcher.dispatch('POST', '/data/task/query', {}, {}, { request: {} } as HttpProtocolContext); + expect(served.handled).toBe(true); + expect(served.response.status).toBe(200); + }, 60_000); + + it('a declared endpoint cannot ask for `batch` — the operation enum is closed', () => { + // `endpoint-executor.ts`'s `ObjectOperation` is this enum, so the + // declarative-endpoint path can only ever hand `callData` one of five + // literals. Publish rejects the rest. + const declare = (operation: string) => + ApiEndpointSchema.safeParse({ + name: 'task_batch', + path: '/api/v1/apps/showcase/task', + method: 'POST', + type: 'object_operation', + target: 'task', + objectParams: { object: 'task', operation }, + }); + + expect(declare('batch').success).toBe(false); + expect(declare('create').success).toBe(true); + }); +}); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index bcf6403978..7210d56955 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -347,11 +347,28 @@ export async function callData(deps: ActionExecutionDeps, throw { statusCode: 503, message: 'Data service not available' }; } - if (action === 'batch') { - // Batch operations — not yet supported via direct service dispatch - return { object: params.object, results: [] }; - } - + // [#5856] `batch` deliberately has NO arm here. It used to answer + // `{ object, results: [] }` — an HTTP 200 whose body a consumer cannot + // tell apart from "the batch ran and matched nothing" — on a path that + // opened no transaction and wrote nothing. Its safety was never its own: + // no caller of `callData` can spell `batch` (`domains/data.ts` compares + // `parts[1]` against the literal `'query'`; `domains/mcp.ts`, + // `domains/actions.ts` and `invokeBusinessAction` pass literals; the + // declarative endpoint executor is bounded by + // `ApiEndpointSchema.objectParams.operation`, a closed enum of + // find/get/create/update/delete; and `callData` is not part of this + // package's export surface), so the arm's only live effect was to + // pre-decide — wrongly — what the FIRST caller to arrive would get: + // a silent success where every other unhandled action gets a loud + // refusal. Removed under ADR-0049 enforce-or-remove, so `batch` falls to + // the same 400 as any other unknown action. Batching itself is untouched + // and keeps its ONE owner (route-ownership rule 1): both the atomic + // cross-object `POST /batch` and the per-object `POST /data/:object/batch` + // are mounted by `@objectstack/rest`'s `registerBatchEndpoints` + // (ADR-0119) — which is exactly why this dispatcher answers + // `capabilities.transactionalBatch: false` (#5672, + // `http-dispatcher.ts`). Pinned by + // `action-execution-calldata-batch-retired.test.ts`. throw { statusCode: 400, message: `Unknown data action: ${action}` }; } diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 4337fe38b5..5df631e3cc 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -53,7 +53,18 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m const m = method.toUpperCase(); - // 1. Custom Actions (query, batch) + // 1. Custom Actions (query) + // + // [#5856] `batch` was listed here too, and was the last trace of a wiring + // that never happened: no branch below routes it, and `callData`'s + // `action === 'batch'` arm (which answered a silent `{ results: [] }`) has + // been removed with it. Batching has ONE owner and it is not this domain + // (route-ownership rule 1): `@objectstack/rest`'s `registerBatchEndpoints` + // mounts both `POST /batch` (atomic, cross-object) and `POST + // /data/:object/batch` (per-object) — which is exactly why a host serving + // only this dispatcher reports `capabilities.transactionalBatch: false` + // (#5672). Re-adding `batch` HERE would be a second implementation of a + // path REST already serves, not the missing half of one. if (parts.length > 1) { const action = parts[1]; diff --git a/packages/runtime/src/endpoint-policy.ts b/packages/runtime/src/endpoint-policy.ts index 55412c7397..edf42522f6 100644 --- a/packages/runtime/src/endpoint-policy.ts +++ b/packages/runtime/src/endpoint-policy.ts @@ -270,7 +270,23 @@ export function computeCacheControl( return `private, max-age=${Math.floor(ttl)}`; } -/** The 401 every seam on this platform answers — same code, same message, same envelope. */ +/** + * The anonymous 401 this seam answers: the same DECISION, {@link ANONYMOUS_DENY_CODE} + * and {@link ANONYMOUS_DENY_MESSAGE} as every other seam — in the **dispatcher's** + * envelope, `{ success: false, error: { code, message, httpStatus } }`, which is + * what `apiErrorResponse` builds. + * + * NOT the platform's only 401 body, and this comment used to say it was ("same + * code, same message, same envelope"). The REST seam — `@objectstack/rest`'s + * `enforceAuth`, writing `ANONYMOUS_DENY_BODY` — answers the flat + * `{ error, message }`. Both envelopes are live and sanctioned by ADR-0112's + * 2026-07-30 amendment (#4007); converging them is a breaking wire change owned + * by the envelope-convergence line (#3843 family), not by this function. The + * full two-envelope table lives on `ANONYMOUS_DENY_BODY` + * (`@objectstack/core`, `security/anonymous-deny.ts`), narrowed there by #5632 + * — this was the same claim surviving on the side that PRODUCES the wrapper, + * where it reads as authoritative (#5800). + */ function anonymousDenial(): EndpointPolicyVerdict { const { status, body } = apiErrorResponse({ code: ANONYMOUS_DENY_CODE, diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 0ccd5a300b..3e42ee1790 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -1409,8 +1409,13 @@ export class HttpDispatcher { // (`registerBatchEndpoints`) — this dispatcher has no batch // branch at all: `domains/data.ts` routes only `query` as a // custom action, and `callData`'s vestigial `action === 'batch'` - // arm is unreachable from here and returns `{ results: [] }` - // without opening a transaction. Answering `engine.transaction` + // arm — unreachable from here, and returning `{ results: [] }` + // without opening a transaction — was REMOVED under ADR-0049 + // enforce-or-remove (#5856), so `batch` now falls to the same + // `400 Unknown data action` as any other unhandled action. + // Nothing about this verdict changed with it: the reason was + // "this face does not serve `/batch`" both before and after. + // Answering `engine.transaction` // instead would advertise atomicity for an endpoint this host // does not serve — the `declared ≠ enforced` lie the flag was // introduced (#3298/#1604) to remove. A host that mounts REST