From 108db0f7086aafd162c0b7d6e380796b73c4b06d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 11:49:34 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(runtime):=20retire=20callData's=20`batc?= =?UTF-8?q?h`=20arm=20=E2=80=94=20the=20one=20unimplemented=20action=20tha?= =?UTF-8?q?t=20answered=20success=20(#5856)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `callData`'s `action === 'batch'` arm returned `{ object, results: [] }`: an HTTP 200 a consumer cannot tell apart from "the batch ran and matched nothing", with no transaction opened and nothing written. It was the only arm in that function answering an unimplemented action with success — every other unhandled action throws `400 Unknown data action: …`, `aggregate` throws `503`. Its safety lived upstream, in a route table that happens not to spell `batch`, not in any guard of its own. Every entry point was enumerated before removal (`/data` compares `parts[1]` against the literal 'query'; the MCP bridge, the actions domain and `invokeBusinessAction` pass literals; the declarative endpoint executor is bounded by ApiEndpointSchema.objectParams.operation, a closed enum; `callData` is not exported from this package), so the arm is removed under ADR-0049 enforce-or-remove rather than converted to a 501 nobody would ever receive. `domains/data.ts`'s `// Custom Actions (query, batch)` comment — the last trace of a wiring that never happened — goes with it, and `http-dispatcher.ts`'s #5672 capability comment is updated to describe the code that now exists. No reachable request produced that response, so no online behaviour changes. Batching keeps its single owner: `@objectstack/rest`'s `registerBatchEndpoints` serves both `POST /batch` and `POST /data/:object/batch` (ADR-0119). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Wbxm29qPKnLf44AbSxizqW --- .changeset/calldata-batch-arm-retired.md | 34 +++ ...n-execution-calldata-batch-retired.test.ts | 248 ++++++++++++++++++ packages/runtime/src/action-execution.ts | 27 +- packages/runtime/src/domains/data.ts | 13 +- packages/runtime/src/http-dispatcher.ts | 9 +- 5 files changed, 323 insertions(+), 8 deletions(-) create mode 100644 .changeset/calldata-batch-arm-retired.md create mode 100644 packages/runtime/src/action-execution-calldata-batch-retired.test.ts 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/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 From a1a0bde77bb0e6967e9d0130112cefe02e713a3d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 12:04:42 +0000 Subject: [PATCH 2/2] docs(runtime): endpoint-policy's anonymous 401 comment names the dispatcher envelope, not "the platform's" (#5800) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `anonymousDenial()`'s docstring claimed "same code, same message, same envelope". The first two hold; the third does not: `apiErrorResponse` builds the dispatcher wrapper `{ success: false, error: { code, message, httpStatus } }`, while the REST seam (`@objectstack/rest` `enforceAuth` → `ANONYMOUS_DENY_BODY`) answers the flat `{ error, message }`. Two live, sanctioned envelopes per ADR-0112's 2026-07-30 amendment (#4007). This is the same false claim #5632 narrowed on `ANONYMOUS_DENY_BODY`, surviving on the side that PRODUCES the wrapper — where a reader (especially an AI author) takes it as authoritative. Comment only; the wire body is untouched, and the two-envelope table stays single-sourced in `security/anonymous-deny.ts`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Wbxm29qPKnLf44AbSxizqW --- packages/runtime/src/endpoint-policy.ts | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) 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,