diff --git a/.changeset/24268-declarative-update-undeclared-params-refused.md b/.changeset/24268-declarative-update-undeclared-params-refused.md new file mode 100644 index 0000000000..1ea65f9304 --- /dev/null +++ b/.changeset/24268-declarative-update-undeclared-params-refused.md @@ -0,0 +1,21 @@ +--- +'@objectstack/runtime': minor +--- + +fix(runtime)!: an `operation: 'update'` action that declares no `params` refuses every undeclared wire param at both action doors, instead of writing it to the row + +Clause-②: no (narrowing) + + + +**BREAKING** (the REST `/actions` door and the MCP `run_action` door now refuse, with a 400 `VALIDATION_ERROR`, a wire param that a declarative update action without `params` does not declare; measured consumers: objectstack-ai/hotcrm's `complete_task` called with such params; any console vendored before objectui `fb501bc894`, which posts the action's `patch` keys as `params`; and page action rows (`action:button` / `action:icon` with `operation: 'update'`) whose static object `params` the console forwards to a param-less declarative update action, none of which exist in this repo, with the row surface tracked in #24394), shipped as `minor` under the launch-window convention for breaking changes. + +A declarative update (`operation: 'update'` + `patch`) has no handler: the route merges the request's `params` over `patch` and writes the result to the current record. An action that declared no `params` skipped the ADR-0104 D2 param check altogether, so any key the caller sent was written too. An action declaring `patch: { status: 'completed' }` could rewrite any other field the caller may write, which is not what its author declared. + +Now an empty or absent `params` list on a declarative update is an empty accept set. Every key that is neither a declared param nor a dispatcher built-in (`recordId`, `objectName`, `_selectedIds`) is refused before anything is read or written, through the same D2 path a declared-params action already takes: the same `Invalid action params: Unknown action param "KEY"` message, the same 400, and the same `OS_ALLOW_LAX_ACTION_PARAMS=1` escape hatch. The action still writes its `patch` when the request carries no `params` or an empty bag, and a declared param still writes and still wins over `patch`. + +Unchanged: an action with a handler (`target`, `body`, or a flow) that declares no `params` keeps its pass-through. + +Also at the MCP `run_action` door: every ADR-0104 D2 param refusal, and the declarative update's own refusals (no `recordId`, or nothing to write), now carry `code: 'VALIDATION_ERROR'` and `status: 400`, so the tool error renders the coded envelope the REST door already answers with, rather than the message alone. + +What to do: declare the param on the action if the request's value should be written, or stop sending it, because the route applies `patch` from the declaration and never needs it on the wire. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 5e9042fcfc..2e548b906a 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -1193,7 +1193,8 @@ is consumed: nothing is applied, the run stays parked, and the corrected submission still lands. Three shapes declare no contract and so keep the pass-through — the same way an -action with no `params` is untouched: +action with no `params` is untouched, unless it is a declarative +`operation: 'update'`, whose empty list is an empty accept set: - an **object-form** screen (`config.objectName`), whose flat `fields` list is empty by construction; the client persists the record through the normal write diff --git a/packages/runtime/src/action-declarative-update.test.ts b/packages/runtime/src/action-declarative-update.test.ts index e082757749..7196b7bbc6 100644 --- a/packages/runtime/src/action-declarative-update.test.ts +++ b/packages/runtime/src/action-declarative-update.test.ts @@ -87,6 +87,15 @@ const TRIAGE_CASE = { ai: { exposed: true, description: 'Triage this case.' }, }; +/** `params` only, no `patch` — the shape whose empty bag is point 7's "nothing to write". */ +const NOTE_CASE = { + name: 'note_case', + label: 'Note', + operation: 'update', + params: [{ name: 'note', type: 'text' }], + ai: { exposed: true, description: 'Write a note on this case.' }, +}; + /** The REVERSE pin's subject: a handler-less `type: 'script'` action with NO `operation`. */ const HANDLER_ONLY = { name: 'nudge_case', @@ -103,7 +112,7 @@ const OBJECT_DEF = { priority: { type: 'text' }, note: { type: 'text' }, }, - actions: [CLOSE_CASE, TRIAGE_CASE, HANDLER_ONLY], + actions: [CLOSE_CASE, TRIAGE_CASE, NOTE_CASE, HANDLER_ONLY], }; /** The acting principal, as `resolveExecutionContext` builds one. */ @@ -323,6 +332,116 @@ describe('#15079 point 4 — the write is `{ ...patch, ...params }`, a param of }); }); +// ─────────────────────────────────────────────────────────────────────────── +// #24268 — the declared write set IS the accept set, also when it is empty +// ─────────────────────────────────────────────────────────────────────────── + +/** + * [#24268] A declarative update has no handler: its param bag is merged over + * `patch` and WRITTEN to the row. So its declared `params` list is its whole + * wire write set, and an empty or absent list is an EMPTY accept set — not the + * absent contract a param-less HANDLER action has. Before this, `close_case` + * (patch `{ status }`, no `params`) wrote any field the caller may write. + * + * Every refusal here is pinned on the envelope (`code` + `status`) and on "no + * write happened" — never on a bare throw, which an unfixed door that failed + * for any other reason would satisfy too. + */ +describe('#24268 — a param-less declarative update writes EXACTLY its patch, at both doors', () => { + /** A key the object declares and the action does not: the widening the card measured. */ + const WIDENING = { note: 'WIDENED BY AN UNDECLARED PARAM' }; + + it('REST: an undeclared key is refused with the ADR-0104 D2 envelope, and the row is unchanged', async () => { + const rig = makeRig(); + const res = await dispatchRest(OWNER, rig, `/${OBJECT}/close_case/${RECORD_ID}`, { params: WIDENING }); + + expect(res.status).toBe(400); + expect(res.body.error.code).toBe('VALIDATION_ERROR'); + // The named subject: the refusal says WHICH key, so the caller can fix it. + expect(res.body.error.message).toContain('"note"'); + expect(rig.updates).toHaveLength(0); + expect(rig.row).toEqual(freshRow()); + }); + + it('REST: a `patch` key sent as a param is undeclared too — the wire never re-states the patch', async () => { + // The shape a console vendored before objectui fb501bc894 posts: the + // patch keys as `params`. The route applies `patch` from the + // declaration, so the copy on the wire is an undeclared param. + const rig = makeRig(); + const res = await dispatchRest(OWNER, rig, `/${OBJECT}/close_case/${RECORD_ID}`, { + params: { status: 'reopened' }, + }); + + expect(res.status).toBe(400); + expect(res.body.error.code).toBe('VALIDATION_ERROR'); + expect(rig.updates).toHaveLength(0); + expect(rig.row).toEqual(freshRow()); + }); + + it('MCP `run_action`: the same undeclared key is refused with `code` + `status`, before any write', async () => { + const rig = makeRig(); + const err: any = await dispatchMcp(OWNER, rig, 'close_case', { recordId: RECORD_ID, params: WIDENING }) + .then(() => undefined, (e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + // What the MCP tool renderer reads off a throw (`thrownRefusalEnvelope`, + // `@objectstack/mcp`): a string `code` and a numeric `status`. The code + // is the one the REST door's `deps.error(…, 400)` derives. + expect(err.code).toBe('VALIDATION_ERROR'); + expect(err.status).toBe(400); + expect(String(err.message)).toContain('"note"'); + expect(rig.updates).toHaveLength(0); + expect(rig.row).toEqual(freshRow()); + }); + + it('an ABSENT or EMPTY bag still writes the patch — at both doors', async () => { + for (const body of [{}, { params: {} }]) { + const rig = makeRig(); + const res = await dispatchRest(OWNER, rig, `/${OBJECT}/close_case/${RECORD_ID}`, body); + expect(res.status, JSON.stringify(body)).toBe(200); + expect(rig.updates.map((u) => u.data)).toEqual([{ status: 'closed' }]); + expect(rig.row.status).toBe('closed'); + } + for (const input of [{ recordId: RECORD_ID }, { recordId: RECORD_ID, params: {} }]) { + const rig = makeRig(); + const out: any = await dispatchMcp(OWNER, rig, 'close_case', input); + expect(out.ok, JSON.stringify(input)).toBe(true); + expect(rig.updates.map((u) => u.data)).toEqual([{ status: 'closed' }]); + expect(rig.row.status).toBe('closed'); + } + }); + + it('a DECLARED param still writes and wins over `patch` — at both doors', async () => { + const rest = makeRig(); + const res = await dispatchRest(OWNER, rest, `/${OBJECT}/triage_case/${RECORD_ID}`, { + params: { priority: 'urgent' }, + }); + expect(res.status).toBe(200); + expect(rest.updates.map((u) => u.data)).toEqual([{ status: 'triaged', priority: 'urgent' }]); + + const mcp = makeRig(); + const out: any = await dispatchMcp(OWNER, mcp, 'triage_case', { + recordId: RECORD_ID, + params: { priority: 'urgent' }, + }); + expect(out.ok).toBe(true); + expect(mcp.updates.map((u) => u.data)).toEqual([{ status: 'triaged', priority: 'urgent' }]); + expect(mcp.row).toMatchObject({ status: 'triaged', priority: 'urgent' }); + }); + + it('the control: a param-less HANDLER action keeps the pass-through — the bag reaches the registry', async () => { + // Not this card's population: a handler is the author's own code + // deciding what its bag means. The registry miss (404) is the proof + // the param gate let the undeclared key through, as it always did. + const rig = makeRig(); + const res = await dispatchRest(OWNER, rig, `/${OBJECT}/nudge_case/${RECORD_ID}`, { params: WIDENING }); + + expect(res.status).toBe(404); + expect(rig.ql.executeAction).toHaveBeenCalled(); + expect(rig.updates).toHaveLength(0); + }); +}); + // ─────────────────────────────────────────────────────────────────────────── // Contract point 3 — permissions, hooks and validations, exactly as a user edit // ─────────────────────────────────────────────────────────────────────────── @@ -500,6 +619,33 @@ describe('#15079 point 7 — no current record is a located refusal, not a silen expect(res.body.error.message).toMatch(/nothing to write/); expect(rig.updates).toHaveLength(0); }); + + // [#24268] The MCP door renders a thrown refusal through `@objectstack/mcp`'s + // `thrownRefusalEnvelope`, which reads a string `code` and a numeric + // `status` off the throw — a throw with no `code` reaches the agent as a + // bare sentence. Measured over the real `/mcp` bridge before the fix: both + // refusals below answered message-only. So the pin is on the two members + // that renderer reads, and on "nothing was written". + it('the MCP door answers the same two refusals with `code` + `status`, and writes nothing', async () => { + const noRecord = makeRig(); + const missing: any = await dispatchMcp(OWNER, noRecord, 'close_case', {}) + .then(() => undefined, (e: unknown) => e); + expect(missing).toBeInstanceOf(Error); + expect(missing.code).toBe('VALIDATION_ERROR'); + expect(missing.status).toBe(400); + expect(String(missing.message)).toMatch(/recordId/); + expect(noRecord.updates).toHaveLength(0); + + const empty = makeRig(); + const nothing: any = await dispatchMcp(OWNER, empty, 'note_case', { recordId: RECORD_ID, params: {} }) + .then(() => undefined, (e: unknown) => e); + expect(nothing).toBeInstanceOf(Error); + expect(nothing.code).toBe('VALIDATION_ERROR'); + expect(nothing.status).toBe(400); + expect(String(nothing.message)).toMatch(/nothing to write/); + expect(empty.updates).toHaveLength(0); + expect(empty.row).toEqual(freshRow()); + }); }); // ─────────────────────────────────────────────────────────────────────────── diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 270d6e4396..f379ccfb9e 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -21,6 +21,7 @@ import { validateActionParams, type ActionSession, type ResolvedActionParam } fr import { EngineQueryOptionsSchema, DATA_ACTION_TO_API_OPERATION } from '@objectstack/spec/data'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { IObjectQLEngine, ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts'; +import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; // [#15942 / #16293] The confirmation member's SPELLING is the contract, so it // is imported, never hand-spelled: the door that refuses and the client that // retries have to agree on it, and two doors each writing their own 'confirm' @@ -1655,16 +1656,33 @@ export function resolveDeclaredActionParams(_deps: ActionExecutionDeps, action: * costs one edited call. (D1's half went the opposite way for the opposite * reason — it strands stored rows, which nobody can edit their way out of.) * - * Actions that declare no `params` keep the pass-through — there is nothing to - * validate against, so existing param-less actions are untouched. + * ## Which param-less actions keep the pass-through: those with a HANDLER + * + * A handler action that declares no `params` keeps the pass-through (ADR-0104 + * D2's boundary, unchanged for it): its handler is the author's own code + * deciding what its bag means, and there is no declaration to judge the bag by. + * + * A declarative update (`operation: 'update'`, {@link isDeclarativeUpdateAction}) + * does NOT, and the reason is what its bag is for (#24268). It has no handler: + * the bag is merged over `patch` and WRITTEN to the row + * ({@link declarativeUpdateWrite}). So its declared `params` list is its whole + * wire write set, and an empty or absent list is an EMPTY accept set rather + * than an absent contract. Without that reading, an action declaring + * `patch: { status }` and nothing else wrote any field the caller may write — + * declared one field, enforced none. Its bag is judged here by the same + * validator against `[]`, so every key is refused except the dispatcher + * built-ins: the same message, the same 400 at the REST door, the same + * `OS_ALLOW_LAX_ACTION_PARAMS` escape hatch a declared-params action gets. No + * switch of its own. */ -export function enforceActionParams(deps: ActionExecutionDeps, +export function enforceActionParams(deps: ActionExecutionDeps, action: any, obj: any, bag: Record, where: { objectName?: string; actionName?: string }, ): string | null { - if (!Array.isArray(action?.params) || action.params.length === 0) return null; + const declaresParams = Array.isArray(action?.params) && action.params.length > 0; + if (!declaresParams && !isDeclarativeUpdateAction(action)) return null; const resolved = resolveDeclaredActionParams(deps, action, obj); const issues = validateActionParams(resolved, bag); if (issues.length === 0) return null; @@ -2183,7 +2201,11 @@ export interface DeclarativeUpdateResult { * Contract point 4, and the precedence is the whole content of it — the static * `patch` sits UNDER the values the dialog collected, so a param of the same * name WINS. That is the bulk def's own rule (`bulkActionDefs`), mirrored word - * for word; the spec's `patch` describe states it too. ⛔ Nothing else from the + * for word; the spec's `patch` describe states it too. The collected values are + * the action's DECLARED params (plus the dispatcher built-ins the D2 validator + * allowlists): both doors run {@link enforceActionParams} first, which refuses + * every other wire key — on a param-less update too, whose declared accept set + * is empty (#24268). ⛔ Nothing else from the * action is merged: not `bodyExtra` (refused beside `operation`), not * `recordIdField`, not the route's own `objectName`/`recordId` stamps — those * are handler plumbing, and a declarative update has no handler. @@ -2197,15 +2219,22 @@ export function declarativeUpdateWrite(action: any, params: Record } & AIActionConfirmation, wiring: { @@ -2501,8 +2531,16 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, // bag, and a rejection is corrective feedback the agent consumes in-loop, // which a server-side warning never was. Strict by default (#3438); // OS_ALLOW_LAX_ACTION_PARAMS=1 restores the pass-through. + // + // [#24268] Thrown with `code` + `status` — the 400 and the standard code + // the REST door's `deps.error(paramError, 400)` answers — so the MCP tool + // surface renders the same ADR-0112 envelope rather than a bare sentence + // (`thrownRefusalEnvelope` reads a string `code`). The code is derived + // from the status, never spelled, as `declarativeUpdateRefusal` does. const paramError = enforceActionParams(deps, action, obj, params, { objectName, actionName: name }); - if (paramError) throw new Error(paramError); + if (paramError) { + throw Object.assign(new Error(paramError), { code: standardErrorCodeForHttpStatus(400), status: 400 }); + } // [#15942 / #16293] CONFIRMATION GATE — the last pre-dispatch check, and // the first one that can refuse a well-formed, fully-entitled request. diff --git a/packages/runtime/src/action-params-enforcement.test.ts b/packages/runtime/src/action-params-enforcement.test.ts index 612474201d..8a5c7367db 100644 --- a/packages/runtime/src/action-params-enforcement.test.ts +++ b/packages/runtime/src/action-params-enforcement.test.ts @@ -71,13 +71,48 @@ describe('enforceActionParams — the ADR-0104 D2 gate (#14864)', () => { expect(error).toContain('title'); }); - it('passes an action that declares NO params straight through', () => { + it('passes a HANDLER action that declares NO params straight through', () => { // The documented compatibility leg: nothing to validate against, so a - // param-less action is untouched however odd its bag looks. + // param-less handler action is untouched however odd its bag looks. expect(enforceActionParams(NO_DEPS, { name: 'ping' }, NO_OBJECT, { anything: 1 }, WHERE)).toBeNull(); expect(enforceActionParams(NO_DEPS, { name: 'ping', params: [] }, NO_OBJECT, { anything: 1 }, WHERE)).toBeNull(); }); + describe('[#24268] a param-less DECLARATIVE UPDATE has an empty accept set, not an absent one', () => { + // Its bag is merged over `patch` and written to the row, so what it + // declares is its whole wire write set. Both spellings of "declares + // none" — the key absent, and an empty list — are the same verdict. + const CLOSE = { name: 'close_case', operation: 'update', patch: { status: 'closed' } }; + const CLOSE_EMPTY_LIST = { ...CLOSE, params: [] }; + + it('refuses an undeclared key, naming it — through the same strict path', () => { + for (const action of [CLOSE, CLOSE_EMPTY_LIST]) { + const error = enforceActionParams(NO_DEPS, action, NO_OBJECT, { subject: 'x' }, WHERE); + expect(error).toContain('Invalid action params'); + expect(error).toContain('"subject"'); + } + }); + + it('accepts the empty bag, and the dispatcher built-ins, exactly as the D2 validator allowlists them', () => { + // The positive controls for the refusal above: a gate that refused + // EVERY bag on an update would pass "refuses an undeclared key". + for (const action of [CLOSE, CLOSE_EMPTY_LIST]) { + expect(enforceActionParams(NO_DEPS, action, NO_OBJECT, {}, WHERE)).toBeNull(); + expect(enforceActionParams(NO_DEPS, action, NO_OBJECT, { recordId: 'r1', objectName: 'crm_case' }, WHERE)) + .toBeNull(); + } + }); + + it('`OS_ALLOW_LAX_ACTION_PARAMS=1` accepts it and warns — the one escape hatch, no switch of its own', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.stubEnv('OS_ALLOW_LAX_ACTION_PARAMS', '1'); + const where = { objectName: 'crm_case', actionName: 'close_case_lax_probe' }; + expect(enforceActionParams(NO_DEPS, CLOSE, NO_OBJECT, { subject: 'x' }, where)).toBeNull(); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain('OS_ALLOW_LAX_ACTION_PARAMS=1'); + }); + }); + it('is STRICT by default — no environment variable needed to reject (#3438)', () => { vi.stubEnv('OS_ALLOW_LAX_ACTION_PARAMS', ''); expect(enforceActionParams(NO_DEPS, REQUIRES_TITLE, NO_OBJECT, {}, WHERE)).toContain('Invalid action params'); diff --git a/packages/spec/src/contracts/ai-service.ts b/packages/spec/src/contracts/ai-service.ts index 84b68834da..a6ac55fec7 100644 --- a/packages/spec/src/contracts/ai-service.ts +++ b/packages/spec/src/contracts/ai-service.ts @@ -223,8 +223,10 @@ export interface AIToolDefinition { * declares params at all a platform `confirm` riding there is REFUSED as an * unknown action param — a 400 raised before the confirmation gate is ever * reached, not merely a collision with a name the author might already have - * used. On an action declaring no params that check is a pass-through, so - * the same member would be silently accepted there instead: one placement, + * used. On a handler action declaring no params that check is a + * pass-through (a declarative `operation: 'update'` is the exception: its + * empty list is an empty accept set), so the same member would be silently + * accepted there instead: one placement, * two opposite behaviours, which on its own disqualifies it for a safety * member. * - NOT a transport header. A header is invisible to the tool schema an agent