fix(journeys): assign-bot effect re-read parses wrong CRM envelope key → false negative (EVO-1930)

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);
+  });
 });

src/modules/temporal/activities/nodes/evoai/assignment/assign-bot.node.ts

@@ -51,13 +51,59 @@ 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<any>(
+        { 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,
         botId: bot_id || 'none',
         inboxId: inbox_id,
         action,
         nodeId: input.nodeId,
+        effectVerified: verification.verified,
       });
 
       return {

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,
+  ),
 });

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<T>(
+    context: { nodeType: string; resourceId: string },
+    probe: () => Promise<T>,
+    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
   // ============================================================================