diff --git a/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.spec.ts b/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.spec.ts index f90d632..5099a50 100644 --- a/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.spec.ts +++ b/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.spec.ts @@ -1,16 +1,93 @@ +// AssignBotNode instantiates CrmClientService in its constructor, which +// requires these env vars. Set them before import so the suite runs (and so the +// EVO-1919/EVO-1930 verification tests below are exercised rather than blocked +// at ctor). +process.env.EVOAI_CRM_API_TOKEN ||= 'test-token'; +process.env.EVOAI_CRM_BASE_URL ||= 'http://crm-test.local'; + import { AssignBotNode } from './assign-bot.node'; describe('AssignBotNode', () => { + // Build a node with a stubbed crmService. + // + // EVO-1930: `getInboxBot` must mirror the REAL CRM envelope. GET + // /inboxes/:id/agent_bot returns `success_response(data: + // AgentBotSerializer.serialize(agent_bot, agent_bot_inbox:))`, i.e.: + // - bound → { success, data: { agent_bot: {...}, configuration: {...} } } + // - unbound → { success, data: null } (serializer returns nil for no bot) + // The previous (EVO-1919) spec mocked `{ data: { data: bot } }`, which did NOT + // match production and is exactly why the false-negative slipped through. + // `boundBot` here is the inbox's bound bot (null = no binding). + const makeNode = (opts: { + assignBot?: jest.Mock; + boundBot?: any; + verifyEnabled?: boolean; + getInboxBot?: jest.Mock; + }) => { + const node = new AssignBotNode(); + const assignBot = + opts.assignBot ?? jest.fn().mockResolvedValue({ success: true, data: {} }); + const getInboxBot = + opts.getInboxBot ?? + jest.fn().mockResolvedValue({ + success: true, + // Real CRM body: data is null when unbound, otherwise the serializer's + // { agent_bot, configuration } shape. + data: { + success: true, + data: opts.boundBot + ? { agent_bot: opts.boundBot, configuration: {} } + : null, + }, + }); + (node as any).crmService = { + assignBot, + getInboxBot, + isEffectVerificationEnabled: jest + .fn() + .mockReturnValue(opts.verifyEnabled ?? true), + }; + // Reuse the real verifyEffect implementation from CrmClientService so the + // node's verification wiring is exercised end-to-end. + const { + CrmClientService, + } = require('../../../../../../shared/crm-client/crm-client.service'); + (node as any).crmService.verifyEffect = + CrmClientService.prototype.verifyEffect.bind((node as any).crmService); + + jest + .spyOn(node as any, 'interpolateNodeData') + .mockImplementation((_input: any, nodeData: any) => + Promise.resolve(nodeData), + ); + // logNodeStart/Success/Error call @temporalio/activity log which requires + // an activity context; stub them out for unit tests. + jest + .spyOn(node as any, 'logNodeStart') + .mockImplementation(() => undefined); + jest + .spyOn(node as any, 'logNodeSuccess') + .mockImplementation(() => undefined); + jest + .spyOn(node as any, 'logNodeError') + .mockImplementation(() => undefined); + jest.spyOn((node as any).logger, 'log').mockImplementation(() => undefined); + jest + .spyOn((node as any).logger, 'warn') + .mockImplementation(() => undefined); + jest + .spyOn((node as any).logger, 'error') + .mockImplementation(() => undefined); + return { node, assignBot, getInboxBot }; + }; + + afterEach(() => jest.restoreAllMocks()); + // EVO-1741 regression guard: assign-bot is inbox-level (set_agent_bot) and // does NOT use conversationId, so it must NOT be treated as conversation- // required — it runs on a contact-only trigger as long as inbox_id is set. it('runs without a conversation (inbox-level) and is not falsely skipped', async () => { - const node = new AssignBotNode(); - const assignBot = jest.fn().mockResolvedValue({ success: true, data: {} }); - (node as any).crmService = { assignBot }; - jest - .spyOn(node as any, 'interpolateNodeData') - .mockImplementation((_input, nodeData) => Promise.resolve(nodeData)); + const { node, assignBot } = makeNode({ boundBot: { id: 'b1' } }); const result = await node.execute({ nodeId: 'n1', @@ -23,4 +100,110 @@ describe('AssignBotNode', () => { expect(result.success).toBe(true); expect(result.skipped).toBeFalsy(); }); + + it('EVO-1930: confirms success when the inbox agent_bot binding reflects the requested bot (no false negative)', async () => { + // Real CRM bound envelope: { success, data: { agent_bot: { id }, configuration } }. + const { node, getInboxBot } = makeNode({ boundBot: { id: 'b1' } }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: 'b1' }, + }); + + // Re-read targets the INBOX binding (GET /inboxes/:id/agent_bot). + expect(getInboxBot).toHaveBeenCalledWith('inbox-1'); + expect(result.success).toBe(true); + expect(result.error).toBeFalsy(); + }); + + it('EVO-1930: tolerates numeric bot ids from the CRM (string-coerced compare)', async () => { + const { node } = makeNode({ boundBot: { id: 42 } }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: '42' }, + }); + + expect(result.success).toBe(true); + }); + + it('EVO-1919: fails when CRM returns 2xx but the bot binding was not created (D11)', async () => { + // assignBot 200, but re-read shows no bound bot (data: null). + const { node, getInboxBot } = makeNode({ boundBot: null }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: 'b1' }, + }); + + expect(getInboxBot).toHaveBeenCalledWith('inbox-1'); + expect(result.success).toBe(false); + expect(result.error).toMatch(/not persisted/i); + }); + + it('EVO-1930: fails when re-read shows a DIFFERENT bot than requested', async () => { + const { node } = makeNode({ boundBot: { id: 'other-bot' } }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: 'b1' }, + }); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/not persisted/i); + }); + + it('EVO-1919: unassignment is confirmed when re-read shows no bound bot', async () => { + const { node } = makeNode({ boundBot: null }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1' }, // no bot_id → unassign + }); + + expect(result.success).toBe(true); + }); + + it('EVO-1919: a flaky verification probe does NOT fail the node', async () => { + const getInboxBot = jest + .fn() + .mockRejectedValue(new Error('CRM unavailable')); + const { node } = makeNode({ getInboxBot }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: 'b1' }, + }); + + expect(result.success).toBe(true); + }); + + it('EVO-1919: skips verification entirely when the flag is disabled', async () => { + const { node, getInboxBot } = makeNode({ + verifyEnabled: false, + boundBot: null, + }); + + const result = await node.execute({ + nodeId: 'n1', + conversationId: '', + sessionId: 's1', + nodeData: { inbox_id: 'inbox-1', bot_id: 'b1' }, + }); + + expect(getInboxBot).not.toHaveBeenCalled(); + expect(result.success).toBe(true); + }); }); diff --git a/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.ts b/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.ts index 1783799..c8d2fe8 100644 --- a/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.ts +++ b/src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.ts @@ -51,6 +51,51 @@ export class AssignBotNode extends BaseNode { const isUnassignment = !bot_id; const action = isUnassignment ? 'unassigned' : 'assigned'; + // EVO-1919 hardening: POST /inboxes/:id/set_agent_bot returns 200 even + // when it never creates the agent_bot_inboxes binding (D11). Re-read the + // inbox's bound bot (GET /inboxes/:id/agent_bot) and confirm the binding + // matches the requested state; fail the node when the effect is + // unconfirmed. + // + // EVO-1930: the re-read targets the correct inbox resource, but the + // confirm predicate parsed the wrong level of the CRM envelope. The CRM + // (AgentBotSerializer) wraps the bot under `data.agent_bot` when a binding + // exists ({ success, data: { agent_bot: {...}, configuration: {...} } }), + // and returns `data: null` when there is no binding. The previous code + // read `.id` off the `{ agent_bot, configuration }` wrapper (which has no + // `id`), so a present binding produced `boundBotId === null` → false + // negative ("does not reflect ... after re-read"). Unwrap `agent_bot`. + const verification = await this.crmService.verifyEffect( + { nodeType: 'assign-bot', resourceId: inbox_id }, + () => this.crmService.getInboxBot(inbox_id), + (botResponse: any) => { + // getInboxBot → CrmApiResponse whose `data` is the full CRM body: + // { success, data: { agent_bot, configuration } | null, meta }. + const body = botResponse?.data; + const envelope = body?.data ?? body; + // Bound state nests the bot under `agent_bot`; tolerate a flattened + // shape too (agent_bot directly), but never treat the wrapper as the + // bot itself. + const boundBot = envelope?.agent_bot ?? null; + const boundBotId = + boundBot?.id !== undefined && boundBot?.id !== null + ? String(boundBot.id) + : null; + if (isUnassignment) { + return boundBotId === null; + } + return boundBotId === String(bot_id); + }, + ); + + if (verification.verified && !verification.confirmed) { + throw new Error( + `Bot ${action} not persisted: CRM accepted the request (2xx) but the ` + + `inbox ${inbox_id} bot binding does not reflect ` + + `${isUnassignment ? 'unassignment' : `bot ${bot_id}`} after re-read`, + ); + } + // Log successful assignment/unassignment this.logger.log(`Bot ${action} successfully`, { conversationId: input.conversationId, @@ -58,6 +103,7 @@ export class AssignBotNode extends BaseNode { inboxId: inbox_id, action, nodeId: input.nodeId, + effectVerified: verification.verified, }); return { diff --git a/src/shared/crm-client/crm-client.config.ts b/src/shared/crm-client/crm-client.config.ts index d63c5f9..2bd2854 100644 --- a/src/shared/crm-client/crm-client.config.ts +++ b/src/shared/crm-client/crm-client.config.ts @@ -28,6 +28,15 @@ export interface CrmClientConfig { * total) on 5xx/network errors, matching the PRD (FR35, NFR31). */ genericRetryBackoffMs: number[]; + /** + * EVO-1919 hardening: when true (default), journey write-nodes whose effect + * is cheaply verifiable (add-label, assign-bot) re-read the resource after a + * 2xx write and fail the node if the change did not persist — defense in + * depth against CRM endpoints that respond 200 without persisting (D8/D11). + * Set `EVOAI_JOURNEY_VERIFY_EFFECT=false` to disable (e.g. to shave the extra + * GET if latency matters and the CRM persistence is trusted). + */ + journeyVerifyEffect: boolean; } const parseInteger = (value: string | undefined, fallback: number): number => { @@ -36,6 +45,14 @@ const parseInteger = (value: string | undefined, fallback: number): number => { return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; }; +const parseBoolean = (value: string | undefined, fallback: boolean): boolean => { + if (value === undefined || value === '') return fallback; + const normalized = value.trim().toLowerCase(); + if (['false', '0', 'no', 'off'].includes(normalized)) return false; + if (['true', '1', 'yes', 'on'].includes(normalized)) return true; + return fallback; +}; + const parseBackoffSchedule = ( value: string | undefined, fallback: number[], @@ -67,4 +84,8 @@ export const getCrmClientConfig = (): CrmClientConfig => ({ process.env.EVOAI_CRM_CLIENT_RETRY_BACKOFF_MS, [1_000, 2_000, 4_000], ), + journeyVerifyEffect: parseBoolean( + process.env.EVOAI_JOURNEY_VERIFY_EFFECT, + true, + ), }); diff --git a/src/shared/crm-client/crm-client.service.ts b/src/shared/crm-client/crm-client.service.ts index d2ceeb8..4bfb4cd 100644 --- a/src/shared/crm-client/crm-client.service.ts +++ b/src/shared/crm-client/crm-client.service.ts @@ -965,6 +965,80 @@ export class CrmClientService { ); } + // ============================================================================ + // Effect verification (EVO-1919 hardening — defense in depth vs D8/D11) + // ============================================================================ + + /** + * Whether journey write-nodes should re-read the resource after a 2xx write + * to confirm the effect actually persisted (defends against CRM endpoints + * that answer 200 without persisting — D8 labels, D11 set_agent_bot). + * Controlled by `EVOAI_JOURNEY_VERIFY_EFFECT` (default true). + */ + isEffectVerificationEnabled(): boolean { + return getCrmClientConfig().journeyVerifyEffect; + } + + /** + * Reusable "verify effect" wrapper for write-nodes. After a write that + * returned a 2xx, the caller passes a `probe` that re-reads the resource and + * a `confirm` predicate that decides whether the effect is present in the + * re-read state. + * + * Returns a discriminated result: + * - `{ verified: true, confirmed }` — the probe ran; `confirmed` reflects + * whether the effect persisted (caller fails the node when false). + * - `{ verified: false }` — verification was skipped (disabled + * via flag) OR the probe itself failed (network/timeout/breaker). We do + * NOT fail the node on a flaky read — the write already returned 2xx, so a + * re-read failure is treated as "cannot confirm" rather than "did not + * persist", avoiding false negatives. The reason is logged. + * + * The probe is a single cheap GET; 4xx/404 during the probe are classified + * as client responses by the generic path and do NOT open the circuit + * breaker (EVO-1918), so verification never trips resilience. + */ + async verifyEffect( + context: { nodeType: string; resourceId: string }, + probe: () => Promise, + confirm: (state: T) => boolean, + ): Promise<{ verified: true; confirmed: boolean } | { verified: false }> { + if (!this.isEffectVerificationEnabled()) { + return { verified: false }; + } + + let state: T; + try { + state = await probe(); + } catch (error) { + // Re-read failed (network/timeout/circuit). Don't fail the node on a + // flaky probe — the write itself already succeeded (2xx). + CrmClientService.logger.warn( + `Effect verification probe failed; cannot confirm effect ${JSON.stringify( + { + nodeType: context.nodeType, + resourceId: context.resourceId, + error: error instanceof Error ? error.message : String(error), + }, + )}`, + ); + return { verified: false }; + } + + const confirmed = confirm(state); + if (!confirmed) { + CrmClientService.logger.warn( + `Effect verification FAILED: CRM returned 2xx but the change did not persist ${JSON.stringify( + { + nodeType: context.nodeType, + resourceId: context.resourceId, + }, + )}`, + ); + } + return { verified: true, confirmed }; + } + // ============================================================================ // Test/diagnostic helpers // ============================================================================