From 9f9aaffddaac3a9c33a3bebc1a56bf32b2a66ba3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 09:24:55 +0000 Subject: [PATCH 1/3] fix(runtime): consume the caller-scope load verdict at the flow and script action doors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loadActionSubjectRecord` computes one `recordLoadDenied` verdict for every action door, and exactly one door consumed it as a refusal — the declarative update. The flow door and the script/body door spread it into the context as a field and proceeded, so MCP `run_action` on a `type: 'flow'` action answered `ok: true` and started a persisted run for a `recordId` the caller cannot read, and identically for an id that names nothing at all. Both remaining doors now consume the verdict on both surfaces (REST `/actions` and the MCP `run_action` bridge) through one shared refusal, `refuseDeniedSubjectLoad`, placed before the automation run is created and before a trusted body is entered. The envelope is the shared not-found one (`RECORD_NOT_FOUND`, 404), so an unreadable row and an id that names nothing stay one answer. Record-less and new-record actions never attempt a load, so their verdict can never be `true` and their stamp behaviour is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- ...ction-door-record-load-verdict-consumed.md | 15 + .../action-door-record-load-denied.test.ts | 475 ++++++++++++++++++ packages/runtime/src/action-execution.ts | 79 ++- .../src/action-record-load-denied.test.ts | 253 +++++++--- packages/runtime/src/domains/actions.ts | 16 + 5 files changed, 755 insertions(+), 83 deletions(-) create mode 100644 .changeset/action-door-record-load-verdict-consumed.md create mode 100644 packages/runtime/src/action-door-record-load-denied.test.ts diff --git a/.changeset/action-door-record-load-verdict-consumed.md b/.changeset/action-door-record-load-verdict-consumed.md new file mode 100644 index 0000000000..5f03f1e899 --- /dev/null +++ b/.changeset/action-door-record-load-verdict-consumed.md @@ -0,0 +1,15 @@ +--- +"@objectstack/runtime": patch +--- + +An action whose caller-scope record load was DENIED is now refused at every action door, not at one of the three. + +`loadActionSubjectRecord` computes one verdict — `recordLoadDenied` — for every door, and exactly one door consumed it as a refusal: the declarative update. The flow door and the script/body door spread the same verdict into the context as a field and proceeded. So MCP `run_action` on a `type: 'flow'` action answered `ok: true` and started a persisted run for a `recordId` the caller cannot read — and, identically, for an id that names nothing at all — while `get_record` answered "not found" and `update_record` answered "no access" for that same id in the same session. Nothing in the response told the calling agent the row had not been delivered. + +Both remaining doors now consume the verdict, on both surfaces (the REST `/actions` route and the MCP `run_action` bridge), through one shared refusal: + +- **What is refused.** A row-scoped invocation whose caller-scope load was attempted and did not deliver the row. The refusal lands before the automation run is created and before a trusted, RLS/FLS-bypassing action body is entered — not after, which would answer an error with the run already persisted. +- **The envelope is the shared not-found one** — `RECORD_NOT_FOUND`, 404, the same `recordNotFoundError` the read path and the declarative door already answer. Not a 403 and not a new "denied" code: the read path collapses "filtered out by row-level security" and "this id names nothing" on purpose, so answering the two differently would make this door disclose existence where every other door declines to. +- **Record-less and new-record actions are unchanged.** The verdict can only be `true` when a load was actually attempted — a `recordId` was supplied and the action key is not object-less — so an object-less ("global") action and an invocation with no `recordId` never reach the refusal, and both still receive the `recordId` stamp on `ctx.record` exactly as before. The predicate is the load's own verdict, deliberately not the `locations`-derived `requiresRecord` of an action listing, which an author may omit entirely. + +`AutomationContext.recordLoadDenied` and the handler-side `ctx.recordLoadDenied` are untouched and still populated by the same producer; an author guard written against either keeps working. What changed is that the platform no longer depends on that guard being written. diff --git a/packages/runtime/src/action-door-record-load-denied.test.ts b/packages/runtime/src/action-door-record-load-denied.test.ts new file mode 100644 index 0000000000..0022598dde --- /dev/null +++ b/packages/runtime/src/action-door-record-load-denied.test.ts @@ -0,0 +1,475 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16370] The caller-scope load's verdict is CONSUMED at all three action + * doors, not at one of them. + * + * ## The defect + * + * `loadActionSubjectRecord` computes one verdict (`recordLoadDenied`) for every + * door. Exactly ONE door consumed it as a refusal — the declarative update + * (#15079, contract point 3). The **flow** door and the **script/body** door + * spread the same verdict into the context as a field and PROCEEDED, so MCP + * `run_action` on a `type: 'flow'` action answered + * + * { "ok": true, …, "result": { "success": true, "status": "paused", + * "runId": "run_94fcb26b-…" } } + * + * for a `recordId` the caller cannot read — and, identically, for an id that + * names nothing at all — while `get_record` answered "not found" and + * `update_record` answered "no access" for that same id in the same session. A + * persisted run existed for a row the caller had never demonstrated read access + * to, and nothing in the response told the agent the row had not been + * delivered. + * + * #15168 carried the verdict INTO the flow context and said in as many words + * that "whether the automation engine acts on it … is a separate reading". This + * file is that reading: the platform refuses, rather than delegating the + * refusal to an author-written decision node that #15168's own docs describe as + * opt-in. + * + * ## What is pinned + * + * 1. **Both doors × both surfaces.** The flow door and the script/body door, + * on the REST `/actions` route and on the MCP `run_action` bridge. A rule + * implemented at one door is the failure class #14143 and #15168 each paid + * for on this exact seam, so every case below is asserted on all four. + * 2. **⛔ No run, no body.** The refusal lands BEFORE `automation.execute` + * (no persisted run) and BEFORE `executeAction` (no trusted, RLS-bypassing + * body). `expect(...).not.toHaveBeenCalled()` is the half that makes the + * status assertion mean anything — "refused afterwards" would answer 404 + * with the run already created. + * 3. ⭐ **The door is NOT an existence oracle.** An unreadable row and an id + * that names nothing produce the SAME envelope, compared field by field + * with the id itself normalised out. This is the property that keeps the + * card p1 rather than p0, and it is the report's step 4 re-run. + * 4. **Record-less and new-record actions are byte-for-byte unchanged.** An + * object-less action key never attempts a load, so its verdict is never + * `true` and it still receives the `recordId` stamp — the regression that + * #14143 deliberately kept and that this card must not take away. + * 5. **A load that SUCCEEDS still runs.** The owner reaches the flow and the + * handler exactly as before; this is the firing control that stops every + * zero above from being a rig that dispatches nothing. + * + * ## The RLS double is faithful on the one point that matters + * + * `find` honours `options.context.userId`: the row exists and is returned to + * its owner, and is INVISIBLE to anyone else — which is how row-level security + * manifests to `callData('get', …)`, and why the real `recordNotFoundError` + * (404 `RECORD_NOT_FOUND`) is what the door then catches. The MCP cases run the + * REAL `callData`, so nothing about the refused/absent collapse is mocked away. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from './http-dispatcher.js'; +import { + callData, + invokeBusinessAction, + refuseDeniedSubjectLoad, + loadActionSubjectRecord, + GLOBAL_ACTION_OBJECT_KEY, +} from './action-execution.js'; + +const OWNER = 'usr_owner'; +const STRANGER = 'usr_stranger'; +const OBJECT = 'crm_case'; +const RECORD_ID = 'case_1'; +/** The report's step 4, verbatim: an id that names nothing at all. */ +const GHOST_ID = 'does-not-exist-0000'; +const FLOW_NAME = 'crm_case_escalate_wizard'; + +const SCRIPT_ACTION = { + name: 'close_case', + label: 'Close', + objectName: OBJECT, + type: 'script', + target: 'close_case', + ai: { exposed: true, description: 'Close a case.' }, +}; +const FLOW_ACTION = { + name: 'escalate_case', + label: 'Escalate', + objectName: OBJECT, + type: 'flow', + target: FLOW_NAME, + ai: { exposed: true, description: 'Escalate a case.' }, +}; +const OBJECT_DEF = { name: OBJECT, actions: [SCRIPT_ACTION, FLOW_ACTION] }; + +/** + * The record-LESS pair: standalone action items with no `objectName`, so both + * resolve to the object-less `GLOBAL_ACTION_OBJECT_KEY` on both surfaces. These + * are the declarations whose behaviour must not move by one byte. + */ +const GLOBAL_SCRIPT_ACTION = { + name: 'log_call', + label: 'Log call', + type: 'script', + target: 'log_call', + ai: { exposed: true, description: 'Log a call.' }, +}; +const GLOBAL_FLOW_ACTION = { + name: 'escalate_global', + label: 'Escalate (global)', + type: 'flow', + target: FLOW_NAME, + ai: { exposed: true, description: 'Escalate, globally.' }, +}; +const STANDALONE = [GLOBAL_SCRIPT_ACTION, GLOBAL_FLOW_ACTION]; + +/** The acting principal, as `resolveExecutionContext` builds one. */ +function ec(userId: string) { + return { userId, tenantId: 'org_1', positions: [], permissions: [], systemPermissions: [] }; +} + +/** + * An engine whose reads are ROW-SCOPED: `crm_case:case_1` is visible to its + * owner and to nobody else, and no id but `case_1` exists at all. A double that + * returned the row to everyone would pass while the defect was live. + */ +function makeQl() { + const executeAction = vi.fn(async (_object: string, _action: string, _ctx: any) => ({ ok: true })); + const schemaOf = (n: string) => (n === OBJECT ? OBJECT_DEF : undefined); + return { + executeAction, + getSchema: schemaOf, + registry: { getObject: schemaOf, getItem: () => undefined }, + find: vi.fn(async (object: string, options?: any) => { + if (object !== OBJECT) return []; + const caller = options?.context?.userId; + const wanted = options?.filters?.[0]?.[2] ?? options?.where?.id; + if (wanted !== undefined && wanted !== RECORD_ID) return []; + return caller === OWNER ? [{ id: RECORD_ID, status: 'open', owner_id: OWNER }] : []; + }), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + } as any; +} + +/** Records the context handed to the automation service — and whether it ran. */ +function makeAutomation() { + return { + execute: vi.fn(async (_flow: string, _context?: any) => ({ + success: true, status: 'paused', runId: 'run_test', durationMs: 1, + })), + getFlow: vi.fn(async (name: string) => (name === FLOW_NAME ? { name } : null)), + }; +} + +/** The metadata service both doors read: objects, plus the standalone pair. */ +function makeMetadata() { + const standaloneByName = new Map(STANDALONE.map((a) => [a.name, a])); + const synth = (type: string, name: string) => + (type === 'action' ? (standaloneByName.get(name) ?? null) : null); + return { + load: vi.fn(async (type: string, name: string) => synth(type, name)), + loadDiagnosed: vi.fn(async (type: string, name: string) => ({ + data: synth(type, name), degraded: false, errors: [], + })), + loadMany: vi.fn(async (type: string) => (type === 'action' ? STANDALONE : [])), + listObjects: vi.fn(async () => [OBJECT_DEF]), + getObject: vi.fn(async (n: string) => (n === OBJECT ? OBJECT_DEF : undefined)), + }; +} + +/** REST — `POST /actions/`, through the real dispatcher. */ +async function rest(userId: string, path: string) { + const ql = makeQl(); + const automation = makeAutomation(); + const metadata = makeMetadata(); + const resolve = (n: string) => + n === 'objectql' || n === 'data' ? ql + : n === 'metadata' ? metadata + : n === 'automation' ? automation + : null; + const kernel: any = { + getService: resolve, + getServiceAsync: async (n: string) => resolve(n), + context: { getService: resolve }, + }; + const context: any = { request: {}, environmentId: 'platform', executionContext: ec(userId) }; + const res: any = await (new HttpDispatcher(kernel) as any).handleActions(path, 'POST', {}, context); + return { + response: res.response, + automation, + ql, + actionCtx: ql.executeAction.mock.calls[0]?.[2], + flowCtx: automation.execute.mock.calls[0]?.[1], + }; +} + +/** + * MCP — `run_action`, wired to the REAL `callData`, so the row-scoped read and + * its 404 are the ones production runs rather than a hand-thrown stand-in. + */ +async function mcp(userId: string, name: string, input: Record) { + const ql = makeQl(); + const automation = makeAutomation(); + const metadata = makeMetadata(); + const deps: any = { + resolveService: async (_ctx: any, service: string) => (service === 'automation' ? automation : undefined), + getObjectQL: async () => ql, + }; + const requestContext: any = { request: {}, environmentId: 'platform' }; + const run = () => invokeBusinessAction(deps, requestContext, name, input as any, { + driver: undefined, + envId: 'platform', + ec: ec(userId), + getMeta: () => metadata, + callData: (action, params, dataDriver, scopeId, execCtx) => + callData(deps, requestContext, action, params, dataDriver, scopeId, execCtx), + }); + return { run, automation, ql }; +} + +/** The refusal envelope, with the id normalised out so two ids compare equal. */ +function envelopeOf(status: number, code: unknown, message: unknown, id: string) { + return { status, code, message: String(message).split(id).join('') }; +} + +// ─────────────────────────────────────────────────────────────────────────── +// The FLOW door — the one the card measured +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#16370] flow door — a denied caller-scope load is refused before the run exists', () => { + it('REST: a caller who cannot read the row gets 404 RECORD_NOT_FOUND and NO run is created', async () => { + const { response, automation } = await rest(STRANGER, `/${OBJECT}/escalate_case/${RECORD_ID}`); + + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(response.body.error.message).toContain(RECORD_ID); + expect(response.body.error.message).toContain(OBJECT); + // ⭐ The half that makes the status mean something: refusing AFTER the + // dispatch would answer 404 with a persisted run already created. + expect(automation.execute).not.toHaveBeenCalled(); + }); + + it('MCP: the same caller is thrown at, never answered `ok: true`, and NO run is created', async () => { + const { run, automation } = await mcp(STRANGER, FLOW_ACTION.name, { recordId: RECORD_ID }); + + await expect(run()).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(automation.execute).not.toHaveBeenCalled(); + }); + + it('MCP: an id that names NOTHING is refused identically — the report\'s step 4', async () => { + // Driven as the row's OWNER on purpose: the only thing wrong here is + // the id, so a door that leaked existence would have to answer this + // differently from the case above. + const { run, automation } = await mcp(OWNER, FLOW_ACTION.name, { recordId: GHOST_ID }); + + await expect(run()).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(automation.execute).not.toHaveBeenCalled(); + }); + + it('REST: an id that names NOTHING is refused identically', async () => { + const { response, automation } = await rest(OWNER, `/${OBJECT}/escalate_case/${GHOST_ID}`); + + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(automation.execute).not.toHaveBeenCalled(); + }); + + it('a load that SUCCEEDS still starts the run — the firing control for every zero above', async () => { + const { response, automation, flowCtx } = await rest(OWNER, `/${OBJECT}/escalate_case/${RECORD_ID}`); + + expect(response.status).toBe(200); + expect(automation.execute).toHaveBeenCalledTimes(1); + expect(automation.execute.mock.calls[0][0]).toBe(FLOW_NAME); + expect(flowCtx.record).toMatchObject({ id: RECORD_ID, status: 'open', owner_id: OWNER }); + // Unchanged by this card: the verdict key is ABSENT, not `false`. + expect('recordLoadDenied' in flowCtx).toBe(false); + + const viaMcp = await mcp(OWNER, FLOW_ACTION.name, { recordId: RECORD_ID }); + const result: any = await viaMcp.run(); + expect(result.ok).toBe(true); + expect(viaMcp.automation.execute).toHaveBeenCalledTimes(1); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The SCRIPT/BODY door — the identical second hole, fixed in the same PR +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#16370] script/body door — the same verdict, the same refusal', () => { + it('REST: a caller who cannot read the row gets 404 and NO trusted body is entered', async () => { + const { response, ql } = await rest(STRANGER, `/${OBJECT}/close_case/${RECORD_ID}`); + + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + // ⛔ The body runs ELEVATED (`isSystem: true`, #3914) — RLS/FLS + // bypassing. It must not be entered at all for a caller who has not + // demonstrated read access to its subject row. + expect(ql.executeAction).not.toHaveBeenCalled(); + }); + + it('MCP: the same caller is thrown at and NO trusted body is entered', async () => { + const { run, ql } = await mcp(STRANGER, SCRIPT_ACTION.name, { recordId: RECORD_ID }); + + await expect(run()).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(ql.executeAction).not.toHaveBeenCalled(); + }); + + it('an id that names NOTHING is refused identically, on both surfaces', async () => { + const viaRest = await rest(OWNER, `/${OBJECT}/close_case/${GHOST_ID}`); + expect(viaRest.response.status).toBe(404); + expect(viaRest.response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(viaRest.ql.executeAction).not.toHaveBeenCalled(); + + const viaMcp = await mcp(OWNER, SCRIPT_ACTION.name, { recordId: GHOST_ID }); + await expect(viaMcp.run()).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(viaMcp.ql.executeAction).not.toHaveBeenCalled(); + }); + + it('a load that SUCCEEDS still reaches the handler with the real row', async () => { + const { response, actionCtx } = await rest(OWNER, `/${OBJECT}/close_case/${RECORD_ID}`); + + expect(response.status).toBe(200); + expect(actionCtx.record).toMatchObject({ id: RECORD_ID, status: 'open', owner_id: OWNER }); + expect('recordLoadDenied' in actionCtx).toBe(false); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// ⭐ Not an existence oracle — the property the p1 grading rests on +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#16370] the refusal discloses nothing — an unreadable row and a ghost id are ONE answer', () => { + it('flow door: the two refusals are the same envelope, field for field', async () => { + const denied = await rest(STRANGER, `/${OBJECT}/escalate_case/${RECORD_ID}`); + const ghost = await rest(OWNER, `/${OBJECT}/escalate_case/${GHOST_ID}`); + + expect(envelopeOf(ghost.response.status, ghost.response.body.error.code, + ghost.response.body.error.message, GHOST_ID)) + .toEqual(envelopeOf(denied.response.status, denied.response.body.error.code, + denied.response.body.error.message, RECORD_ID)); + // ⛔ And neither of them created a run. + expect(denied.automation.execute).not.toHaveBeenCalled(); + expect(ghost.automation.execute).not.toHaveBeenCalled(); + }); + + it('script door: the two refusals are the same envelope, field for field', async () => { + const denied = await rest(STRANGER, `/${OBJECT}/close_case/${RECORD_ID}`); + const ghost = await rest(OWNER, `/${OBJECT}/close_case/${GHOST_ID}`); + + expect(envelopeOf(ghost.response.status, ghost.response.body.error.code, + ghost.response.body.error.message, GHOST_ID)) + .toEqual(envelopeOf(denied.response.status, denied.response.body.error.code, + denied.response.body.error.message, RECORD_ID)); + }); + + it('MCP: the two thrown refusals carry the same code and status', async () => { + const deniedErr = await mcp(STRANGER, FLOW_ACTION.name, { recordId: RECORD_ID }) + .then(({ run }) => run().then(() => null, (e: any) => e)); + const ghostErr = await mcp(OWNER, FLOW_ACTION.name, { recordId: GHOST_ID }) + .then(({ run }) => run().then(() => null, (e: any) => e)); + + // Read as a SET so a collapse to one answer reddens here whatever that + // one answer is — including the collapse to "both succeeded". + expect([ + deniedErr && { code: deniedErr.code, status: deniedErr.status }, + ghostErr && { code: ghostErr.code, status: ghostErr.status }, + ]).toEqual([ + { code: 'RECORD_NOT_FOUND', status: 404 }, + { code: 'RECORD_NOT_FOUND', status: 404 }, + ]); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The boundary — record-less and new-record actions, byte for byte +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#16370] record-less and new-record actions keep the stamp behaviour unchanged', () => { + it('REST: an object-less SCRIPT action invoked WITH a recordId still dispatches, stamp intact', async () => { + const { response, actionCtx, ql } = await rest(STRANGER, `/global/log_call/${RECORD_ID}`); + + expect(response.status).toBe(200); + expect(ql.executeAction).toHaveBeenCalledTimes(1); + // ⛔ The prohibition this case exists for: the stamp is still there and + // the verdict was never `true`, because no load was attempted at all. + expect(actionCtx.record).toEqual({ id: RECORD_ID }); + expect('recordLoadDenied' in actionCtx).toBe(false); + expect(ql.find.mock.calls.filter((c: any[]) => c[0] === OBJECT)).toHaveLength(0); + }); + + it('REST: an object-less FLOW action invoked WITH a recordId still starts its run', async () => { + const { response, automation, flowCtx } = await rest(STRANGER, `/global/escalate_global/${RECORD_ID}`); + + expect(response.status).toBe(200); + expect(automation.execute).toHaveBeenCalledTimes(1); + expect(flowCtx.record).toEqual({ id: RECORD_ID }); + expect('recordLoadDenied' in flowCtx).toBe(false); + // An object-less key carries no `object` on the flow context. + expect('object' in flowCtx).toBe(false); + }); + + it('MCP: an object-less action invoked WITH a recordId still dispatches, stamp intact', async () => { + const { run, ql } = await mcp(STRANGER, GLOBAL_SCRIPT_ACTION.name, { recordId: RECORD_ID }); + + const result: any = await run(); + expect(result.ok).toBe(true); + expect(ql.executeAction).toHaveBeenCalledTimes(1); + expect(ql.executeAction.mock.calls[0][2].record).toEqual({ id: RECORD_ID }); + }); + + it('MCP: a new-record invocation (no recordId at all) is untouched — no load, no refusal', async () => { + const { run, ql } = await mcp(STRANGER, SCRIPT_ACTION.name, {}); + + const result: any = await run(); + expect(result.ok).toBe(true); + expect(ql.executeAction.mock.calls[0][2].record).toEqual({}); + expect(ql.find.mock.calls.filter((c: any[]) => c[0] === OBJECT)).toHaveLength(0); + }); + + it('REST: a new-record invocation (no recordId in the path) is untouched', async () => { + const { response, actionCtx, ql } = await rest(STRANGER, `/${OBJECT}/close_case`); + + expect(response.status).toBe(200); + expect(actionCtx.record).toEqual({}); + expect('recordLoadDenied' in actionCtx).toBe(false); + expect(ql.find.mock.calls.filter((c: any[]) => c[0] === OBJECT)).toHaveLength(0); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The shared refusal itself — ONE implementation, three doors +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#16370] refuseDeniedSubjectLoad — the rule, isolated from every door', () => { + it('throws the shared not-found envelope when the verdict is `true`', () => { + let thrown: any; + try { + refuseDeniedSubjectLoad(OBJECT, RECORD_ID, { record: { id: RECORD_ID }, recordLoadDenied: true }); + } catch (e) { thrown = e; } + + expect(thrown).toBeDefined(); + expect(thrown.code).toBe('RECORD_NOT_FOUND'); + expect(thrown.status).toBe(404); + expect(thrown.message).toBe(`Record ${RECORD_ID} not found in ${OBJECT}`); + }); + + it('returns silently when the verdict is `false` — and the stamp is NOT the predicate', () => { + // ⛔ `record.id` is truthy in BOTH cases; re-deriving the verdict from + // it is the #14143 defect verbatim, so this pair is what says the + // implementation reads the flag and nothing else. + expect(() => refuseDeniedSubjectLoad(OBJECT, RECORD_ID, + { record: { id: RECORD_ID }, recordLoadDenied: false })).not.toThrow(); + expect(() => refuseDeniedSubjectLoad(OBJECT, RECORD_ID, + { record: { id: RECORD_ID }, recordLoadDenied: true })).toThrow(); + }); + + it('is unreachable for the two carved-out shapes, straight off the producer', async () => { + // Record-less: an object-less key never attempts a load… + const recordLess = await loadActionSubjectRecord(GLOBAL_ACTION_OBJECT_KEY, RECORD_ID, + async () => { throw new Error('must not be called'); }); + expect(recordLess.recordLoadDenied).toBe(false); + expect(() => refuseDeniedSubjectLoad(GLOBAL_ACTION_OBJECT_KEY, RECORD_ID, recordLess)).not.toThrow(); + + // …and neither does a new-record invocation. Both keep their stamp + // behaviour because the verdict they carry can never be `true`. + const newRecord = await loadActionSubjectRecord(OBJECT, undefined, + async () => { throw new Error('must not be called'); }); + expect(newRecord.recordLoadDenied).toBe(false); + expect(() => refuseDeniedSubjectLoad(OBJECT, undefined, newRecord)).not.toThrow(); + }); +}); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index b947296b0c..bcf429e0fd 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1454,6 +1454,63 @@ export function actionRecordLoadSignal(load: ActionSubjectRecordLoad): { recordL return load.recordLoadDenied ? { recordLoadDenied: true } : {}; } +/** + * [#16370] Contract point 3, as the ONE function all three action doors call. + * + * ## What was wrong + * + * {@link loadActionSubjectRecord} computes the caller-scope load's verdict for + * every door, and exactly ONE door consumed it as a refusal — the declarative + * update (#15079). The flow door and the script/body door spread the same + * verdict into the context as a field and PROCEEDED: MCP `run_action` on a + * `type: 'flow'` action answered `ok: true` and started a persisted run for a + * `recordId` the caller cannot read — and for an id that names nothing at all — + * while `get_record` answered "not found" and `update_record` answered "no + * access" for that same id, in the same session. + * + * #15168 made the verdict AVAILABLE to an author's opt-in guard, which is not + * the same thing as the platform enforcing it, and its own note said so: the + * reading it left open ("whether the automation engine acts on it … is a + * separate reading") is this function. A swallowed load must never become an + * implicit grant — the rule is #15079's, and a rule implemented at one of three + * doors is the failure class #14143 and #15168 each already paid for here. + * + * ## The predicate is the LOAD's verdict — ⛔ never the action's `locations` + * + * `recordLoadDenied` is `true` exactly when a caller-scope load was ATTEMPTED + * and did not deliver the row: a `recordId` was supplied AND the action key is + * not object-less. So the two shapes that must not change are excluded BY + * CONSTRUCTION rather than by a second test that could drift out of step with + * the producer — a **record-less** action (object-less key) and a **new-record** + * action (no `recordId`) never attempt a load, so their verdict is never `true` + * and their stamp behaviour is byte-for-byte what it was. + * + * ⛔ NOT {@link summarizeAction}'s `requiresRecord`, which is derived from + * `locations` — optional metadata an author may omit entirely, which is exactly + * why contract point 7 refuses to be left to it. An authorization refusal keyed + * on optional metadata is absent for precisely the authors who declared least. + * + * ⚠️ The refusal is the SHARED not-found envelope (`recordNotFoundError`, 404 + * `RECORD_NOT_FOUND`) — ⛔ never a 403, ⛔ never a new "denied" code. The read + * path collapses "filtered out by RLS" and "this id names nothing" on purpose + * (existence non-disclosure), nothing in the caught error separates them, and a + * refusal that distinguished the two would make this door the existence oracle + * every other door declines to be. #15079 argued it; this consumes the argument + * rather than reopening it. + */ +export function refuseDeniedSubjectLoad( + objectName: string, + recordId: string | undefined, + subject: ActionSubjectRecordLoad, +): void { + if (!subject.recordLoadDenied) return; + // The producer never attempts a load without a `recordId`, so the verdict + // cannot be `true` without one. The `??` names the id in the envelope; it is + // ⛔ NOT a second condition — a guard whose extra clause can switch it off is + // the inert-guard shape this whole seam exists to remove. + throw recordNotFoundError(objectName, recordId ?? ''); +} + /** * [#15079] The prior/next value pair a successful declarative update hands back * when the action declares `undoable: true` — the anchor that key never had. @@ -1651,9 +1708,11 @@ export async function executeDeclarativeUpdateAction( } // ── contract point 3: the caller-scope load's VERDICT, consumed ────────── - if (subject.recordLoadDenied) { - throw recordNotFoundError(objectName, recordId); - } + // [#16370] Through {@link refuseDeniedSubjectLoad}, the ONE implementation + // the flow door and the script/body door now call too. Behaviour here is + // unchanged to the byte — what changed is that this is no longer the only + // door that reads the verdict, so the rule cannot be inert two doors over. + refuseDeniedSubjectLoad(objectName, recordId, subject); // ── contract point 2: ONE data-plane update, AS THE CALLER ─────────────── // ⛔ `ec`, never `buildActionExecutionContext(ec)`. See the docblock — this @@ -1812,6 +1871,20 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, return { ok: true, action: action.name, objectName, ...(recordId ? { recordId } : {}), result }; } + // ── contract point 3: the caller-scope load's VERDICT, consumed ────────── + // [#16370] The flow door and the script/body door, on the SAME rule the + // declarative door has enforced since #15079 — one implementation, three + // doors. Placed HERE deliberately: + // + // - AFTER the declarative branch above, whose own points 7 and 4 answer a + // LOCATED 400 before point 3 ever runs; hoisting the refusal over them + // would re-label those prescriptions as a 404; + // - BEFORE the identity resolution and everything below it, so no + // persisted automation run exists and no trusted body has been entered + // when the refusal lands. `ok: true` on a row the caller cannot read is + // the defect; refusing after the run was created would only describe it. + refuseDeniedSubjectLoad(objectName, recordId, subject); + // [#5372] One shared producer for the user shape (`security/actor-user.ts`), // the same one the REST `/actions` route and the AI routes use. What stood // here was `name: ec.userName ?? ec.userDisplayName ?? ec.userId` — a `??` diff --git a/packages/runtime/src/action-record-load-denied.test.ts b/packages/runtime/src/action-record-load-denied.test.ts index 0f1738b8e5..8ccc962af3 100644 --- a/packages/runtime/src/action-record-load-denied.test.ts +++ b/packages/runtime/src/action-record-load-denied.test.ts @@ -53,6 +53,21 @@ * read the context the automation service is actually handed, through BOTH * doors, and pin that the two agree. * + * ## [#16370] What this file pins CHANGED at the doors + * + * The verdict is now CONSUMED by all three doors (`refuseDeniedSubjectLoad`) + * rather than forwarded by two of them: a row-scoped flow or script action + * invoked with a `recordId` whose caller-scope load was denied is refused with + * the shared not-found envelope (404 `RECORD_NOT_FOUND`) BEFORE a run is + * created and BEFORE a trusted body is entered. So the four door-level "denied" + * cases below assert the REFUSAL, and #15168's flow-context assertions moved + * onto `dispatchFlowAction` itself, where a denied subject is still + * constructible. Everything else is untouched — the producer, the stamp, the + * absence convention, the sandbox face, and the record-less / new-record + * carve-outs. The full refusal matrix (both doors × both surfaces × denied · + * nonexistent id · record-less · a load that succeeds) lives in + * `action-door-record-load-denied.test.ts`. + * * ## The RLS double is faithful on the one point that matters * * `find` here honours `options.context.userId`: the row exists and is returned @@ -71,6 +86,7 @@ import { HttpDispatcher } from './http-dispatcher.js'; import { callData, invokeBusinessAction, + dispatchFlowAction, loadActionSubjectRecord, actionRecordLoadSignal, GLOBAL_ACTION_OBJECT_KEY, @@ -175,23 +191,22 @@ async function dispatchMcp(userId: string, ql: any, input: Record { - it('a caller who CANNOT read the row reaches the handler with recordLoadDenied === true', async () => { +describe('#14143 — REST /actions and the caller-scope load verdict', () => { + it('a caller who CANNOT read the row never reaches the handler — the door refuses', async () => { const ql = makeQl(); - const { actionCtx } = await dispatchRest(STRANGER, ql); - - expect(actionCtx).toBeDefined(); - expect(actionCtx.recordLoadDenied).toBe(true); - - // ⛔ The stamp is NOT removed — a record-less action depends on it, and - // this is the coincidence that made the natural guard useless: the id - // is here whether or not the caller can see the row, which is why the - // flag above (and not `record.id`) is the authorization predicate. - expect(actionCtx.record.id).toBe(RECORD_ID); - expect(Boolean(actionCtx.record?.id)).toBe(true); - // …and nothing of the row itself leaked to a caller who cannot read it. - expect(actionCtx.record.status).toBeUndefined(); - expect(actionCtx.record.owner_id).toBeUndefined(); + const { response, actionCtx } = await dispatchRest(STRANGER, ql); + + // [#16370] SUPERSEDED AT THE DOOR — re-pinned, ⛔ not deleted. This case + // used to assert that the handler was reached carrying + // `recordLoadDenied === true`. The door now CONSUMES that verdict + // (`refuseDeniedSubjectLoad`) instead of forwarding it, so the handler is + // never entered at all: the same guarantee, moved from a guard an author + // had to remember to write into the platform. The full refusal matrix + // lives in `action-door-record-load-denied.test.ts`. + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(actionCtx).toBeUndefined(); + expect(ql.executeAction).not.toHaveBeenCalled(); }); it('the row OWNER reaches the handler with the real row and no flag at all', async () => { @@ -217,14 +232,16 @@ describe('#14143 — REST /actions tells a handler its caller-scope load was ref }); }); -describe('#14143 — MCP run_action emits the SAME signal as the REST door', () => { - it('a caller who CANNOT read the row reaches the handler with recordLoadDenied === true', async () => { +describe('#14143 — MCP run_action answers the SAME way as the REST door', () => { + it('a caller who CANNOT read the row never reaches the handler — the door refuses', async () => { const ql = makeQl(); - const { actionCtx } = await dispatchMcp(STRANGER, ql); - expect(actionCtx.recordLoadDenied).toBe(true); - expect(actionCtx.record.id).toBe(RECORD_ID); // stamp preserved - expect(actionCtx.record.status).toBeUndefined(); + // [#16370] Superseded at the door, exactly as on the REST side above — + // and asserted here too, because a refusal on one door and a pass on the + // other is the divergence this whole file exists to prevent. + await expect(dispatchMcp(STRANGER, ql)) + .rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(ql.executeAction).not.toHaveBeenCalled(); }); it('the row OWNER reaches the handler with the real row and no flag at all', async () => { @@ -374,8 +391,14 @@ async function dispatchRestFlow(userId: string, ql: any, path = `/crm_case/escal } /** MCP — `run_action` on the same flow action, through the REAL `callData`. */ -async function dispatchMcpFlow(userId: string, ql: any, input: Record = { recordId: RECORD_ID }) { - const automation = makeAutomation(); +async function dispatchMcpFlow( + userId: string, + ql: any, + input: Record = { recordId: RECORD_ID }, + // [#16370] Injectable so a case that expects the door to THROW can still + // assert on the double afterwards — a rejected call returns nothing. + automation: ReturnType = makeAutomation(), +) { const deps: any = { resolveService: async (_ctx: any, name: string) => (name === 'automation' ? automation : undefined), getObjectQL: async () => ql, @@ -393,31 +416,19 @@ async function dispatchMcpFlow(userId: string, ql: any, input: Record { - it('a caller who CANNOT read the row starts the flow with ctx.recordLoadDenied === true', async () => { +describe('[#15168] the FLOW door and its verdict — REST /actions', () => { + it('a caller who CANNOT read the row never starts the flow — the door refuses', async () => { const ql = makeQl(); - const { automation, flowCtx } = await dispatchRestFlow(STRANGER, ql); - - expect(automation.execute).toHaveBeenCalledTimes(1); - expect(automation.execute.mock.calls[0][0]).toBe(FLOW_NAME); - expect(flowCtx).toBeDefined(); - // The contract's own predicate, verbatim (`AutomationContext`): a flow - // reads `recordLoadDenied === true`, never a truthiness of `false`. - expect(flowCtx.recordLoadDenied).toBe(true); - - // ⛔ Sibling of `record`, never a key ON it — a flow node reading - // `{{record.recordLoadDenied}}` must find nothing, or the signal would - // arrive as a phantom field of the subject row. - expect('recordLoadDenied' in flowCtx.record).toBe(false); - - // The stamp survives here exactly as it does on the handler face — it - // is why `record.id` cannot be the authorization predicate. - expect(flowCtx.record.id).toBe(RECORD_ID); - expect(flowCtx.record.status).toBeUndefined(); - expect(flowCtx.record.owner_id).toBeUndefined(); - // The rest of the envelope is untouched by this card. - expect(flowCtx.object).toBe(OBJECT_DEF.name); - expect(flowCtx.userId).toBe(STRANGER); + const { response, automation } = await dispatchRestFlow(STRANGER, ql); + + // [#16370] Superseded at the door. `AutomationContext.recordLoadDenied` + // is still DERIVED and still spread — pinned directly on + // `dispatchFlowAction` in the describe below, where it is reachable — + // but this door no longer hands the dispatcher a denied subject, so no + // persisted run is created for a row the caller cannot read. + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(automation.execute).not.toHaveBeenCalled(); }); it('the row OWNER starts the flow with the key ABSENT — not `false`', async () => { @@ -442,16 +453,17 @@ describe('[#15168] the FLOW face receives the signal — REST /actions', () => { }); }); -describe('[#15168] the FLOW face receives the signal — MCP run_action', () => { - it('a caller who CANNOT read the row starts the flow with ctx.recordLoadDenied === true', async () => { +describe('[#15168] the FLOW door and its verdict — MCP run_action', () => { + it('a caller who CANNOT read the row never starts the flow — the door refuses', async () => { const ql = makeQl(); - const { automation, flowCtx } = await dispatchMcpFlow(STRANGER, ql); - - expect(automation.execute).toHaveBeenCalledTimes(1); - expect(flowCtx.recordLoadDenied).toBe(true); - expect('recordLoadDenied' in flowCtx.record).toBe(false); - expect(flowCtx.record.id).toBe(RECORD_ID); // stamp preserved - expect(flowCtx.record.status).toBeUndefined(); + const automation = makeAutomation(); + + // [#16370] The MCP half of the same supersession. `ok: true` with a + // `runId` for a row the caller cannot read is the reported defect + // verbatim; the door throws the shared not-found envelope instead. + await expect(dispatchMcpFlow(STRANGER, ql, { recordId: RECORD_ID }, automation)) + .rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(automation.execute).not.toHaveBeenCalled(); }); it('the row OWNER starts the flow with the key ABSENT — not `false`', async () => { @@ -472,6 +484,76 @@ describe('[#15168] the FLOW face receives the signal — MCP run_action', () => }); }); +/** + * [#15168] The verdict is still DERIVED and still spread onto the flow context + * — pinned where it is reachable. + * + * [#16370] closed both doors ahead of `dispatchFlowAction`, so no door hands it + * a denied subject any more. That does NOT make #15168's contract optional: + * `AutomationContext.recordLoadDenied` is a declared spec key + * (`contracts/automation-service.ts`, pinned in `packages/spec`), the + * dispatcher is its ONE populator, and a populator that quietly stopped + * populating would be exactly the inert-signal shape #14143 was filed for. So + * the assertions #15168 wrote at the doors are re-pinned HERE, on the + * dispatcher itself, where a denied subject can still be constructed. + * + * ⚠️ Stated plainly, because it is the honest reading and not a comfortable + * one: through the two doors this repo ships, the `true` arm of the signal is + * now unreachable — the platform refuses first. The key stays declared and + * populated for any future caller of this dispatcher that legitimately does not + * refuse; surfacing the verdict to an MCP caller is a separate card. + */ +describe('[#15168] dispatchFlowAction derives the verdict from the subject load', () => { + const flowDeps = (automation: any): any => ({ + resolveService: async (_ctx: any, name: string) => (name === 'automation' ? automation : undefined), + }); + + it('a denied subject reaches the automation context as recordLoadDenied === true', async () => { + const automation = makeAutomation(); + const subject = await loadActionSubjectRecord(OBJECT_DEF.name, RECORD_ID, async () => { + throw Object.assign(new Error('Record case_1 not found in crm_case'), { + code: 'RECORD_NOT_FOUND', status: 404, + }); + }); + + await dispatchFlowAction(flowDeps(automation), { request: {}, environmentId: 'platform' } as any, FLOW_ACTION, { + objectName: OBJECT_DEF.name, subject, params: {}, recordId: RECORD_ID, ec: ec(STRANGER), envId: 'platform', + }); + + const flowCtx = flowContextOf(automation); + // The contract's own predicate, verbatim (`AutomationContext`): a flow + // reads `recordLoadDenied === true`, never a truthiness of `false`. + expect(flowCtx.recordLoadDenied).toBe(true); + // ⛔ Sibling of `record`, never a key ON it — a flow node reading + // `{{record.recordLoadDenied}}` must find nothing, or the signal would + // arrive as a phantom field of the subject row. + expect('recordLoadDenied' in flowCtx.record).toBe(false); + // The stamp survives, which is why `record.id` cannot be the predicate. + expect(flowCtx.record.id).toBe(RECORD_ID); + expect(flowCtx.record.status).toBeUndefined(); + expect(flowCtx.object).toBe(OBJECT_DEF.name); + expect(flowCtx.userId).toBe(STRANGER); + }); + + it('a delivered row reaches it with the key ABSENT — not `false`', async () => { + const automation = makeAutomation(); + const row = { id: RECORD_ID, status: 'open', owner_id: OWNER }; + const subject = await loadActionSubjectRecord(OBJECT_DEF.name, RECORD_ID, async () => ({ record: row })); + + await dispatchFlowAction(flowDeps(automation), { request: {}, environmentId: 'platform' } as any, FLOW_ACTION, { + objectName: OBJECT_DEF.name, subject, params: {}, recordId: RECORD_ID, ec: ec(OWNER), envId: 'platform', + }); + + const flowCtx = flowContextOf(automation); + expect(flowCtx.record).toMatchObject(row); + // The assertion that catches the most likely wrong implementation — + // spreading `{ recordLoadDenied: false }`. Its firing positive control is + // the case above: same rig, same expectation shape, reports `true`. + expect('recordLoadDenied' in flowCtx).toBe(false); + expect(flowCtx.recordLoadDenied).toBeUndefined(); + }); +}); + /** * [#15168] The convergence itself. A per-door assertion is satisfied by two * copies of a rule, and two copies drifting apart is the defect #14143 was @@ -480,32 +562,43 @@ describe('[#15168] the FLOW face receives the signal — MCP run_action', () => * signal is compared as a set. */ describe('[#15168] the two flow doors agree — the same caller, the same row, the same signal', () => { - it('both doors deny for the stranger and both stay silent for the owner', async () => { - const deniedRest = (await dispatchRestFlow(STRANGER, makeQl())).flowCtx; - const deniedMcp = (await dispatchMcpFlow(STRANGER, makeQl())).flowCtx; - const okRest = (await dispatchRestFlow(OWNER, makeQl())).flowCtx; - const okMcp = (await dispatchMcpFlow(OWNER, makeQl())).flowCtx; - - // Read as a SET: a collapse to one answer on both doors reddens here - // whatever that one answer is. + it('both doors REFUSE for the stranger and both run for the owner', async () => { + const deniedRest = await dispatchRestFlow(STRANGER, makeQl()); + const deniedMcpAutomation = makeAutomation(); + const deniedMcpErr = await dispatchMcpFlow(STRANGER, makeQl(), { recordId: RECORD_ID }, deniedMcpAutomation) + .then(() => null, (e: any) => e); + const okRest = await dispatchRestFlow(OWNER, makeQl()); + const okMcp = await dispatchMcpFlow(OWNER, makeQl()); + + // [#16370] Read as a SET, exactly as before: a collapse to one answer on + // both doors reddens here whatever that one answer is. What changed is + // which answer the denied halves carry — a refusal, not a started run. + expect([ + deniedRest.response.status, + deniedMcpErr?.status, + ]).toEqual([404, 404]); + expect([ + deniedRest.response.body.error.code, + deniedMcpErr?.code, + ]).toEqual(['RECORD_NOT_FOUND', 'RECORD_NOT_FOUND']); + + // ⛔ Neither denied door created a run… expect([ - deniedRest.recordLoadDenied, - deniedMcp.recordLoadDenied, - okRest.recordLoadDenied, - okMcp.recordLoadDenied, - ]).toEqual([true, true, undefined, undefined]); + deniedRest.automation.execute.mock.calls.length, + deniedMcpAutomation.execute.mock.calls.length, + ]).toEqual([0, 0]); + // …and both owner doors did, with the verdict key ABSENT — the firing + // control that stops the two zeros above from being a rig that + // dispatches nothing. expect([ - 'recordLoadDenied' in deniedRest, - 'recordLoadDenied' in deniedMcp, - 'recordLoadDenied' in okRest, - 'recordLoadDenied' in okMcp, - ]).toEqual([true, true, false, false]); - - // And the stamp is present on all four, which is what makes the flag — - // not `record.id` — the only usable predicate on either door. + okRest.automation.execute.mock.calls.length, + okMcp.automation.execute.mock.calls.length, + ]).toEqual([1, 1]); expect([ - deniedRest.record.id, deniedMcp.record.id, okRest.record.id, okMcp.record.id, - ]).toEqual([RECORD_ID, RECORD_ID, RECORD_ID, RECORD_ID]); + 'recordLoadDenied' in okRest.flowCtx, + 'recordLoadDenied' in okMcp.flowCtx, + ]).toEqual([false, false]); + expect([okRest.flowCtx.record.id, okMcp.flowCtx.record.id]).toEqual([RECORD_ID, RECORD_ID]); }); }); diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index e58cba1e56..dd3bf99854 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -696,6 +696,22 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string return { handled: true, response: deps.success(result) }; } + // ── contract point 3: the caller-scope load's VERDICT, consumed ────── + // [#16370] The same shared refusal the MCP `run_action` bridge calls at + // the same point in its own sequence — ONE implementation of the rule + // #15079 wrote into the declarative executor, now read by the flow door + // and the script/body door as well. A signal only one of three doors + // consumed is an authorization rule silently inert on the other two, + // which is the #14143 / #15168 failure class on this exact seam. + // + // Inside the `try`, like the declarative branch above, so the 404 takes + // the ONE catch this door already has and is served with its `.status` / + // `.code` intact. Ahead of `actionContext` on purpose: the trusted-mode + // audit line and the RLS/FLS-bypassing `ctx.engine` / `ctx.api` are + // built below, and none of them may exist for a caller who has not + // demonstrated read access to the subject row. + actionExec.refuseDeniedSubjectLoad(objectName, recordId, subject); + const actionContext: any = { record, // [#14143] The caller-scope load's verdict — see From 8c2af9a326a278f267f9daca66ba9b96eb850ade Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 09:56:47 +0000 Subject: [PATCH 2/3] test(runtime): re-point the action fixtures the door refusal moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine assertions across six files dispatched a row-scoped action with a `recordId` against a double that answered every read with `[]`, so the caller-scope subject load never delivered and the door now refuses. Four rigs answer the by-id pre-load instead — their subject is the elevated `ctx.api` binding, the mounted route's auth gate, handler-key addressing and flow dispatch, none of which is the load. Two fixtures pinned the branch this card closes and are re-pinned: a degraded engine with no `find` now fails closed on a row-scoped call (404, never a 500) and still runs the record-less one, and the flow route's "seeds recordId even when the record never loaded" case is split into the refusal it now is plus the new-record invocation it conflated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- .../runtime/src/action-body-identity.test.ts | 8 +++- .../runtime/src/action-ctx-user-shape.test.ts | 38 +++++++++++++++++-- ...-plugin.anonymous-gate.integration.test.ts | 7 +++- ...atcher.actions-identity-addressing.test.ts | 8 +++- ...p-dispatcher.actions-type-dispatch.test.ts | 38 ++++++++++++++++--- packages/runtime/src/http-dispatcher.test.ts | 6 ++- 6 files changed, 92 insertions(+), 13 deletions(-) diff --git a/packages/runtime/src/action-body-identity.test.ts b/packages/runtime/src/action-body-identity.test.ts index f489280233..f1d18223a0 100644 --- a/packages/runtime/src/action-body-identity.test.ts +++ b/packages/runtime/src/action-body-identity.test.ts @@ -59,7 +59,13 @@ function makeSharingEngine(extra: Record = {}) { }, async find(object: string, options?: any) { gate('find', object, options?.context); - return []; + // [#16370] The by-id pre-load has to be ANSWERED: an action door now + // refuses a row-scoped invocation whose caller-scope subject load did + // not deliver the row, so a rig that answered every read with `[]` + // would collect a 404 before the handler this file is about is ever + // built. Every other read still reads empty — the WRITES above are + // this file's subject, and they are untouched. + return options?.where?.id ? [{ id: options.where.id }] : []; }, async count(object: string, options?: any) { gate('count', object, options?.context); diff --git a/packages/runtime/src/action-ctx-user-shape.test.ts b/packages/runtime/src/action-ctx-user-shape.test.ts index 826146ab96..767fcd6cca 100644 --- a/packages/runtime/src/action-ctx-user-shape.test.ts +++ b/packages/runtime/src/action-ctx-user-shape.test.ts @@ -102,6 +102,22 @@ async function dispatchRest(ec: any, ql: any, context?: HttpProtocolContext) { return { response: res.response, actionCtx: ql.executeAction.mock.calls[0]?.[2] }; } +/** + * REST — the same action with NO `recordId`, so no subject load is attempted. + * [#16370] The row-scoped door refuses a load that did not deliver, so a case + * whose subject is something OTHER than the load reaches the handler here. + */ +async function dispatchRestNoRecord(ec: any, ql: any, context?: HttpProtocolContext) { + const kernel: any = { + context: { getService: (n: string) => (n === 'objectql' || n === 'data' ? ql : null) }, + }; + const ctx = context ?? ({ request: {}, environmentId: 'platform', executionContext: ec } as any); + const res: any = await new HttpDispatcher(kernel).handleActions( + '/crm_case/close_case', 'POST', {}, ctx, + ); + return { response: res.response, actionCtx: ql.executeAction.mock.calls[0]?.[2] }; +} + /** MCP — `run_action`. Returns the body ctx. */ async function dispatchMcp(ec: any, ql: any) { const deps: any = { resolveService: async () => null, getObjectQL: async () => ql }; @@ -247,14 +263,30 @@ describe('#5372 — the FAILURE MODE: an unresolvable name is quiet', () => { it('an engine with no `find` at all does not break the dispatch', async () => { const ql = makeQl(DEV_ADMIN); delete (ql as any).find; - // The record pre-load needs `find` too, so this also proves the name - // resolution is not what turns a degraded engine into a 500. - const { response, actionCtx } = await dispatchRest(makeEc(), ql); + // No `recordId`, so no subject load is attempted and the ONE degraded + // read left is the name resolution — which is this case's subject: an + // unresolvable name falls back to the id and the action still runs. + const { response, actionCtx } = await dispatchRestNoRecord(makeEc(), ql); expect(response.status).toBe(200); expect(actionCtx.user.name).toBe('usr_admin'); }); + it('[#16370] …and a ROW-SCOPED call on that engine fails CLOSED, not with a 500', async () => { + const ql = makeQl(DEV_ADMIN); + delete (ql as any).find; + // The subject pre-load needs `find` too, so on a degraded engine it + // cannot deliver the row. Since #16370 the door consumes that verdict: + // the refusal is the shared not-found envelope, ⛔ never a 500 and ⛔ + // never a dispatch onto a row nobody read. The case above is the firing + // control that says this 404 is the LOAD's, not the name resolution's. + const { response } = await dispatchRest(makeEc(), ql); + + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(ql.executeAction).not.toHaveBeenCalled(); + }); + it('the read is system-elevated — resolving WHO the caller is cannot depend on their own grants', async () => { const ql = makeQl(DEV_ADMIN); await dispatchRest(makeEc(), ql); diff --git a/packages/runtime/src/dispatcher-plugin.anonymous-gate.integration.test.ts b/packages/runtime/src/dispatcher-plugin.anonymous-gate.integration.test.ts index 37d8e82aac..264d679878 100644 --- a/packages/runtime/src/dispatcher-plugin.anonymous-gate.integration.test.ts +++ b/packages/runtime/src/dispatcher-plugin.anonymous-gate.integration.test.ts @@ -66,7 +66,12 @@ function servicesPlugin(): Plugin { executeAction, getSchema: (n: string) => (n === objectDef.name ? objectDef : undefined), registry: { getObject: (n: string) => (n === objectDef.name ? objectDef : undefined), getItem: () => undefined }, - find: async () => [], + // [#16370] Answer the by-id subject pre-load: the door refuses a + // row-scoped invocation whose caller-scope load did not deliver, + // and this file's subject is the AUTH gate on the mounted route — + // an unrelated 404 would hide exactly the 200 it has to prove. + find: async (_object: string, options?: any) => + (options?.where?.id ? [{ id: options.where.id }] : []), insert: async () => ({}), update: async () => ({}), delete: async () => ({}), }); ctx.registerService('automation', { diff --git a/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts b/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts index 8360eb09ea..7457fccbbd 100644 --- a/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts @@ -94,7 +94,10 @@ describe('REST /actions — identity is `name`, the handler key is derived (ADR- // `content/docs/ui/actions.mdx` teaches exactly this URL. Before D2 it // 404ed: the route used `complete_task` as the registry key, but the // handler lives under `completeTask`. - const { dispatcher, calls } = makeDispatcher(); + // [#16370] `record` is seeded because the body names a `recordId`: the + // door refuses a row-scoped invocation whose subject load did not + // deliver, and this case is about ADDRESSING, not about the load. + const { dispatcher, calls } = makeDispatcher({ record: { id: 'task_1' } }); const res = await dispatcher.handleActions( '/todo_task/complete_task', 'POST', { recordId: 'task_1' }, ctxFor(), @@ -123,8 +126,11 @@ describe('REST /actions — identity is `name`, the handler key is derived (ADR- it('runs the gated action for a caller who holds the capability', async () => { const gated = { ...targetBoundAction, requiredPermissions: ['task.manage'] }; + // [#16370] Same reason as above — the capability gate is this case's + // subject, and it must be reached with a deliverable subject row. const { dispatcher } = makeDispatcher({ objectDef: { name: 'todo_task', actions: [gated] }, + record: { id: 'task_1' }, }); const res = await dispatcher.handleActions( diff --git a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts index 6892de69fe..a05f8180e3 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -194,19 +194,45 @@ describe('REST /actions — flow dispatch (#3915)', () => { expect((execute.mock.calls[0]?.[1] as any).params.recordId).toBe('explicit_override'); }); - it('seeds `recordId` from the URL even when the record never loaded', async () => { - // New-record / unreadable-record invocations pass an empty record; the - // flow still needs the id the caller named. + it('[#16370] a ROW-SCOPED flow whose record never loaded is REFUSED — no run is created', async () => { + // ⚠️ This case used to assert the opposite: that the id was seeded and + // the flow started anyway ("new-record / unreadable-record invocations + // pass an empty record"). That conflated two shapes. A NEW-RECORD + // invocation names no `recordId` and still attempts no load — untouched + // below. An UNREADABLE one names a row the caller's own scope did not + // deliver, and starting a persisted run on it is the defect #16370 was + // filed for: MCP `run_action` answered `ok: true` with a `runId` for a + // row `get_record` calls not-found in the same session. const execute = vi.fn(async () => ({ success: true })); const { dispatcher } = makeDispatcher({ objectDef: { name: 'crm_lead', actions: [flowAction] }, automation: { execute }, - // no `record` → the best-effort load returns nothing + // no `record` → the caller-scope load delivers nothing }); - await dispatcher.handleActions('/crm_lead/convert_lead/lead_404', 'POST', {}, ctxFor()); + const res = await dispatcher.handleActions('/crm_lead/convert_lead/lead_404', 'POST', {}, ctxFor()); - expect((execute.mock.calls[0]?.[1] as any).params.recordId).toBe('lead_404'); + expect(res.response.status).toBe(404); + expect(res.response.body.error.code).toBe('RECORD_NOT_FOUND'); + // ⛔ The half that makes the status mean anything. + expect(execute).not.toHaveBeenCalled(); + }); + + it('a NEW-RECORD invocation (no recordId) still dispatches — nothing to seed, nothing to refuse', async () => { + // The other half of the shape the case above used to conflate: no id + // was named, so no load is attempted, so there is no verdict to consume + // and the flow starts exactly as it always did. + const execute = vi.fn(async () => ({ success: true })); + const { dispatcher } = makeDispatcher({ + objectDef: { name: 'crm_lead', actions: [flowAction] }, + automation: { execute }, + }); + + const res = await dispatcher.handleActions('/crm_lead/convert_lead', 'POST', {}, ctxFor()); + + expect(res.response.status).toBe(200); + expect(execute).toHaveBeenCalledTimes(1); + expect((execute.mock.calls[0]?.[1] as any).params.recordId).toBeUndefined(); }); it('forwards the caller identity so a `runAs: user` flow enforces RLS as the invoker', async () => { diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 3380acba73..5406bcc922 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -4410,7 +4410,11 @@ describe('HttpDispatcher — MCP action bridge (list_actions / run_action)', () const ql: any = { executeAction: vi.fn(), registry: { getObject: () => flowObject }, - find: vi.fn(async () => []), + // [#16370] Answer the by-id subject pre-load — the bridge refuses a + // row-scoped invocation whose caller-scope load did not deliver the row, + // and these cases are about the flow DISPATCH, not about the load. + find: vi.fn(async (_object: string, options?: any) => + (options?.where?.id ? [{ id: options.where.id }] : [])), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), From 2aaad3e517fcd37631507c16dcc0bf0880fb434d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 10:22:29 +0000 Subject: [PATCH 3/3] test(runtime): bind the two new dispatch responses through a local, so the type-check debt ledger does not grow Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- .../src/http-dispatcher.actions-type-dispatch.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts index a05f8180e3..63e0014131 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -211,9 +211,10 @@ describe('REST /actions — flow dispatch (#3915)', () => { }); const res = await dispatcher.handleActions('/crm_lead/convert_lead/lead_404', 'POST', {}, ctxFor()); + const response: any = res.response; - expect(res.response.status).toBe(404); - expect(res.response.body.error.code).toBe('RECORD_NOT_FOUND'); + expect(response.status).toBe(404); + expect(response.body.error.code).toBe('RECORD_NOT_FOUND'); // ⛔ The half that makes the status mean anything. expect(execute).not.toHaveBeenCalled(); }); @@ -229,8 +230,9 @@ describe('REST /actions — flow dispatch (#3915)', () => { }); const res = await dispatcher.handleActions('/crm_lead/convert_lead', 'POST', {}, ctxFor()); + const response: any = res.response; - expect(res.response.status).toBe(200); + expect(response.status).toBe(200); expect(execute).toHaveBeenCalledTimes(1); expect((execute.mock.calls[0]?.[1] as any).params.recordId).toBeUndefined(); });