diff --git a/.changeset/flow-action-record-id-seeding.md b/.changeset/flow-action-record-id-seeding.md new file mode 100644 index 0000000000..ee6ba5710b --- /dev/null +++ b/.changeset/flow-action-record-id-seeding.md @@ -0,0 +1,33 @@ +--- +"@objectstack/runtime": patch +--- + +fix(actions): seed a flow action's params with the row id, like the trigger route does (#3915 follow-up) + +#3915 gave the REST `/actions/:object/:action` route its flow dispatch and +documented it as "equivalent to `POST /api/v1/automation/:target/trigger`, +without having to know the flow name". A real run showed that claim did not +hold: the params bag carried the subject record's fields — so `id` — but never +`recordId`. The CRM's own `crm_convert_lead` action declares +`recordIdParam: 'recordId'` and its flow reads `{recordId}`, so invoking it +through the actions endpoint reached the automation engine and then died at its +first node: + +``` +Flow 'crm_convert_lead_wizard' failed: Node 'get_lead' failed: get_record: +refusing to run — 1 filter condition(s) resolved to nothing … `{recordId}` (at id) +``` + +while the identical run through `/automation/crm_convert_lead_wizard/trigger` +paused normally on its first screen. Only a live invocation surfaced it — the +unit tests mock `automation.execute`, so they pinned the call shape without +noticing the bag was missing the key flows actually read. + +`dispatchFlowAction` now seeds the row id under the same keys +`domains/automation.ts` seeds for the trigger route — `recordId` and the +`Id` camelCase alias — plus the action's own declared +`recordIdParam` (sourced from `recordIdField`, default `id`) when it names a +third key. Explicit action params still win over every seed, and the seeding +applies to the MCP `run_action` path too, which shared the same gap. A declared +`recordIdParam` that no dispatcher honoured was the `declared ≠ enforced` shape +in miniature. diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index b686cae484..fcb3aca552 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -319,6 +319,66 @@ export function flowActionUnavailableError(action: any): string { return `Action '${action?.name ?? 'unknown'}' is a flow but no automation service is available`; } +/** + * The params bag a flow action hands the automation engine. + * + * Three seeds, weakest first — each only fills a key the stronger one left + * unset: + * 1. the subject record's fields, which populate a flow's named `isInput` + * variables the way the record-change trigger does; + * 2. the row id under the keys a flow author actually writes — + * `recordId` and the `Id` camelCase alias — the SAME two + * `POST /automation/:name/trigger` seeds (`domains/automation.ts`), plus + * the action's own declared `recordIdParam` (seeded from `recordIdField`, + * default `id`) when it names a third key; + * 3. the caller's explicit action params, which win outright. + * + * Seed 2 is the one #3915's first pass missed, and only a real run caught it: + * the params bag carried the record's `id` but never `recordId`, so the CRM's + * own `crm_convert_lead` action — which declares `recordIdParam: 'recordId'` + * and whose flow reads `{recordId}` — reached the engine and died at its first + * node ("1 filter condition(s) resolved to nothing"), while the identical run + * through `/automation/crm_convert_lead_wizard/trigger` succeeded. A declared + * `recordIdParam` that nothing honours is the `declared ≠ enforced` shape in + * miniature. + */ +export function seedFlowActionParams(deps: ActionExecutionDeps, + action: any, + input: { + objectName: string; + record: Record; + params: Record; + recordId?: string; + }, +): Record { + const { objectName, record, params, recordId } = input; + const seeded: Record = { ...record }; + + // `recordIdField` names the row field whose value seeds the key (default + // `id`) — a declaration may want a non-id value (spec: `token` for + // revoke-session). Fall back to the explicit recordId when the record + // never loaded (a record-less / new-record invocation). + const idField: string = typeof action?.recordIdField === 'string' && action.recordIdField + ? action.recordIdField + : 'id'; + const rowId: unknown = record?.[idField] ?? (idField === 'id' ? recordId : undefined); + + if (rowId != null) { + const keys = new Set(['recordId']); + if (objectName && objectName !== 'global') { + keys.add(`${objectName.replace(/_([a-z])/g, (_m: string, c: string) => c.toUpperCase())}Id`); + } + if (typeof action?.recordIdParam === 'string' && action.recordIdParam) { + keys.add(action.recordIdParam); + } + for (const key of keys) { + if (seeded[key] === undefined) seeded[key] = rowId; + } + } + + return { ...seeded, ...params }; +} + /** * Dispatch a `type: 'flow'` action through the automation service. * @@ -333,6 +393,12 @@ export function flowActionUnavailableError(action: any): string { * what lets a `runAs: 'user'` flow enforce RLS as the invoker instead of * falling into the user-less UNSCOPED path (#2849, ADR-0049 / #1888; mirrors * the record-change trigger's context shape). + * + * The params bag is seeded exactly like `POST /automation/:name/trigger` + * (`domains/automation.ts`) — see {@link seedFlowActionParams}. Invoking a + * flow ACTION and triggering its flow directly must land the same run, or + * "the actions endpoint dispatches flows for you" is a claim the runtime + * doesn't keep. */ export async function dispatchFlowAction(deps: ActionExecutionDeps, action: any, @@ -340,11 +406,12 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, objectName: string; record: Record; params: Record; + recordId?: string; ec: any; envId?: string; }, ): Promise { - const { objectName, record, params, ec, envId } = wiring; + const { objectName, record, params, recordId, ec, envId } = wiring; const automation = await resolveAutomationService(deps, envId); if (!automation) { throw new Error(flowActionUnavailableError(action)); @@ -358,9 +425,7 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, ...(Array.isArray(ec?.positions) && ec.positions.length ? { positions: ec.positions } : {}), ...(Array.isArray(ec?.permissions) && ec.permissions.length ? { permissions: ec.permissions } : {}), ...(ec?.tenantId ? { tenantId: ec.tenantId } : {}), - // Record fields seed flows' named `isInput` variables (like the - // record-change trigger); explicit action params win on clash. - params: { ...record, ...params }, + params: seedFlowActionParams(deps, action, { objectName, record, params, recordId }), }); if (result && typeof result === 'object' && 'success' in result && result.success === false) { throw new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); @@ -645,7 +710,7 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, // ── flow dispatch ── (shared with the REST /actions route, #3915) if (action.type === 'flow') { - const result = await dispatchFlowAction(deps, action, { objectName, record, params, ec, envId }); + const result = await dispatchFlowAction(deps, action, { objectName, record, params, recordId, ec, envId }); return { ok: true, action: action.name, objectName, ...(recordId ? { recordId } : {}), result }; } diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 9087e6c09b..0d73de0314 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -240,6 +240,7 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string objectName, record, params: reqParams, + recordId, ec, envId: _context?.environmentId, }); 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 821f45290c..08405e3d03 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -109,6 +109,95 @@ describe('REST /actions — flow dispatch (#3915)', () => { expect(res.response.body.data).toEqual({ success: true, data: { success: true, output: { converted: true } } }); }); + // ── params seeding ── the half a mocked automation service could not + // catch. Found by invoking the CRM's real `crm_convert_lead` against a + // running server: the bag carried the record's `id` but never `recordId`, + // so the flow's `get_lead` node died on `{recordId}` resolving to nothing + // while `/automation/crm_convert_lead_wizard/trigger` ran the same flow + // fine. Invoking the ACTION and triggering its FLOW must land the same run. + it('seeds the row id under `recordId` and the `Id` alias, like the trigger route', async () => { + const execute = vi.fn(async () => ({ success: true })); + const { dispatcher } = makeDispatcher({ + objectDef: { name: 'crm_lead', actions: [flowAction] }, + automation: { execute }, + record: { id: 'lead_1', company: 'Radium Labs' }, + }); + + await dispatcher.handleActions('/crm_lead/convert_lead/lead_1', 'POST', {}, ctxFor()); + + expect(execute.mock.calls[0]?.[1]).toMatchObject({ + params: { id: 'lead_1', recordId: 'lead_1', crmLeadId: 'lead_1', company: 'Radium Labs' }, + }); + }); + + it('honours a declared `recordIdParam` naming a key of its own', async () => { + const execute = vi.fn(async () => ({ success: true })); + const { dispatcher } = makeDispatcher({ + objectDef: { + name: 'crm_lead', + actions: [{ ...flowAction, recordIdParam: 'leadToConvert' }], + }, + automation: { execute }, + record: { id: 'lead_1' }, + }); + + await dispatcher.handleActions('/crm_lead/convert_lead/lead_1', 'POST', {}, ctxFor()); + + expect((execute.mock.calls[0]?.[1] as any).params).toMatchObject({ + leadToConvert: 'lead_1', + recordId: 'lead_1', + }); + }); + + it('seeds from `recordIdField` when the declaration wants a non-id value', async () => { + const execute = vi.fn(async () => ({ success: true })); + const { dispatcher } = makeDispatcher({ + objectDef: { + name: 'crm_lead', + actions: [{ ...flowAction, recordIdParam: 'token', recordIdField: 'session_token' }], + }, + automation: { execute }, + record: { id: 'lead_1', session_token: 'tok_abc' }, + }); + + await dispatcher.handleActions('/crm_lead/convert_lead/lead_1', 'POST', {}, ctxFor()); + + expect((execute.mock.calls[0]?.[1] as any).params.token).toBe('tok_abc'); + }); + + it('lets an explicit param win over every seeded key', async () => { + const execute = vi.fn(async () => ({ success: true })); + const { dispatcher } = makeDispatcher({ + objectDef: { name: 'crm_lead', actions: [flowAction] }, + automation: { execute }, + record: { id: 'lead_1' }, + }); + + await dispatcher.handleActions( + '/crm_lead/convert_lead/lead_1', + 'POST', + { params: { recordId: 'explicit_override' } }, + ctxFor(), + ); + + 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. + 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 + }); + + await dispatcher.handleActions('/crm_lead/convert_lead/lead_404', 'POST', {}, ctxFor()); + + expect((execute.mock.calls[0]?.[1] as any).params.recordId).toBe('lead_404'); + }); + it('forwards the caller identity so a `runAs: user` flow enforces RLS as the invoker', async () => { const execute = vi.fn(async () => ({ success: true })); const { dispatcher } = makeDispatcher({