From 1afc6bd023e91a4e1284c2a94e11aa8f69d7bdb5 Mon Sep 17 00:00:00 2001 From: Matheus Pastorini Date: Thu, 23 Jul 2026 14:37:10 -0300 Subject: [PATCH 01/29] fix(EVO-1840): Set Variable node honors Increase/Decrease at runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Set Variable config UI offers numeric Increase/Decrease (preview shows +40), but the runtime never read `operation` — SetVariableNodeInput.nodeData didn't even declare it — so every op was a plain SET and "increase lead_score by 40" never accumulated (silent-success family, EVO-1740/EVO-1757). - Declare operation/value/category on nodeData (removes the `as any` casts). - Add loadSessionVariables() (mirrors conditional.node.ts / EVO-1913) to read the current value. - For increase/decrease: base = Number(prior) (unset/non-numeric prior -> 0), delta = Number(value); write base +/- delta as a number (so downstream numeric comparisons keep working). A non-numeric amount throws -> visible failure (success:false) instead of a silent no-op (AC #3). Plain SET and all other ops are unchanged. Scope: increase/decrease only. - New set-variable.node.spec.ts (jest): increase from numeric/unset/non-numeric prior, decrease, plain set unchanged + doesn't read session, default set, non-numeric amount fails visibly. 7 tests pass; tsc clean. --- .../nodes/set-variable.node.spec.ts | 94 +++++++++++++++++++ .../activities/nodes/set-variable.node.ts | 82 ++++++++++++++-- 2 files changed, 170 insertions(+), 6 deletions(-) create mode 100644 src/modules/temporal/activities/nodes/set-variable.node.spec.ts diff --git a/src/modules/temporal/activities/nodes/set-variable.node.spec.ts b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts new file mode 100644 index 0000000..465a5bb --- /dev/null +++ b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts @@ -0,0 +1,94 @@ +import { SetVariableNode, SetVariableNodeInput } from './set-variable.node'; + +// EVO-1840: the Set Variable node offers Increase/Decrease in the UI but the +// runtime used to ignore `operation` and do a plain SET, so increments never +// accumulated. These lock the arithmetic and the visible-failure on a bad amount. +describe('SetVariableNode', () => { + let node: SetVariableNode; + + const input = ( + nodeData: SetVariableNodeInput['nodeData'], + ): SetVariableNodeInput => ({ + nodeId: 'n1', + contactId: 'c1', + sessionId: 's1', + nodeData, + }); + + beforeEach(() => { + node = new SetVariableNode(); + // logNodeError calls the @temporalio/activity logger, which needs an activity + // context; stub it out for unit tests. + 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); + }); + + afterEach(() => jest.restoreAllMocks()); + + function stubSession(vars: Record) { + jest + .spyOn(node as any, 'loadSessionVariables') + .mockResolvedValue(vars); + } + + it('increase adds the amount to the current numeric value', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.success).toBe(true); + expect(result.variables?.lead_score).toBe(50); + }); + + it('increase from an unset variable starts at 0 (lands on the delta)', async () => { + stubSession({}); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.variables?.lead_score).toBe(40); + }); + + it('increase from a non-numeric prior value treats the base as 0', async () => { + stubSession({ lead_score: 'not-a-number' }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.variables?.lead_score).toBe(40); + }); + + it('decrease subtracts the amount', async () => { + stubSession({ lead_score: 100 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'decrease', value: '30' }), + ); + expect(result.variables?.lead_score).toBe(70); + }); + + it('plain SET is unchanged and does not read the session', async () => { + const loadSpy = jest + .spyOn(node as any, 'loadSessionVariables') + .mockResolvedValue({}); + const result = await node.execute( + input({ variableName: 'greeting', operation: 'set', value: 'hello' }), + ); + expect(result.variables?.greeting).toBe('hello'); + expect(loadSpy).not.toHaveBeenCalled(); + }); + + it('SET is the default when no operation is given', async () => { + const result = await node.execute( + input({ variableName: 'greeting', value: 'hi' }), + ); + expect(result.variables?.greeting).toBe('hi'); + }); + + it('a non-numeric amount fails visibly instead of a silent no-op', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: 'abc' }), + ); + expect(result.success).toBe(false); + }); +}); diff --git a/src/modules/temporal/activities/nodes/set-variable.node.ts b/src/modules/temporal/activities/nodes/set-variable.node.ts index 9740823..939afc8 100644 --- a/src/modules/temporal/activities/nodes/set-variable.node.ts +++ b/src/modules/temporal/activities/nodes/set-variable.node.ts @@ -7,6 +7,21 @@ export interface SetVariableNodeInput { nodeData: { variableName?: string; variableValue?: any; + // EVO-1840: the config UI (SetVariablePanel) sends `operation` + `value`; the + // runtime declared neither, so `operation` was silently dropped and every op + // became a plain SET. Declaring them removes the `as any` casts below. + operation?: + | 'set' + | 'clear' + | 'increase' + | 'decrease' + | 'now' + | 'yesterday' + | 'tomorrow' + | 'time_of_day' + | 'random_id'; + value?: any; + category?: string; variables?: Array<{ name: string; value: any; @@ -35,15 +50,43 @@ export class SetVariableNode extends BaseNode { // Extract clean variable name from {{variableName}} format const cleanName = input.nodeData.variableName.replace(/^\{\{|\}\}$/g, ''); // Use value or variableValue - const value = (input.nodeData as any).value !== undefined - ? (input.nodeData as any).value - : input.nodeData.variableValue; - variablesToSet[cleanName] = value; - + const value = + input.nodeData.value !== undefined + ? input.nodeData.value + : input.nodeData.variableValue; + const operation = input.nodeData.operation ?? 'set'; + + if (operation === 'increase' || operation === 'decrease') { + // EVO-1840: honor the numeric operation the UI offers. The runtime used + // to ignore `operation` and do a plain SET, so "increase lead_score by + // 40" never accumulated. Read the current value and apply arithmetic. + const delta = Number(value); + if (!Number.isFinite(delta)) { + // EVO-1740 family: fail visibly instead of silently no-op'ing. + throw new Error( + `Set Variable ${operation} requires a numeric amount, got ${JSON.stringify( + value, + )}`, + ); + } + const sessionVariables = await this.loadSessionVariables( + input.sessionId, + ); + const priorRaw = Number(sessionVariables[cleanName]); + // Unset or non-numeric prior value → treat as 0 (first increment lands + // on the delta itself). + const base = Number.isFinite(priorRaw) ? priorRaw : 0; + variablesToSet[cleanName] = + operation === 'increase' ? base + delta : base - delta; + } else { + variablesToSet[cleanName] = value; + } + this.logger.log('Setting single variable', { originalName: input.nodeData.variableName, cleanName, - value, + operation, + value: variablesToSet[cleanName], }); } else if ( input.nodeData.variables && @@ -112,6 +155,33 @@ export class SetVariableNode extends BaseNode { }); } + // EVO-1840: read the session's current variables so increase/decrease can apply + // arithmetic to the prior value. Mirrors conditional.node.ts (EVO-1913): degrade + // to {} on failure but log at ERROR so the cause is visible. + private async loadSessionVariables( + sessionId: string, + ): Promise> { + try { + const dataSource = await this.initializeDatabase(); + const { JourneySession } = await import( + '../../../journeys/entities/journey-session.entity' + ); + const sessionRepository = dataSource.getRepository(JourneySession); + + const session = await sessionRepository.findOne({ + where: { id: sessionId }, + }); + + return session?.variables || {}; + } catch (error: any) { + this.logger.error('Failed to load session variables', { + sessionId, + error: error.message, + }); + return {}; + } + } + private processVariableValue(value: any, context: Record): any { // If not a string, return as is if (typeof value !== 'string') { From 22fae6c7529f52276d91b9277968952a1fb6298b Mon Sep 17 00:00:00 2001 From: Nickolas Oliveira Date: Fri, 24 Jul 2026 13:43:37 -0300 Subject: [PATCH 02/29] feat(evo-flow): surface the CRM rejection reason on a 422, not a generic error (EVO-2203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pipeline nodes already turn a CRM failure into a visible node error via createErrorResult(error), but a 422 threw BadRequestException(body) whose message is generic — so an archived-pipeline refusal reached the journey run as "Bad Request Exception". The 422 handler now lifts the CRM envelope's error.message to the exception message, so the run shows the reason ("Pipeline is archived and cannot receive conversations") while getResponse() keeps error.code for callers that branch on it. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../crm-client/crm-client.service.spec.ts | 24 ++++++++++++++++++ src/shared/crm-client/crm-client.service.ts | 25 ++++++++++++++++--- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/shared/crm-client/crm-client.service.spec.ts b/src/shared/crm-client/crm-client.service.spec.ts index a335c2c..0a97aaf 100644 --- a/src/shared/crm-client/crm-client.service.spec.ts +++ b/src/shared/crm-client/crm-client.service.spec.ts @@ -142,6 +142,30 @@ describe('CrmClientService', () => { ).rejects.toBeInstanceOf(BadRequestException); }); + // EVO-2203: an archived-pipeline refusal must reach the journey run as a readable + // reason, not a generic "Bad Request Exception". + it('surfaces the CRM error message on a 422 envelope, keeping the code', async () => { + fetchMock.mockResolvedValueOnce( + buildFetchResponse({ + status: 422, + body: { + success: false, + error: { + code: 'PIPELINE_ARCHIVED', + message: 'Pipeline is archived and cannot receive conversations', + }, + }, + }), + ); + + await expect( + service.post('/api/v1/pipelines/p1/pipeline_items', { type: 'conversation' }), + ).rejects.toMatchObject({ + message: 'Pipeline is archived and cannot receive conversations', + response: { error: { code: 'PIPELINE_ARCHIVED' } }, + }); + }); + it('throws ServiceUnavailableException on 5xx after exhausting retries', async () => { fetchMock.mockResolvedValue( buildFetchResponse({ status: 500, body: { error: 'boom' } }), diff --git a/src/shared/crm-client/crm-client.service.ts b/src/shared/crm-client/crm-client.service.ts index 1c5f516..e2e7610 100644 --- a/src/shared/crm-client/crm-client.service.ts +++ b/src/shared/crm-client/crm-client.service.ts @@ -28,6 +28,12 @@ export interface CrmApiResponse { statusCode?: number; } +// The CRM 422 error envelope, as returned by ApiErrorCodes-backed responses. +interface CrmErrorEnvelope { + error?: { code?: string; message?: string }; + [key: string]: unknown; +} + export interface CrmConversationContext { conversationId: string; inboxId?: string; @@ -469,13 +475,26 @@ export class CrmClientService { } if (response.status === 422) { - let errorBody: any = null; + let errorBody: CrmErrorEnvelope | string | null = null; try { - errorBody = await response.json(); + errorBody = (await response.json()) as CrmErrorEnvelope; } catch { errorBody = await response.text(); } - throw new BadRequestException(errorBody); + // The CRM error envelope is { error: { code, message } }. Lift the message to the top + // level so the exception's `message` reads the reason ("Pipeline is archived...") + // instead of a generic "Bad Request Exception", while getResponse() keeps error.code + // for callers that branch on it (EVO-2203). + const reason = + (typeof errorBody === 'object' && errorBody?.error?.message) || + (typeof errorBody === 'string' + ? errorBody + : 'CRM rejected the request'); + throw new BadRequestException( + typeof errorBody === 'object' && errorBody !== null + ? { ...errorBody, message: reason } + : reason, + ); } // Other 4xx — surface as BadRequest (unexpected but client-fault). From 0b7e25c8ef97765197009ce9b9f84be829c8e752 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Fri, 24 Jul 2026 20:11:27 -0300 Subject: [PATCH 03/29] fix(EVO-1840): make Set Variable arithmetic fail visibly instead of writing a wrong number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review follow-up on the increase/decrease fix. The arithmetic worked, but every way it could NOT be honored still wrote a wrong value and reported success — the same silent-success class the card set out to close (EVO-1740). - Failed/missing session read no longer degrades to {}. Rebasing to 0 on an unreadable prior silently turned lead_score 500 into 40 with success:true. The read moves to BaseNode.readSessionVariables() (it was duplicated verbatim here and in conditional.node.ts) and throws; conditional keeps its local degrade-to-{} policy, set-variable lets it propagate. - Empty/null amount no longer counts as 0. Number('') and Number(null) are 0, so an empty Amount was a silent "increase by 0" — while the panel renders 1 as the placeholder in that state. - A {{variable}} amount is resolved against the session before parsing. The panel's Amount field has a variable picker and the executor passes nodeData raw, so {{bonus}} arrived literal and aborted the whole journey. - A non-numeric CURRENT value now fails instead of being clobbered to the delta (AC#3); genuinely unset/null/'' still starts at 0. - The array input shape honors operation too — it kept degrading increase to a plain SET, the exact bug being fixed, on the other half of the contract. - The error path reports a duration instead of Date.now() (an epoch timestamp was flowing into logNodeExecution/trackNodeExecution as the node's duration). Tests: 17 in set-variable.node.spec.ts (was 7), covering accumulation across runs, {{var}} amounts, the array shape, and each visible-failure case. tsc clean; conditional/base specs unaffected (47 pass). --- .../temporal/activities/nodes/base.node.ts | 27 +++ .../activities/nodes/conditional.node.ts | 15 +- .../nodes/set-variable.node.spec.ts | 225 ++++++++++++++---- .../activities/nodes/set-variable.node.ts | 190 +++++++++++---- 4 files changed, 345 insertions(+), 112 deletions(-) diff --git a/src/modules/temporal/activities/nodes/base.node.ts b/src/modules/temporal/activities/nodes/base.node.ts index b9af90d..08c5a6d 100644 --- a/src/modules/temporal/activities/nodes/base.node.ts +++ b/src/modules/temporal/activities/nodes/base.node.ts @@ -38,6 +38,33 @@ export abstract class BaseNode { return AppDataSource; } + // EVO-1840: single source of truth for reading a session's variables — this + // read was duplicated verbatim in conditional.node.ts and set-variable.node.ts, + // each with its own catch. It THROWS on a failed read or a missing session; + // the caller decides how to degrade, because the right answer differs: + // conditional evaluates against {} (EVO-1913), while set-variable's + // increase/decrease must fail — a lost read there would silently rebase the + // counter to 0 and clobber the accumulated value. + protected async readSessionVariables( + sessionId: string, + ): Promise> { + const dataSource = await this.initializeDatabase(); + const { JourneySession } = await import( + '../../../journeys/entities/journey-session.entity' + ); + const sessionRepository = dataSource.getRepository(JourneySession); + + const session = await sessionRepository.findOne({ + where: { id: sessionId }, + }); + + if (!session) { + throw new Error(`Journey session ${sessionId} not found`); + } + + return session.variables || {}; + } + protected logNodeStart(nodeId: string, input: any): void { // log.info(`Executing ${this.nodeType} node`, { // nodeId, diff --git a/src/modules/temporal/activities/nodes/conditional.node.ts b/src/modules/temporal/activities/nodes/conditional.node.ts index d82a4d5..dcc9ff2 100644 --- a/src/modules/temporal/activities/nodes/conditional.node.ts +++ b/src/modules/temporal/activities/nodes/conditional.node.ts @@ -684,17 +684,10 @@ export class ConditionalNode extends BaseNode { sessionId: string, ): Promise> { try { - const dataSource = await this.initializeDatabase(); - const { JourneySession } = await import( - '../../../journeys/entities/journey-session.entity' - ); - const sessionRepository = dataSource.getRepository(JourneySession); - - const session = await sessionRepository.findOne({ - where: { id: sessionId }, - }); - - return session?.variables || {}; + // EVO-1840: the read itself now lives in BaseNode (it was duplicated here + // and in set-variable.node.ts); the degrade-to-{} policy below stays local + // because it is specific to condition evaluation. + return await this.readSessionVariables(sessionId); } catch (error: any) { // EVO-1913: surface the failure at ERROR level instead of swallowing it // as an empty bag silently (which made {{session var}} conditions all diff --git a/src/modules/temporal/activities/nodes/set-variable.node.spec.ts b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts index 465a5bb..87ec342 100644 --- a/src/modules/temporal/activities/nodes/set-variable.node.spec.ts +++ b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts @@ -2,7 +2,9 @@ import { SetVariableNode, SetVariableNodeInput } from './set-variable.node'; // EVO-1840: the Set Variable node offers Increase/Decrease in the UI but the // runtime used to ignore `operation` and do a plain SET, so increments never -// accumulated. These lock the arithmetic and the visible-failure on a bad amount. +// accumulated. These lock the arithmetic and — just as important for this card's +// silent-success family (EVO-1740) — every way the operation can NOT be honored +// must surface as a visible failure instead of quietly writing a wrong number. describe('SetVariableNode', () => { let node: SetVariableNode; @@ -33,62 +35,187 @@ describe('SetVariableNode', () => { .mockResolvedValue(vars); } - it('increase adds the amount to the current numeric value', async () => { - stubSession({ lead_score: 10 }); - const result = await node.execute( - input({ variableName: 'lead_score', operation: 'increase', value: '40' }), - ); - expect(result.success).toBe(true); - expect(result.variables?.lead_score).toBe(50); - }); + describe('arithmetic', () => { + it('increase adds the amount to the current numeric value', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.success).toBe(true); + expect(result.variables?.lead_score).toBe(50); + }); - it('increase from an unset variable starts at 0 (lands on the delta)', async () => { - stubSession({}); - const result = await node.execute( - input({ variableName: 'lead_score', operation: 'increase', value: '40' }), - ); - expect(result.variables?.lead_score).toBe(40); - }); + it('increase from an unset variable starts at 0 (lands on the delta)', async () => { + stubSession({}); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.variables?.lead_score).toBe(40); + }); - it('increase from a non-numeric prior value treats the base as 0', async () => { - stubSession({ lead_score: 'not-a-number' }); - const result = await node.execute( - input({ variableName: 'lead_score', operation: 'increase', value: '40' }), - ); - expect(result.variables?.lead_score).toBe(40); - }); + it('increase from a null/empty prior value starts at 0', async () => { + stubSession({ lead_score: null, other: '' }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.variables?.lead_score).toBe(40); + }); + + it('decrease subtracts the amount', async () => { + stubSession({ lead_score: 100 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'decrease', value: '30' }), + ); + expect(result.variables?.lead_score).toBe(70); + }); - it('decrease subtracts the amount', async () => { - stubSession({ lead_score: 100 }); - const result = await node.execute( - input({ variableName: 'lead_score', operation: 'decrease', value: '30' }), - ); - expect(result.variables?.lead_score).toBe(70); + it('accumulates across runs (0 -> +40 -> 40 -> +30 -> 70)', async () => { + stubSession({}); + const first = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(first.variables?.lead_score).toBe(40); + + // second run sees the persisted value + stubSession({ lead_score: first.variables?.lead_score }); + const second = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '30' }), + ); + expect(second.variables?.lead_score).toBe(70); + }); + + it('resolves a {{variable}} amount against the session before parsing', async () => { + // the panel's Amount field has a variable picker, and the executor passes + // nodeData raw — the node must interpolate or a valid config would abort. + stubSession({ lead_score: 10, bonus: 5 }); + const result = await node.execute( + input({ + variableName: 'lead_score', + operation: 'increase', + value: '{{bonus}}', + }), + ); + expect(result.success).toBe(true); + expect(result.variables?.lead_score).toBe(15); + }); }); - it('plain SET is unchanged and does not read the session', async () => { - const loadSpy = jest - .spyOn(node as any, 'loadSessionVariables') - .mockResolvedValue({}); - const result = await node.execute( - input({ variableName: 'greeting', operation: 'set', value: 'hello' }), - ); - expect(result.variables?.greeting).toBe('hello'); - expect(loadSpy).not.toHaveBeenCalled(); + describe('plain SET is untouched', () => { + it('plain SET is unchanged and does not read the session', async () => { + const loadSpy = jest + .spyOn(node as any, 'loadSessionVariables') + .mockResolvedValue({}); + const result = await node.execute( + input({ variableName: 'greeting', operation: 'set', value: 'hello' }), + ); + expect(result.variables?.greeting).toBe('hello'); + expect(loadSpy).not.toHaveBeenCalled(); + }); + + it('SET is the default when no operation is given', async () => { + const result = await node.execute( + input({ variableName: 'greeting', value: 'hi' }), + ); + expect(result.variables?.greeting).toBe('hi'); + }); }); - it('SET is the default when no operation is given', async () => { - const result = await node.execute( - input({ variableName: 'greeting', value: 'hi' }), - ); - expect(result.variables?.greeting).toBe('hi'); + describe('multiple variables', () => { + it('honors increase on the array input shape too', async () => { + stubSession({ lead_score: 10, visits: 2 }); + const result = await node.execute( + input({ + operation: 'increase', + variables: [ + { name: 'lead_score', value: 40 }, + { name: 'visits', value: 1 }, + ], + }), + ); + expect(result.success).toBe(true); + expect(result.variables?.lead_score).toBe(50); + expect(result.variables?.visits).toBe(3); + }); + + it('plain SET on the array shape is unchanged', async () => { + const result = await node.execute( + input({ variables: [{ name: 'greeting', value: 'hello' }] }), + ); + expect(result.variables?.greeting).toBe('hello'); + }); }); - it('a non-numeric amount fails visibly instead of a silent no-op', async () => { - stubSession({ lead_score: 10 }); - const result = await node.execute( - input({ variableName: 'lead_score', operation: 'increase', value: 'abc' }), - ); - expect(result.success).toBe(false); + // AC#3 / EVO-1740: an operation that cannot be honored fails visibly. Every + // case below used to (or would) write a wrong value and report success. + describe('visible failure instead of a silent wrong write', () => { + it('a non-numeric amount fails visibly', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: 'abc' }), + ); + expect(result.success).toBe(false); + expect(result.error).toContain('numeric amount'); + }); + + it('an empty amount fails instead of incrementing by 0', async () => { + // Number('') === 0, so this used to be a silent no-op reported as success + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '' }), + ); + expect(result.success).toBe(false); + }); + + it('a null amount fails instead of incrementing by 0', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: null }), + ); + expect(result.success).toBe(false); + }); + + it('an unresolvable {{variable}} amount fails visibly', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ + variableName: 'lead_score', + operation: 'increase', + value: '{{missing}}', + }), + ); + expect(result.success).toBe(false); + }); + + it('a non-numeric CURRENT value fails instead of being clobbered to the delta', async () => { + stubSession({ lead_score: 'not-a-number' }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.success).toBe(false); + expect(result.error).toContain('not numeric'); + expect(result.variables?.lead_score).toBeUndefined(); + }); + + it('a failed session read fails instead of silently rebasing the counter to 0', async () => { + // degrading to {} here would turn lead_score 500 into 40 and report success + jest + .spyOn(node as any, 'readSessionVariables') + .mockRejectedValue(new Error('connection refused')); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: '40' }), + ); + expect(result.success).toBe(false); + expect(result.variables?.lead_score).toBeUndefined(); + }); + + it('reports the failure duration, not an epoch timestamp', async () => { + stubSession({ lead_score: 10 }); + const result = await node.execute( + input({ variableName: 'lead_score', operation: 'increase', value: 'abc' }), + ); + expect(result.success).toBe(false); + expect(result.executionTime).toBeGreaterThanOrEqual(0); + expect(result.executionTime).toBeLessThan(60_000); + }); }); }); diff --git a/src/modules/temporal/activities/nodes/set-variable.node.ts b/src/modules/temporal/activities/nodes/set-variable.node.ts index 939afc8..9f19f0e 100644 --- a/src/modules/temporal/activities/nodes/set-variable.node.ts +++ b/src/modules/temporal/activities/nodes/set-variable.node.ts @@ -1,5 +1,7 @@ import { BaseNode, NodeExecutionResult } from './base.node'; +type ArithmeticOperation = 'increase' | 'decrease'; + export interface SetVariableNodeInput { nodeId: string; contactId: string; @@ -10,6 +12,11 @@ export interface SetVariableNodeInput { // EVO-1840: the config UI (SetVariablePanel) sends `operation` + `value`; the // runtime declared neither, so `operation` was silently dropped and every op // became a plain SET. Declaring them removes the `as any` casts below. + // NOTE: this union models what the panel can SEND, not what the runtime + // honors. Only set / increase / decrease are implemented; clear, now, + // yesterday, tomorrow, time_of_day and random_id still fall through to the + // plain-SET branch (writing the raw `value`, usually '') — same + // UI-promises-what-the-runtime-drops class as this card, tracked separately. operation?: | 'set' | 'clear' @@ -36,6 +43,8 @@ export class SetVariableNode extends BaseNode { } async execute(input: SetVariableNodeInput): Promise { + const startTime = Date.now(); + return await this.executeWithTiming(input.nodeId, input, async () => { const variablesToSet: Record = {}; @@ -45,6 +54,17 @@ export class SetVariableNode extends BaseNode { nodeData: input.nodeData, }); + const operation = input.nodeData.operation ?? 'set'; + const isArithmetic = + operation === 'increase' || operation === 'decrease'; + + // EVO-1840: increase/decrease is a read-modify-write, so it needs the + // session's current values. Read once, and only when an arithmetic + // operation is actually configured (a plain SET must not touch the DB). + const sessionVariables = isArithmetic + ? await this.loadSessionVariables(input.sessionId) + : {}; + // Support both single variable and multiple variables if (input.nodeData.variableName) { // Extract clean variable name from {{variableName}} format @@ -54,33 +74,16 @@ export class SetVariableNode extends BaseNode { input.nodeData.value !== undefined ? input.nodeData.value : input.nodeData.variableValue; - const operation = input.nodeData.operation ?? 'set'; - - if (operation === 'increase' || operation === 'decrease') { - // EVO-1840: honor the numeric operation the UI offers. The runtime used - // to ignore `operation` and do a plain SET, so "increase lead_score by - // 40" never accumulated. Read the current value and apply arithmetic. - const delta = Number(value); - if (!Number.isFinite(delta)) { - // EVO-1740 family: fail visibly instead of silently no-op'ing. - throw new Error( - `Set Variable ${operation} requires a numeric amount, got ${JSON.stringify( - value, - )}`, - ); - } - const sessionVariables = await this.loadSessionVariables( - input.sessionId, - ); - const priorRaw = Number(sessionVariables[cleanName]); - // Unset or non-numeric prior value → treat as 0 (first increment lands - // on the delta itself). - const base = Number.isFinite(priorRaw) ? priorRaw : 0; - variablesToSet[cleanName] = - operation === 'increase' ? base + delta : base - delta; - } else { - variablesToSet[cleanName] = value; - } + + variablesToSet[cleanName] = isArithmetic + ? this.applyArithmetic( + cleanName, + value, + operation as ArithmeticOperation, + sessionVariables, + input, + ) + : value; this.logger.log('Setting single variable', { originalName: input.nodeData.variableName, @@ -92,9 +95,22 @@ export class SetVariableNode extends BaseNode { input.nodeData.variables && Array.isArray(input.nodeData.variables) ) { - // Multiple variables + // Multiple variables. EVO-1840: the array form carries the same + // node-level `operation`, so it gets the same arithmetic — otherwise + // increase/decrease would keep silently degrading to a plain SET on this + // input shape, which is the exact bug this card fixes. for (const variable of input.nodeData.variables) { - variablesToSet[variable.name] = variable.value; + const cleanName = String(variable.name).replace(/^\{\{|\}\}$/g, ''); + + variablesToSet[cleanName] = isArithmetic + ? this.applyArithmetic( + cleanName, + variable.value, + operation as ArithmeticOperation, + sessionVariables, + input, + ) + : variable.value; } } @@ -150,36 +166,106 @@ export class SetVariableNode extends BaseNode { return this.createSuccessResult(input, executionTime, variables); }) .catch((error) => { - const executionTime = Date.now(); - return this.createErrorResult(error, executionTime); + // executionTime is a DURATION everywhere else (it feeds + // logNodeExecution/trackNodeExecution); this branch used to report + // Date.now(), i.e. an epoch timestamp, as the node's duration. + return this.createErrorResult(error, Date.now() - startTime); }); } - // EVO-1840: read the session's current variables so increase/decrease can apply - // arithmetic to the prior value. Mirrors conditional.node.ts (EVO-1913): degrade - // to {} on failure but log at ERROR so the cause is visible. - private async loadSessionVariables( - sessionId: string, - ): Promise> { - try { - const dataSource = await this.initializeDatabase(); - const { JourneySession } = await import( - '../../../journeys/entities/journey-session.entity' + // EVO-1840: apply the numeric operation the UI offers. The runtime used to + // ignore `operation` and do a plain SET, so "increase lead_score by 40" never + // accumulated. + private applyArithmetic( + name: string, + rawAmount: any, + operation: ArithmeticOperation, + sessionVariables: Record, + input: SetVariableNodeInput, + ): number { + // The panel's Amount field is a VariableInput WITH a variable picker, and + // the executor hands the node its raw nodeData (no interpolation upstream), + // so `{{bonus}}` arrives literal. Resolve it against the session before + // parsing — otherwise a UI-supported config would abort the whole journey. + const resolvedAmount = this.processVariableValue(rawAmount, { + ...sessionVariables, + contactId: input.contactId, + sessionId: input.sessionId, + timestamp: new Date().toISOString(), + }); + + const delta = this.toFiniteNumber(resolvedAmount); + if (delta === null) { + // EVO-1740 family: fail visibly instead of silently no-op'ing. + throw new Error( + `Set Variable ${operation} requires a numeric amount, got ${JSON.stringify( + rawAmount, + )}`, ); - const sessionRepository = dataSource.getRepository(JourneySession); + } - const session = await sessionRepository.findOne({ - where: { id: sessionId }, - }); + const base = this.resolveArithmeticBase( + name, + sessionVariables[name], + operation, + ); - return session?.variables || {}; - } catch (error: any) { - this.logger.error('Failed to load session variables', { - sessionId, - error: error.message, - }); - return {}; + return operation === 'increase' ? base + delta : base - delta; + } + + // An unset variable legitimately starts at 0 (the first increment lands on the + // delta itself). A variable that HOLDS a non-numeric value is different: there + // is no sane arithmetic for it, and rebasing to 0 would silently destroy the + // stored value while reporting success — AC#3 wants that visible. + private resolveArithmeticBase( + name: string, + prior: any, + operation: ArithmeticOperation, + ): number { + if (prior === undefined || prior === null || prior === '') { + return 0; } + + const parsed = this.toFiniteNumber(prior); + if (parsed === null) { + throw new Error( + `Set Variable ${operation} cannot be applied to "${name}": current value ${JSON.stringify( + prior, + )} is not numeric`, + ); + } + + return parsed; + } + + // Number('') and Number(null) are both 0, which would turn an empty/absent + // Amount into a silent "increase by 0" reported as success (the panel even + // renders 1 as the placeholder in that state). Treat "no value" — and + // booleans, which Number() happily coerces — as not-a-number. + private toFiniteNumber(value: any): number | null { + if (value === undefined || value === null || value === '') { + return null; + } + + if (typeof value === 'boolean') { + return null; + } + + const parsed = Number(value); + + return Number.isFinite(parsed) ? parsed : null; + } + + // EVO-1840: read the session's current variables so increase/decrease can + // apply arithmetic to the prior value. Deliberately NO catch here: unlike + // conditional.node.ts (which degrades to {} so evaluation continues), a failed + // read on a read-modify-write would rebase the counter to 0 and silently + // clobber the accumulated value (lead_score 500 → 40) while reporting success + // — the very silent-success class this card fixes (EVO-1740). Let it throw. + private async loadSessionVariables( + sessionId: string, + ): Promise> { + return await this.readSessionVariables(sessionId); } private processVariableValue(value: any, context: Record): any { From 51b332f0ac57e65951bba4a39cb0894fcc76c9fe Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Fri, 24 Jul 2026 20:41:06 -0300 Subject: [PATCH 04/29] fix(evo-flow): surface the CRM refusal on the path the pipeline nodes actually use (EVO-2203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review of #111. The 422 message lift landed in requestGeneric, whose only consumer is contacts-client. The three pipeline nodes call addToPipeline / moveToPipelineStage / createPipelineTask, which go through executeRequest — its 422 branch was untouched, so the journey run kept showing the raw JSON envelope ("CRM Validation error: {\"success\":false,\"error\":{...}}"). describeCrm422 now folds code and reason into that string. The "CRM Validation error" prefix stays: executeRequest matches on it to not retry a refusal. Spreading the envelope also overwrote a top-level `message` with the placeholder, so a 422 from render_record_invalid's fallback lost its reason ("Email is invalid" became "CRM rejected the request"). crmRejectionReason now prefers error.message, then a top-level message, then the raw body. Tests: the refusal is covered where the nodes read it (client level, both endpoints, no retry) and end to end on assign-to-pipeline with a real client — the node specs mock the CRM client away, which is why the gap went unseen. --- .../pipeline/assign-to-pipeline.node.spec.ts | 51 ++++++++++++ .../crm-client/crm-client.service.spec.ts | 78 ++++++++++++++++++- src/shared/crm-client/crm-client.service.ts | 52 ++++++++++--- 3 files changed, 169 insertions(+), 12 deletions(-) diff --git a/src/modules/temporal/activities/nodes/evoai/pipeline/assign-to-pipeline.node.spec.ts b/src/modules/temporal/activities/nodes/evoai/pipeline/assign-to-pipeline.node.spec.ts index 693ed8b..5cffcf7 100644 --- a/src/modules/temporal/activities/nodes/evoai/pipeline/assign-to-pipeline.node.spec.ts +++ b/src/modules/temporal/activities/nodes/evoai/pipeline/assign-to-pipeline.node.spec.ts @@ -1,7 +1,21 @@ +// Silence @temporalio/activity log calls under unit-test (no activity context). +jest.mock('@temporalio/activity', () => ({ + log: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + }, +})); + +process.env.EVOAI_CRM_BASE_URL = 'http://crm-test.local'; +process.env.EVOAI_CRM_API_TOKEN = 'svc-token'; + import { AssignToPipelineNode, AssignToPipelineNodeInput, } from './assign-to-pipeline.node'; +import { CrmClientService } from '../../../../../../shared/crm-client/crm-client.service'; describe('AssignToPipelineNode', () => { let node: AssignToPipelineNode; @@ -49,4 +63,41 @@ describe('AssignToPipelineNode', () => { expect(result.skipped).toBe(true); expect(result.error).toContain('no_pipeline_id'); }); + + // EVO-2203: the examples above mock the CRM client away, so nothing here proved + // what a real refusal looks like on the run. This one drives the real client + // against the CRM's archived-pipeline answer: the journey must stop with the + // reason, never continue as success. + describe('with the real CRM client (archived pipeline)', () => { + it('fails visibly carrying the CRM refusal reason', async () => { + (global as any).fetch = jest.fn().mockResolvedValue({ + ok: false, + status: 422, + statusText: 'HTTP 422', + headers: { get: () => null }, + json: () => Promise.resolve({}), + text: () => + Promise.resolve( + JSON.stringify({ + success: false, + error: { + code: 'PIPELINE_ARCHIVED', + message: + 'Pipeline is archived and cannot receive conversations', + }, + }), + ), + }); + (node as any).crmService = new CrmClientService(); + + const result = await node.execute(baseInput); + + expect(result.success).toBe(false); + expect(result.error).toContain('PIPELINE_ARCHIVED'); + expect(result.error).toContain( + 'Pipeline is archived and cannot receive conversations', + ); + expect(result.error).not.toContain('{'); + }); + }); }); diff --git a/src/shared/crm-client/crm-client.service.spec.ts b/src/shared/crm-client/crm-client.service.spec.ts index 0a97aaf..011e63a 100644 --- a/src/shared/crm-client/crm-client.service.spec.ts +++ b/src/shared/crm-client/crm-client.service.spec.ts @@ -142,8 +142,9 @@ describe('CrmClientService', () => { ).rejects.toBeInstanceOf(BadRequestException); }); - // EVO-2203: an archived-pipeline refusal must reach the journey run as a readable - // reason, not a generic "Bad Request Exception". + // EVO-2203: a refusal on the generic path must read as its reason, not as a + // generic "Bad Request Exception". The journey nodes go through executeRequest + // instead — covered under "pipeline node path" below. it('surfaces the CRM error message on a 422 envelope, keeping the code', async () => { fetchMock.mockResolvedValueOnce( buildFetchResponse({ @@ -159,13 +160,31 @@ describe('CrmClientService', () => { ); await expect( - service.post('/api/v1/pipelines/p1/pipeline_items', { type: 'conversation' }), + service.post('/api/v1/pipelines/p1/pipeline_items', { + type: 'conversation', + }), ).rejects.toMatchObject({ message: 'Pipeline is archived and cannot receive conversations', response: { error: { code: 'PIPELINE_ARCHIVED' } }, }); }); + // Controllers that answer outside the envelope helper put the reason in a + // top-level `message` (render_record_invalid's fallback). Lifting the envelope + // reason must not overwrite it with the placeholder. + it('keeps a top-level message on a 422 body with no error envelope', async () => { + fetchMock.mockResolvedValueOnce( + buildFetchResponse({ + status: 422, + body: { message: 'Email is invalid', attributes: ['email'] }, + }), + ); + + await expect(service.post('/api/v1/contacts', {})).rejects.toMatchObject({ + message: 'Email is invalid', + }); + }); + it('throws ServiceUnavailableException on 5xx after exhausting retries', async () => { fetchMock.mockResolvedValue( buildFetchResponse({ status: 500, body: { error: 'boom' } }), @@ -278,6 +297,59 @@ describe('CrmClientService', () => { }); }); + // EVO-2203: the three pipeline nodes reach the CRM through executeRequest, not + // through the generic path above. This is where an archived-pipeline refusal has + // to become a readable reason — the node copies this string into its error result. + describe('pipeline node path — archived-pipeline refusal', () => { + const archivedEnvelope = { + success: false, + error: { + code: 'PIPELINE_ARCHIVED', + message: 'Pipeline is archived and cannot receive conversations', + }, + meta: { timestamp: '2026-07-24T00:00:00Z' }, + }; + + it('addToPipeline reports the code and the reason, without the raw envelope', async () => { + fetchMock.mockResolvedValue( + buildFetchResponse({ status: 422, body: archivedEnvelope }), + ); + + const result = await service.addToPipeline('p1', 'conv-1', 'st1'); + + expect(result.success).toBe(false); + expect(result.error).toBe( + 'CRM Validation error: PIPELINE_ARCHIVED: Pipeline is archived and cannot receive conversations', + ); + // A refusal is final: retrying it would just archive-reject three times. + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('moveToPipelineStage reports the same refusal', async () => { + fetchMock.mockResolvedValue( + buildFetchResponse({ status: 422, body: archivedEnvelope }), + ); + + const result = await service.moveToPipelineStage('p1', 'conv-1', 'st9'); + + expect(result.success).toBe(false); + expect(result.error).toBe( + 'CRM Validation error: PIPELINE_ARCHIVED: Pipeline is archived and cannot receive conversations', + ); + }); + + it('falls back to the raw body when a 422 is not the CRM envelope', async () => { + fetchMock.mockResolvedValue( + buildFetchResponse({ status: 422, body: 'plain text failure' }), + ); + + const result = await service.addToPipeline('p1', 'conv-1'); + + expect(result.success).toBe(false); + expect(result.error).toBe('CRM Validation error: plain text failure'); + }); + }); + // EVO-1273: pins the HTTP contract the Journey "Create Pipeline Task" node // depends on (URL, method, body and the nested envelope). describe('createPipelineTask — Journey create-task node contract', () => { diff --git a/src/shared/crm-client/crm-client.service.ts b/src/shared/crm-client/crm-client.service.ts index e2e7610..fa275fd 100644 --- a/src/shared/crm-client/crm-client.service.ts +++ b/src/shared/crm-client/crm-client.service.ts @@ -31,9 +31,45 @@ export interface CrmApiResponse { // The CRM 422 error envelope, as returned by ApiErrorCodes-backed responses. interface CrmErrorEnvelope { error?: { code?: string; message?: string }; + message?: string; [key: string]: unknown; } +// A refused write carries its reason in `error.message`; controllers that answer +// outside the envelope helper put it in a top-level `message`. Neither may be +// dropped — the reason is the only thing a journey run shows (EVO-2203). +function crmRejectionReason( + body: CrmErrorEnvelope | string | null, +): string | undefined { + if (typeof body === 'string') return body || undefined; + if (!body || typeof body !== 'object') return undefined; + + const enveloped = body.error?.message; + if (typeof enveloped === 'string' && enveloped) return enveloped; + + return typeof body.message === 'string' && body.message + ? body.message + : undefined; +} + +// A node result carries a plain string, so the refusal's code and reason are folded +// into it — otherwise the journey run shows the raw JSON envelope. Callers keep the +// "CRM Validation error" prefix: executeRequest matches on it to not retry a 422. +function describeCrm422(rawBody: string): string { + let parsed: CrmErrorEnvelope | null = null; + try { + parsed = JSON.parse(rawBody) as CrmErrorEnvelope; + } catch { + return rawBody; + } + + const reason = crmRejectionReason(parsed); + if (!reason) return rawBody; + + const code = parsed?.error?.code; + return typeof code === 'string' && code ? `${code}: ${reason}` : reason; +} + export interface CrmConversationContext { conversationId: string; inboxId?: string; @@ -481,15 +517,11 @@ export class CrmClientService { } catch { errorBody = await response.text(); } - // The CRM error envelope is { error: { code, message } }. Lift the message to the top - // level so the exception's `message` reads the reason ("Pipeline is archived...") - // instead of a generic "Bad Request Exception", while getResponse() keeps error.code - // for callers that branch on it (EVO-2203). + // Lift the reason to the top level so the exception's `message` reads it + // ("Pipeline is archived...") instead of a generic "Bad Request Exception", + // while getResponse() keeps error.code for callers that branch on it (EVO-2203). const reason = - (typeof errorBody === 'object' && errorBody?.error?.message) || - (typeof errorBody === 'string' - ? errorBody - : 'CRM rejected the request'); + crmRejectionReason(errorBody) ?? 'CRM rejected the request'; throw new BadRequestException( typeof errorBody === 'object' && errorBody !== null ? { ...errorBody, message: reason } @@ -632,7 +664,9 @@ export class CrmClientService { if (response.status === 422) { const errorBody = await response.text(); - throw new Error(`CRM Validation error: ${errorBody}`); + throw new Error( + `CRM Validation error: ${describeCrm422(errorBody)}`, + ); } if (response.status === 429) { From 6ef69920f595a40e7cf1f140f9b933ae71d08f71 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Fri, 24 Jul 2026 20:42:37 -0300 Subject: [PATCH 05/29] style(EVO-1840): trim comments to the decision, drop the review narrative Comments only, no behavior change. Each one kept the non-obvious decision and dropped the before/after story, which belongs in the PR, not the source. --- .../temporal/activities/nodes/base.node.ts | 10 +--- .../activities/nodes/conditional.node.ts | 5 +- .../nodes/set-variable.node.spec.ts | 16 ++---- .../activities/nodes/set-variable.node.ts | 55 +++++-------------- 4 files changed, 25 insertions(+), 61 deletions(-) diff --git a/src/modules/temporal/activities/nodes/base.node.ts b/src/modules/temporal/activities/nodes/base.node.ts index 08c5a6d..b2fac53 100644 --- a/src/modules/temporal/activities/nodes/base.node.ts +++ b/src/modules/temporal/activities/nodes/base.node.ts @@ -38,13 +38,9 @@ export abstract class BaseNode { return AppDataSource; } - // EVO-1840: single source of truth for reading a session's variables — this - // read was duplicated verbatim in conditional.node.ts and set-variable.node.ts, - // each with its own catch. It THROWS on a failed read or a missing session; - // the caller decides how to degrade, because the right answer differs: - // conditional evaluates against {} (EVO-1913), while set-variable's - // increase/decrease must fail — a lost read there would silently rebase the - // counter to 0 and clobber the accumulated value. + // Throws on a failed read or a missing session; each caller picks its own + // degrade policy, since evaluating against {} is safe for conditions but not + // for a read-modify-write. protected async readSessionVariables( sessionId: string, ): Promise> { diff --git a/src/modules/temporal/activities/nodes/conditional.node.ts b/src/modules/temporal/activities/nodes/conditional.node.ts index dcc9ff2..2ed5ec6 100644 --- a/src/modules/temporal/activities/nodes/conditional.node.ts +++ b/src/modules/temporal/activities/nodes/conditional.node.ts @@ -684,9 +684,8 @@ export class ConditionalNode extends BaseNode { sessionId: string, ): Promise> { try { - // EVO-1840: the read itself now lives in BaseNode (it was duplicated here - // and in set-variable.node.ts); the degrade-to-{} policy below stays local - // because it is specific to condition evaluation. + // The read lives in BaseNode; the degrade-to-{} below is policy specific + // to condition evaluation. return await this.readSessionVariables(sessionId); } catch (error: any) { // EVO-1913: surface the failure at ERROR level instead of swallowing it diff --git a/src/modules/temporal/activities/nodes/set-variable.node.spec.ts b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts index 87ec342..f1196fe 100644 --- a/src/modules/temporal/activities/nodes/set-variable.node.spec.ts +++ b/src/modules/temporal/activities/nodes/set-variable.node.spec.ts @@ -1,10 +1,8 @@ import { SetVariableNode, SetVariableNodeInput } from './set-variable.node'; -// EVO-1840: the Set Variable node offers Increase/Decrease in the UI but the -// runtime used to ignore `operation` and do a plain SET, so increments never -// accumulated. These lock the arithmetic and — just as important for this card's -// silent-success family (EVO-1740) — every way the operation can NOT be honored -// must surface as a visible failure instead of quietly writing a wrong number. +// EVO-1840: lock the increase/decrease arithmetic, and — just as important — +// that every way it cannot be honored surfaces as a visible failure instead of +// quietly writing a wrong number. describe('SetVariableNode', () => { let node: SetVariableNode; @@ -85,8 +83,6 @@ describe('SetVariableNode', () => { }); it('resolves a {{variable}} amount against the session before parsing', async () => { - // the panel's Amount field has a variable picker, and the executor passes - // nodeData raw — the node must interpolate or a valid config would abort. stubSession({ lead_score: 10, bonus: 5 }); const result = await node.execute( input({ @@ -145,8 +141,6 @@ describe('SetVariableNode', () => { }); }); - // AC#3 / EVO-1740: an operation that cannot be honored fails visibly. Every - // case below used to (or would) write a wrong value and report success. describe('visible failure instead of a silent wrong write', () => { it('a non-numeric amount fails visibly', async () => { stubSession({ lead_score: 10 }); @@ -158,7 +152,7 @@ describe('SetVariableNode', () => { }); it('an empty amount fails instead of incrementing by 0', async () => { - // Number('') === 0, so this used to be a silent no-op reported as success + // Number('') === 0, which would read as a valid amount stubSession({ lead_score: 10 }); const result = await node.execute( input({ variableName: 'lead_score', operation: 'increase', value: '' }), @@ -197,7 +191,7 @@ describe('SetVariableNode', () => { }); it('a failed session read fails instead of silently rebasing the counter to 0', async () => { - // degrading to {} here would turn lead_score 500 into 40 and report success + // degrading to {} here would turn lead_score 500 into 40 jest .spyOn(node as any, 'readSessionVariables') .mockRejectedValue(new Error('connection refused')); diff --git a/src/modules/temporal/activities/nodes/set-variable.node.ts b/src/modules/temporal/activities/nodes/set-variable.node.ts index 9f19f0e..0a9c6fc 100644 --- a/src/modules/temporal/activities/nodes/set-variable.node.ts +++ b/src/modules/temporal/activities/nodes/set-variable.node.ts @@ -9,14 +9,9 @@ export interface SetVariableNodeInput { nodeData: { variableName?: string; variableValue?: any; - // EVO-1840: the config UI (SetVariablePanel) sends `operation` + `value`; the - // runtime declared neither, so `operation` was silently dropped and every op - // became a plain SET. Declaring them removes the `as any` casts below. - // NOTE: this union models what the panel can SEND, not what the runtime - // honors. Only set / increase / decrease are implemented; clear, now, - // yesterday, tomorrow, time_of_day and random_id still fall through to the - // plain-SET branch (writing the raw `value`, usually '') — same - // UI-promises-what-the-runtime-drops class as this card, tracked separately. + // What the panel can send, not what the runtime honors: only + // set/increase/decrease are implemented, the rest fall through to a plain + // SET (tracked separately). operation?: | 'set' | 'clear' @@ -58,9 +53,7 @@ export class SetVariableNode extends BaseNode { const isArithmetic = operation === 'increase' || operation === 'decrease'; - // EVO-1840: increase/decrease is a read-modify-write, so it needs the - // session's current values. Read once, and only when an arithmetic - // operation is actually configured (a plain SET must not touch the DB). + // increase/decrease is a read-modify-write; a plain SET must not hit the DB. const sessionVariables = isArithmetic ? await this.loadSessionVariables(input.sessionId) : {}; @@ -95,10 +88,8 @@ export class SetVariableNode extends BaseNode { input.nodeData.variables && Array.isArray(input.nodeData.variables) ) { - // Multiple variables. EVO-1840: the array form carries the same - // node-level `operation`, so it gets the same arithmetic — otherwise - // increase/decrease would keep silently degrading to a plain SET on this - // input shape, which is the exact bug this card fixes. + // Multiple variables — the array shape carries the same node-level + // `operation`, so it gets the same arithmetic. for (const variable of input.nodeData.variables) { const cleanName = String(variable.name).replace(/^\{\{|\}\}$/g, ''); @@ -166,16 +157,11 @@ export class SetVariableNode extends BaseNode { return this.createSuccessResult(input, executionTime, variables); }) .catch((error) => { - // executionTime is a DURATION everywhere else (it feeds - // logNodeExecution/trackNodeExecution); this branch used to report - // Date.now(), i.e. an epoch timestamp, as the node's duration. + // Elapsed, not Date.now(): this feeds the node telemetry as a duration. return this.createErrorResult(error, Date.now() - startTime); }); } - // EVO-1840: apply the numeric operation the UI offers. The runtime used to - // ignore `operation` and do a plain SET, so "increase lead_score by 40" never - // accumulated. private applyArithmetic( name: string, rawAmount: any, @@ -183,10 +169,8 @@ export class SetVariableNode extends BaseNode { sessionVariables: Record, input: SetVariableNodeInput, ): number { - // The panel's Amount field is a VariableInput WITH a variable picker, and - // the executor hands the node its raw nodeData (no interpolation upstream), - // so `{{bonus}}` arrives literal. Resolve it against the session before - // parsing — otherwise a UI-supported config would abort the whole journey. + // The Amount field accepts {{variables}} and the executor passes nodeData + // raw, so resolve against the session before parsing. const resolvedAmount = this.processVariableValue(rawAmount, { ...sessionVariables, contactId: input.contactId, @@ -196,7 +180,6 @@ export class SetVariableNode extends BaseNode { const delta = this.toFiniteNumber(resolvedAmount); if (delta === null) { - // EVO-1740 family: fail visibly instead of silently no-op'ing. throw new Error( `Set Variable ${operation} requires a numeric amount, got ${JSON.stringify( rawAmount, @@ -213,10 +196,8 @@ export class SetVariableNode extends BaseNode { return operation === 'increase' ? base + delta : base - delta; } - // An unset variable legitimately starts at 0 (the first increment lands on the - // delta itself). A variable that HOLDS a non-numeric value is different: there - // is no sane arithmetic for it, and rebasing to 0 would silently destroy the - // stored value while reporting success — AC#3 wants that visible. + // Unset starts at 0; a variable that holds a non-numeric value has no sane + // arithmetic and must not be rebased to 0, which would destroy it. private resolveArithmeticBase( name: string, prior: any, @@ -238,10 +219,8 @@ export class SetVariableNode extends BaseNode { return parsed; } - // Number('') and Number(null) are both 0, which would turn an empty/absent - // Amount into a silent "increase by 0" reported as success (the panel even - // renders 1 as the placeholder in that state). Treat "no value" — and - // booleans, which Number() happily coerces — as not-a-number. + // Number('') and Number(null) are 0, so "no value" would read as a valid + // amount; booleans coerce too. Treat all of them as not-a-number. private toFiniteNumber(value: any): number | null { if (value === undefined || value === null || value === '') { return null; @@ -256,12 +235,8 @@ export class SetVariableNode extends BaseNode { return Number.isFinite(parsed) ? parsed : null; } - // EVO-1840: read the session's current variables so increase/decrease can - // apply arithmetic to the prior value. Deliberately NO catch here: unlike - // conditional.node.ts (which degrades to {} so evaluation continues), a failed - // read on a read-modify-write would rebase the counter to 0 and silently - // clobber the accumulated value (lead_score 500 → 40) while reporting success - // — the very silent-success class this card fixes (EVO-1740). Let it throw. + // No catch on purpose: degrading to {} would rebase the counter to 0 and + // write a wrong value as success. private async loadSessionVariables( sessionId: string, ): Promise> { From e0f3b4f2bcbc337c5e3f9ce39d460dd0630ace3b Mon Sep 17 00:00:00 2001 From: Matheus Pastorini Date: Thu, 20 Aug 2026 02:32:27 -0300 Subject: [PATCH 06/29] fix(CRM-209): call the flat message_templates endpoint from the journey/campaign node The EVO-1716 cutover removed the inbox-nested GET route (/api/v1/inboxes/:inbox_id/message_templates) and moved template listing to the flat /api/v1/message_templates?inbox_id=... endpoint. Frontend, controller, policy and the CRM's message_templates_service_token_spec were updated, but CrmClientService in evo-flow still called the removed nested route -> 404 -> resolveTemplate returned null -> a send-message node in messageMode: 'template' silently skipped the send. So every journey/campaign template message stopped being delivered. Fix: getInboxMessageTemplates now hits the flat endpoint with inbox_id as a query param. The existing resolveTemplate parsing (Array.isArray(raw?.data)) already matches the flat endpoint's success envelope, so nothing else changes. QA (jest + tsc): 3 suites / 40 tests green incl. a new crm-client.service.spec contract test that pins the flat URL (same pattern as the EVO-1272 moveToPipelineStage guard, which a mocked-client node spec cannot catch). Red counter-check: reverting to the nested URL fails that test (received the removed /inboxes/:id/... path). typecheck clean. Co-Authored-By: Claude Opus 4.8 --- .../crm-client/crm-client.service.spec.ts | 29 +++++++++++++++++++ src/shared/crm-client/crm-client.service.ts | 7 ++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/shared/crm-client/crm-client.service.spec.ts b/src/shared/crm-client/crm-client.service.spec.ts index 011e63a..00da372 100644 --- a/src/shared/crm-client/crm-client.service.spec.ts +++ b/src/shared/crm-client/crm-client.service.spec.ts @@ -383,6 +383,35 @@ describe('CrmClientService', () => { }); }); + // CRM-209: pins the HTTP contract the Journey/Campaign template node depends on. + // The EVO-1716 cutover removed the inbox-nested GET route; getInboxMessageTemplates + // must call the FLAT endpoint with inbox_id as a query param. Mocking the client + // method in the node spec can't catch a URL drift — this can (the nested URL now + // 404s → resolveTemplate sees success:false → the node silently skips the send). + describe('getInboxMessageTemplates — Journey/Campaign template node contract', () => { + it('GETs the flat /message_templates?inbox_id=... endpoint, not the removed nested route', async () => { + fetchMock.mockResolvedValueOnce( + buildFetchResponse({ + status: 200, + body: { success: true, data: [{ id: 'tpl-1', name: 'welcome' }] }, + }), + ); + + const result = await service.getInboxMessageTemplates('inbox-1'); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe( + 'http://crm-test.local/api/v1/message_templates?inbox_id=inbox-1&active=true&per_page=-1', + ); + expect(init.method).toBe('GET'); + // Guard against the removed EVO-1716 nested route re-appearing. + expect(url).not.toContain('/inboxes/inbox-1/message_templates'); + // Envelope: templates land under data (data.data at the node); resolveTemplate reads it. + expect(result.success).toBe(true); + expect(result.data.data[0].id).toBe('tpl-1'); + }); + }); + describe('auth headers', () => { it('uses X-Service-Token header by default (s2s)', async () => { fetchMock.mockResolvedValueOnce( diff --git a/src/shared/crm-client/crm-client.service.ts b/src/shared/crm-client/crm-client.service.ts index fa275fd..e23607f 100644 --- a/src/shared/crm-client/crm-client.service.ts +++ b/src/shared/crm-client/crm-client.service.ts @@ -840,7 +840,12 @@ export class CrmClientService { async getInboxMessageTemplates( inboxId: string, ): Promise> { - const url = `${this.baseURL}/api/v1/inboxes/${inboxId}/message_templates?active=true&per_page=-1`; + // CRM-209: the EVO-1716 cutover removed the inbox-nested GET route; the flat + // endpoint serves it with an inbox_id filter (proven by the CRM's + // message_templates_service_token_spec). The nested URL now 404s, so + // resolveTemplate saw `success:false` and the journey/campaign template node + // silently skipped the send. + const url = `${this.baseURL}/api/v1/message_templates?inbox_id=${inboxId}&active=true&per_page=-1`; return this.executeRequest( url, { method: 'GET' }, From 4e8d523c13e2427ebcd36a6835949b84639beeba Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Thu, 20 Aug 2026 09:46:45 -0300 Subject: [PATCH 07/29] fix(segments): escape user-controlled values in ClickHouse SQL builder (CRM-60) SegmentClickHouseQueryBuilderService interpolated segment definition values (customAttributeName, value, prop.path, labelId, templateId) directly into ClickHouse string literals, with no escaping. A value containing a single quote breaks out of the literal inside the INSERT INTO ... SELECT that computes segment membership. Route every user-controlled string through the existing (previously unused) SegmentQueryUtils.sanitizeStringValue helper before interpolation, and coerce numeric comparison operands through Number()/Number.isFinite instead of splicing raw text outside quotes. --- ...egment-clickhouse-query-builder.service.ts | 184 +++++++++++------- .../segment-computation.live-path.spec.ts | 97 +++++++++ 2 files changed, 207 insertions(+), 74 deletions(-) diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index b5b1750..17f141a 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -7,6 +7,7 @@ import { OrSegmentNode, } from '../entities/segment.entity'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; +import { SegmentQueryUtils } from '../utils/segment-query.utils'; interface StateSubQuery { stateId: string; @@ -34,6 +35,25 @@ export class SegmentClickHouseQueryBuilderService { SegmentClickHouseQueryBuilderService.name, ); + /** + * Escapes a user-controlled string for safe interpolation inside a SQL + * string literal (single-quoted). + */ + private escapeSql(value: unknown): string { + return SegmentQueryUtils.sanitizeStringValue(String(value ?? '')); + } + + /** + * Coerces a user-controlled value to a safe numeric literal for + * interpolation outside of quotes. Non-finite input becomes the SQL + * literal `null`, so the comparison evaluates to NULL/false instead of + * splicing arbitrary text into the query. + */ + private escapeNumeric(value: unknown): string { + const num = Number(value); + return Number.isFinite(num) ? String(num) : 'null'; + } + /** * Convert segment nodes to state sub-queries using modular builders */ @@ -178,10 +198,10 @@ export class SegmentClickHouseQueryBuilderService { // Labels agora usam eventos separados, mas mantemos suporte legado switch (operator) { case 'Contains': - condition = `has(JSONExtractArrayRaw(traits, 'labels'), '"${value}"')`; + condition = `has(JSONExtractArrayRaw(traits, 'labels'), '"${this.escapeSql(value)}"')`; break; case 'NotContains': - condition = `NOT has(JSONExtractArrayRaw(traits, 'labels'), '"${value}"')`; + condition = `NOT has(JSONExtractArrayRaw(traits, 'labels'), '"${this.escapeSql(value)}"')`; break; case 'Exists': condition = `JSONExtractArrayRaw(traits, 'labels') != '[]'`; @@ -190,42 +210,44 @@ export class SegmentClickHouseQueryBuilderService { condition = `JSONExtractArrayRaw(traits, 'labels') = '[]'`; break; default: - condition = `has(JSONExtractArrayRaw(traits, 'labels'), '"${value}"')`; + condition = `has(JSONExtractArrayRaw(traits, 'labels'), '"${this.escapeSql(value)}"')`; } } else { // Para campos string/número - para campos mutáveis, não usar argMax na condição WHERE // A condição inicial será sempre verdadeira e a validação será feita no argMaxValue if (useArgMax) { // Para campos mutáveis: condição sempre verdadeira, validação no argMaxValue - condition = `JSONExtractString(traits, '${extractPath}') != ''`; // Sempre inclui se o campo existe + condition = `JSONExtractString(traits, '${this.escapeSql(extractPath)}') != ''`; // Sempre inclui se o campo existe } else { // Para campos imutáveis: aplicar condição diretamente - const extractFunc = `JSONExtractString(traits, '${extractPath}')`; + const extractFunc = `JSONExtractString(traits, '${this.escapeSql(extractPath)}')`; + const escapedValue = this.escapeSql(value); + const numericValue = this.escapeNumeric(value); switch (operator) { case 'Equals': - condition = `${extractFunc} = '${value}'`; + condition = `${extractFunc} = '${escapedValue}'`; break; case 'NotEquals': - condition = `${extractFunc} != '${value}'`; + condition = `${extractFunc} != '${escapedValue}'`; break; case 'Contains': - condition = `${extractFunc} LIKE '%${value}%'`; + condition = `${extractFunc} LIKE '%${escapedValue}%'`; break; case 'NotContains': - condition = `${extractFunc} NOT LIKE '%${value}%'`; + condition = `${extractFunc} NOT LIKE '%${escapedValue}%'`; break; case 'GreaterThan': - condition = `toFloat64OrNull(${extractFunc}) > ${value}`; + condition = `toFloat64OrNull(${extractFunc}) > ${numericValue}`; break; case 'GreaterThanOrEqual': - condition = `toFloat64OrNull(${extractFunc}) >= ${value}`; + condition = `toFloat64OrNull(${extractFunc}) >= ${numericValue}`; break; case 'LessThan': - condition = `toFloat64OrNull(${extractFunc}) < ${value}`; + condition = `toFloat64OrNull(${extractFunc}) < ${numericValue}`; break; case 'LessThanOrEqual': - condition = `toFloat64OrNull(${extractFunc}) <= ${value}`; + condition = `toFloat64OrNull(${extractFunc}) <= ${numericValue}`; break; case 'Exists': condition = `${extractFunc} != ''`; @@ -240,7 +262,7 @@ export class SegmentClickHouseQueryBuilderService { } } else { // Sem operador, apenas verifica existência - condition = `JSONExtractString(traits, '${extractPath}') != ''`; + condition = `JSONExtractString(traits, '${this.escapeSql(extractPath)}') != ''`; } // Definir argMaxValue baseado na estratégia @@ -249,13 +271,13 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 ) THEN '' - ELSE JSONExtractString(traits, '${extractPath}') + ELSE JSONExtractString(traits, '${this.escapeSql(extractPath)}') END ` .replace(/\s+/g, ' ') @@ -267,8 +289,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -282,13 +304,13 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 ) THEN '' - ELSE JSONExtractString(traits, '${userPropNode.path}') + ELSE JSONExtractString(traits, '${this.escapeSql(userPropNode.path)}') END ` .replace(/\s+/g, ' ') @@ -304,7 +326,7 @@ export class SegmentClickHouseQueryBuilderService { // `removed` change clears it). generateArgMaxValidation then applies the // operator/value comparison over this argMaxValue. if (isCustomAttribute) { - condition = `event_name = 'contact.custom_attribute.changed' AND JSONExtractString(traits, 'attributeName') = '${customAttributeName}'`; + condition = `event_name = 'contact.custom_attribute.changed' AND JSONExtractString(traits, 'attributeName') = '${this.escapeSql(customAttributeName)}'`; argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( @@ -354,7 +376,7 @@ export class SegmentClickHouseQueryBuilderService { return []; } - let condition = `event_name = '${performedNode.event}'`; + let condition = `event_name = '${this.escapeSql(performedNode.event)}'`; // Adicionar condições de propriedades se houver if (performedNode.properties && performedNode.properties.length > 0) { @@ -362,30 +384,33 @@ export class SegmentClickHouseQueryBuilderService { (prop: any) => { const value = prop.operator?.value || ''; const operator = prop.operator?.type || 'Equals'; + const path = this.escapeSql(prop.path); + const escapedValue = this.escapeSql(value); + const numericValue = this.escapeNumeric(value); switch (operator) { case 'Equals': - return `JSONExtractString(properties, '${prop.path}') = '${value}'`; + return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; case 'NotEquals': - return `JSONExtractString(properties, '${prop.path}') != '${value}'`; + return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; case 'Contains': - return `JSONExtractString(properties, '${prop.path}') LIKE '%${value}%'`; + return `JSONExtractString(properties, '${path}') LIKE '%${escapedValue}%'`; case 'NotContains': - return `JSONExtractString(properties, '${prop.path}') NOT LIKE '%${value}%'`; + return `JSONExtractString(properties, '${path}') NOT LIKE '%${escapedValue}%'`; case 'GreaterThan': - return `toFloat64OrNull(JSONExtractString(properties, '${prop.path}')) > ${value}`; + return `toFloat64OrNull(JSONExtractString(properties, '${path}')) > ${numericValue}`; case 'GreaterThanOrEqual': - return `toFloat64OrNull(JSONExtractString(properties, '${prop.path}')) >= ${value}`; + return `toFloat64OrNull(JSONExtractString(properties, '${path}')) >= ${numericValue}`; case 'LessThan': - return `toFloat64OrNull(JSONExtractString(properties, '${prop.path}')) < ${value}`; + return `toFloat64OrNull(JSONExtractString(properties, '${path}')) < ${numericValue}`; case 'LessThanOrEqual': - return `toFloat64OrNull(JSONExtractString(properties, '${prop.path}')) <= ${value}`; + return `toFloat64OrNull(JSONExtractString(properties, '${path}')) <= ${numericValue}`; case 'Exists': - return `JSONExtractString(properties, '${prop.path}') != ''`; + return `JSONExtractString(properties, '${path}') != ''`; case 'NotExists': - return `JSONExtractString(properties, '${prop.path}') = ''`; + return `JSONExtractString(properties, '${path}') = ''`; default: - return `JSONExtractString(properties, '${prop.path}') = '${value}'`; + return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; } }, ); @@ -395,7 +420,7 @@ export class SegmentClickHouseQueryBuilderService { // Adicionar janela de tempo se especificada if (performedNode.withinSeconds) { - condition += ` AND occurred_at >= now() - INTERVAL ${performedNode.withinSeconds} SECOND`; + condition += ` AND occurred_at >= now() - INTERVAL ${this.escapeNumeric(performedNode.withinSeconds)} SECOND`; } // Para times e timesOperator, precisamos usar uma abordagem diferente para contar @@ -429,8 +454,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -458,7 +483,7 @@ export class SegmentClickHouseQueryBuilderService { return []; } - let condition = `event_name = '${lastPerformedNode.event}'`; + let condition = `event_name = '${this.escapeSql(lastPerformedNode.event)}'`; // Adicionar condições whereProperties se houver if ( @@ -469,20 +494,22 @@ export class SegmentClickHouseQueryBuilderService { (prop: any) => { const value = prop.operator?.value || ''; const operator = prop.operator?.type || 'Equals'; + const path = this.escapeSql(prop.path); + const escapedValue = this.escapeSql(value); switch (operator) { case 'Equals': - return `JSONExtractString(properties, '${prop.path}') = '${value}'`; + return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; case 'NotEquals': - return `JSONExtractString(properties, '${prop.path}') != '${value}'`; + return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; case 'Contains': - return `JSONExtractString(properties, '${prop.path}') LIKE '%${value}%'`; + return `JSONExtractString(properties, '${path}') LIKE '%${escapedValue}%'`; case 'NotContains': - return `JSONExtractString(properties, '${prop.path}') NOT LIKE '%${value}%'`; + return `JSONExtractString(properties, '${path}') NOT LIKE '%${escapedValue}%'`; case 'Exists': - return `JSONExtractString(properties, '${prop.path}') != ''`; + return `JSONExtractString(properties, '${path}') != ''`; default: - return `JSONExtractString(properties, '${prop.path}') = '${value}'`; + return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; } }, ); @@ -498,8 +525,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -529,7 +556,7 @@ export class SegmentClickHouseQueryBuilderService { let condition = `event_name = '${messageType}_sent'`; if (messageNode.templateId) { - condition += ` AND JSONExtractString(properties, 'template_id') = '${messageNode.templateId}'`; + condition += ` AND JSONExtractString(properties, 'template_id') = '${this.escapeSql(messageNode.templateId)}'`; } if (messageNode.event) { @@ -541,7 +568,9 @@ export class SegmentClickHouseQueryBuilderService { MessageClicked: `${messageType}_clicked`, MessageFailed: `${messageType}_failed`, }; - condition = `event_name = '${eventMap[messageNode.event] || messageNode.event}'`; + const resolvedEvent = + eventMap[messageNode.event] ?? this.escapeSql(messageNode.event); + condition = `event_name = '${resolvedEvent}'`; } return [ @@ -551,8 +580,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -574,10 +603,11 @@ export class SegmentClickHouseQueryBuilderService { case SegmentNodeType.RandomBucket: { const bucketNode = node as any; - const percent = bucketNode.percent || 0.5; // Default 50% + const percent = Number(bucketNode.percent); + const safePercent = Number.isFinite(percent) ? percent : 0.5; // Default 50% // Usa hash do contact_or_anonymous_id para distribuição determinista - const condition = `cityHash64(contact_or_anonymous_id) % 100 < ${Math.floor(percent * 100)}`; + const condition = `cityHash64(contact_or_anonymous_id) % 100 < ${Math.floor(safePercent * 100)}`; return [ { @@ -602,8 +632,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -633,18 +663,20 @@ export class SegmentClickHouseQueryBuilderService { return []; } + const labelId = this.escapeSql(labelNode.labelId); + switch (labelNode.condition) { case 'has': // For 'has', check current state using argMax of both add/remove events return [ { stateId, - condition: `(event_name = 'label_added' OR event_name = 'label_removed') AND JSONExtractString(properties, 'labelId') = '${labelNode.labelId}'`, + condition: `(event_name = 'label_added' OR event_name = 'label_removed') AND JSONExtractString(properties, 'labelId') = '${labelId}'`, argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -677,17 +709,17 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 ) THEN 'false' WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE (event_name = 'label_added' OR event_name = 'label_removed') - AND JSONExtractString(properties, 'labelId') = '${labelNode.labelId}' + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events + WHERE (event_name = 'label_added' OR event_name = 'label_removed') + AND JSONExtractString(properties, 'labelId') = '${labelId}' GROUP BY contact_or_anonymous_id HAVING argMax(if(event_name = 'label_added', 'true', 'false'), occurred_at) = 'true' ) THEN 'false' @@ -726,6 +758,8 @@ export class SegmentClickHouseQueryBuilderService { // For CustomAttribute, check current value using argMax of all change events const operator = customAttrNode.operator?.type || 'Equals'; const value = customAttrNode.operator?.value || ''; + const attributeName = this.escapeSql(customAttrNode.attributeName); + const escapedValue = this.escapeSql(value); // For NotEquals, NotContains, and similar "negative" conditions, we need to include ALL contacts // not just those who have custom attribute events @@ -737,8 +771,8 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events WHERE event_name = 'contact_deleted' GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0 @@ -747,7 +781,7 @@ export class SegmentClickHouseQueryBuilderService { SELECT DISTINCT contact_or_anonymous_id FROM contact_events WHERE event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') - AND JSONExtractString(traits, 'attributeName') = '${customAttrNode.attributeName}' + AND JSONExtractString(traits, 'attributeName') = '${attributeName}' GROUP BY contact_or_anonymous_id HAVING argMax( CASE @@ -755,7 +789,7 @@ export class SegmentClickHouseQueryBuilderService { ELSE JSONExtractString(traits, 'attributeValue') END, occurred_at - ) ${operator === 'NotEquals' ? '=' : 'LIKE'} ${operator === 'NotEquals' ? `'${value}'` : `'%${value}%'`} + ) ${operator === 'NotEquals' ? '=' : 'LIKE'} ${operator === 'NotEquals' ? `'${escapedValue}'` : `'%${escapedValue}%'`} ) THEN 'false' ELSE 'true' END @@ -779,7 +813,7 @@ export class SegmentClickHouseQueryBuilderService { // The custom-attribute change is an identify-DTO event: the CRM stores the // canonical dotted name and the payload in the `traits` column, not // `properties` (EVO-1839). Accept both event-name forms; read from traits. - const condition = `event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') AND JSONExtractString(traits, 'attributeName') = '${customAttrNode.attributeName}'`; + const condition = `event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') AND JSONExtractString(traits, 'attributeName') = '${attributeName}'`; // Get the current value using argMax - handle removed attributes as empty const argMaxValue = ` @@ -863,7 +897,9 @@ export class SegmentClickHouseQueryBuilderService { return defaultValidation; } - const { operator, value, extractPath } = subQuery.validationInfo; + const { operator, extractPath } = subQuery.validationInfo; + const value = this.escapeSql(subQuery.validationInfo.value); + const numericValue = this.escapeNumeric(subQuery.validationInfo.value); const baseValue = subQuery.argMaxValue; this.logger.debug( @@ -899,25 +935,25 @@ export class SegmentClickHouseQueryBuilderService { ); return notContainsStringValidation; case 'GreaterThan': - const gtValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) > ${value}, '1', ''), ce.occurred_at)`; + const gtValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) > ${numericValue}, '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string GreaterThan validation for ${subQuery.stateId}: ${gtValidation}`, ); return gtValidation; case 'GreaterThanOrEqual': - const gteValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) >= ${value}, '1', ''), ce.occurred_at)`; + const gteValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) >= ${numericValue}, '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string GreaterThanOrEqual validation for ${subQuery.stateId}: ${gteValidation}`, ); return gteValidation; case 'LessThan': - const ltValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) < ${value}, '1', ''), ce.occurred_at)`; + const ltValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) < ${numericValue}, '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string LessThan validation for ${subQuery.stateId}: ${ltValidation}`, ); return ltValidation; case 'LessThanOrEqual': - const lteValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) <= ${value}, '1', ''), ce.occurred_at)`; + const lteValidation = `argMaxState(if(toFloat64OrNull(${baseValue}) <= ${numericValue}, '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string LessThanOrEqual validation for ${subQuery.stateId}: ${lteValidation}`, ); diff --git a/src/modules/segments/services/segment-computation.live-path.spec.ts b/src/modules/segments/services/segment-computation.live-path.spec.ts index 6bcf27a..8d5425d 100644 --- a/src/modules/segments/services/segment-computation.live-path.spec.ts +++ b/src/modules/segments/services/segment-computation.live-path.spec.ts @@ -134,3 +134,100 @@ describe('EVO-1901 live segment recompute SQL builder', () => { ); }); }); + +describe('segment recompute SQL builder escapes user-controlled values', () => { + const builder = new SegmentClickHouseQueryBuilderService(); + + it('escapes a single quote in a CustomAttribute value instead of splicing it into the SQL', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.CustomAttribute, + attributeName: 'tier', + operator: { type: 'Equals', value: `platinum' OR '1'='1` }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).not.toContain(`platinum' OR '1'='1`); + expect(subQuery.validationInfo?.value).toBe(`platinum' OR '1'='1`); + }); + + it('escapes a single quote in the attributeName used to filter events', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.CustomAttribute, + attributeName: `tier' OR '1'='1`, + operator: { type: 'Equals', value: 'platinum' }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).not.toContain(`tier' OR '1'='1`); + expect(subQuery.condition).toContain(`tier'' OR ''1''=''1`); + }); + + it('escapes a single quote in a Performed event property path and value', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.Performed, + event: 'order_placed', + properties: [ + { + path: `plan' OR '1'='1`, + operator: { type: 'Equals', value: `gold' OR '1'='1` }, + }, + ], + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).not.toContain(`plan' OR '1'='1`); + expect(subQuery.condition).not.toContain(`gold' OR '1'='1`); + }); + + it('escapes a single quote in a Label labelId', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: `vip' OR '1'='1`, + condition: 'has', + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).not.toContain(`vip' OR '1'='1`); + }); + + it('escapes a single quote in a WhatsApp templateId', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.WhatsApp, + templateId: `welcome' OR '1'='1`, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).not.toContain(`welcome' OR '1'='1`); + }); + + it('falls back to a null numeric literal for a non-numeric GreaterThan value, instead of splicing raw text', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.UserProperty, + path: 'leadScore', + operator: { type: 'GreaterThan', value: '0 OR 1=1' }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + const validation = builder.generateArgMaxValidation(subQuery); + + expect(validation).not.toContain('0 OR 1=1'); + expect(validation).toContain('null'); + }); +}); From 1d4fad8fe07d839ebad092029a1ff23e9e0a6eb3 Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Thu, 20 Aug 2026 09:49:45 -0300 Subject: [PATCH 08/29] fix(segments): unify custom attribute query generation, fix NotExists (CRM-60) The CustomAttribute node and the legacy UserProperty customAttributes[.] path each built their own ClickHouse SQL for the same delta-event read, and had already drifted: only the CustomAttribute branch accepted the legacy custom_attribute_changed event name and special-cased NotEquals/NotContains to include contacts with no event for the attribute (a contact who never set it trivially satisfies "not equal to X"). Extract both into a single buildCustomAttributeSubQuery, called by both entry points. While unifying, extend the same "include every contact, then flip on a positive match" handling to NotExists: it previously fell through to the generic per-event condition, so a contact who never triggered the attribute's change event got no row in the state table and was silently excluded from a "does not have this attribute" segment. --- ...egment-clickhouse-query-builder.service.ts | 276 ++++++++++-------- .../segment-computation.live-path.spec.ts | 77 ++++- 2 files changed, 228 insertions(+), 125 deletions(-) diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index 17f141a..2a24cf3 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -182,6 +182,27 @@ export class SegmentClickHouseQueryBuilderService { useArgMax = mutableFields.includes(userPropNode.path); } + if (isCustomAttribute) { + const attributeOperator = userPropNode.operator + ? typeof userPropNode.operator === 'object' + ? userPropNode.operator.type + : userPropNode.operator + : ''; + const attributeValue = userPropNode.operator + ? typeof userPropNode.operator === 'object' + ? String(userPropNode.operator.value || '') + : String(userPropNode.value || '') + : ''; + + return this.buildCustomAttributeSubQuery( + stateId, + segment, + customAttributeName, + attributeOperator, + attributeValue, + ); + } + // Aplicar operador if (userPropNode.operator) { operator = @@ -318,32 +339,6 @@ export class SegmentClickHouseQueryBuilderService { } } - // EVO-1901 (D12): custom attributes are stored as delta events - // (`contact.custom_attribute.changed` with { attributeName, attributeValue, - // changeType }), never as a flat/nested `traits` key — so the generic - // extraction above matches zero rows and segments computed 0 members. Read - // the attribute's change stream instead and argMax the latest value (a - // `removed` change clears it). generateArgMaxValidation then applies the - // operator/value comparison over this argMaxValue. - if (isCustomAttribute) { - condition = `event_name = 'contact.custom_attribute.changed' AND JSONExtractString(traits, 'attributeName') = '${this.escapeSql(customAttributeName)}'`; - argMaxValue = ` - CASE - WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 - ) THEN '' - WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' - ELSE JSONExtractString(traits, 'attributeValue') - END - ` - .replace(/\s+/g, ' ') - .trim(); - } - // Para campos mutáveis, incluir informação do operador e valor para validação posterior const validationInfo = useArgMax ? { @@ -755,101 +750,13 @@ export class SegmentClickHouseQueryBuilderService { return []; } - // For CustomAttribute, check current value using argMax of all change events - const operator = customAttrNode.operator?.type || 'Equals'; - const value = customAttrNode.operator?.value || ''; - const attributeName = this.escapeSql(customAttrNode.attributeName); - const escapedValue = this.escapeSql(value); - - // For NotEquals, NotContains, and similar "negative" conditions, we need to include ALL contacts - // not just those who have custom attribute events - if (operator === 'NotEquals' || operator === 'NotContains') { - return [ - { - stateId, - condition: `1 = 1`, // Include all contacts initially - argMaxValue: ` - CASE - WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 - ) THEN 'false' - WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') - AND JSONExtractString(traits, 'attributeName') = '${attributeName}' - GROUP BY contact_or_anonymous_id - HAVING argMax( - CASE - WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' - ELSE JSONExtractString(traits, 'attributeValue') - END, - occurred_at - ) ${operator === 'NotEquals' ? '=' : 'LIKE'} ${operator === 'NotEquals' ? `'${escapedValue}'` : `'%${escapedValue}%'`} - ) THEN 'false' - ELSE 'true' - END - `, - uniqValue: `contact_or_anonymous_id`, - eventTimeExpression: `occurred_at`, - recordMessageId: false, - joinPriorStateValue: false, - type: 'segment' as const, - computedPropertyId: segment.id, - validationInfo: { - operator: 'Equals', - value: 'true', - extractPath: 'argMax', - }, - }, - ]; - } - - // For positive conditions (Equals, Contains, etc.), use the original logic. - // The custom-attribute change is an identify-DTO event: the CRM stores the - // canonical dotted name and the payload in the `traits` column, not - // `properties` (EVO-1839). Accept both event-name forms; read from traits. - const condition = `event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') AND JSONExtractString(traits, 'attributeName') = '${attributeName}'`; - - // Get the current value using argMax - handle removed attributes as empty - const argMaxValue = ` - CASE - WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 - ) THEN '' - WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' - ELSE JSONExtractString(traits, 'attributeValue') - END - ` - .replace(/\s+/g, ' ') - .trim(); - - return [ - { - stateId, - condition, - argMaxValue, - uniqValue: `message_id`, - eventTimeExpression: `occurred_at`, - recordMessageId: false, - joinPriorStateValue: false, - type: 'segment' as const, - computedPropertyId: segment.id, - validationInfo: { - operator, - value, - extractPath: 'argMax', - }, - }, - ]; + return this.buildCustomAttributeSubQuery( + stateId, + segment, + customAttrNode.attributeName, + customAttrNode.operator?.type || 'Equals', + customAttrNode.operator?.value || '', + ); } case SegmentNodeType.And: @@ -884,6 +791,133 @@ export class SegmentClickHouseQueryBuilderService { } } + /** + * Builds the sub-query for a custom attribute condition, shared by the + * dedicated CustomAttribute node and the legacy UserProperty + * `customAttributes[.]` path. Custom attributes are stored as delta + * events (`contact.custom_attribute.changed`/`custom_attribute_changed` + * with `{ attributeName, attributeValue, changeType }`), never as a flat + * `traits` key, so a contact only gets a row in the state table for + * events matching this attributeName. + * + * NotEquals/NotContains/NotExists are "negative" conditions that must + * also match contacts who never had an event for this attribute — a + * contact with no event trivially satisfies "not equal to X" or "has no + * value". The per-event condition alone can't express that, so these + * three include every contact up front and use a subquery to flip back + * to false the ones for whom the underlying positive comparison holds. + */ + private buildCustomAttributeSubQuery( + stateId: string, + segment: Segment, + attributeName: string, + operator: string, + value: string, + ): StateSubQuery[] { + const escapedAttributeName = this.escapeSql(attributeName); + const escapedValue = this.escapeSql(value); + const negatedOperators = ['NotEquals', 'NotContains', 'NotExists']; + + if (negatedOperators.includes(operator)) { + const currentValueExpr = ` + CASE + WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' + ELSE JSONExtractString(traits, 'attributeValue') + END + ` + .replace(/\s+/g, ' ') + .trim(); + + const positiveComparison = + operator === 'NotEquals' + ? `= '${escapedValue}'` + : operator === 'NotContains' + ? `LIKE '%${escapedValue}%'` + : `!= ''`; // NotExists: flip back to false when a current value exists + + return [ + { + stateId, + condition: `1 = 1`, // Include all contacts initially + argMaxValue: ` + CASE + WHEN contact_or_anonymous_id IN ( + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events + WHERE event_name = 'contact_deleted' + GROUP BY contact_or_anonymous_id + HAVING argMax(occurred_at, occurred_at) > 0 + ) THEN 'false' + WHEN contact_or_anonymous_id IN ( + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events + WHERE event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') + AND JSONExtractString(traits, 'attributeName') = '${escapedAttributeName}' + GROUP BY contact_or_anonymous_id + HAVING argMax(${currentValueExpr}, occurred_at) ${positiveComparison} + ) THEN 'false' + ELSE 'true' + END + `, + uniqValue: `contact_or_anonymous_id`, + eventTimeExpression: `occurred_at`, + recordMessageId: false, + joinPriorStateValue: false, + type: 'segment' as const, + computedPropertyId: segment.id, + validationInfo: { + operator: 'Equals', + value: 'true', + extractPath: 'argMax', + }, + }, + ]; + } + + // Positive conditions (Equals, Contains, Exists, etc.): a contact + // without a matching event correctly has no row and is excluded. The + // custom-attribute change is an identify-DTO event: the CRM stores the + // canonical dotted name and the payload in the `traits` column, not + // `properties`. Accept both event-name forms; read from traits. + const condition = `event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') AND JSONExtractString(traits, 'attributeName') = '${escapedAttributeName}'`; + + // Get the current value using argMax - handle removed attributes as empty + const argMaxValue = ` + CASE + WHEN contact_or_anonymous_id IN ( + SELECT DISTINCT contact_or_anonymous_id + FROM contact_events + WHERE event_name = 'contact_deleted' + GROUP BY contact_or_anonymous_id + HAVING argMax(occurred_at, occurred_at) > 0 + ) THEN '' + WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' + ELSE JSONExtractString(traits, 'attributeValue') + END + ` + .replace(/\s+/g, ' ') + .trim(); + + return [ + { + stateId, + condition, + argMaxValue, + uniqValue: `message_id`, + eventTimeExpression: `occurred_at`, + recordMessageId: false, + joinPriorStateValue: false, + type: 'segment' as const, + computedPropertyId: segment.id, + validationInfo: { + operator, + value, + extractPath: 'argMax', + }, + }, + ]; + } + /** * Generate validation for argMax expressions */ diff --git a/src/modules/segments/services/segment-computation.live-path.spec.ts b/src/modules/segments/services/segment-computation.live-path.spec.ts index 8d5425d..419973e 100644 --- a/src/modules/segments/services/segment-computation.live-path.spec.ts +++ b/src/modules/segments/services/segment-computation.live-path.spec.ts @@ -53,10 +53,11 @@ describe('EVO-1901 live segment recompute SQL builder', () => { const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); - // Selects the attribute's change events… - expect(subQuery.condition).toContain( - "event_name = 'contact.custom_attribute.changed'", - ); + // Selects the attribute's change events, accepting both the canonical + // and the legacy event-name form (same as the dedicated CustomAttribute + // node — the two entry points now share one builder)… + expect(subQuery.condition).toContain('contact.custom_attribute.changed'); + expect(subQuery.condition).toContain('custom_attribute_changed'); expect(subQuery.condition).toContain( "JSONExtractString(traits, 'attributeName') = 'tier'", ); @@ -231,3 +232,71 @@ describe('segment recompute SQL builder escapes user-controlled values', () => { expect(validation).toContain('null'); }); }); + +describe('custom attribute sub-query is shared between both entry points', () => { + const builder = new SegmentClickHouseQueryBuilderService(); + + it('generates the same NotEquals sub-query for the CustomAttribute node and the UserProperty path', () => { + const segment = { id: 'seg-1' } as any; + + const [fromCustomAttributeNode] = builder.segmentNodeToStateSubQuery( + segment, + { + id: 'n1', + type: SegmentNodeType.CustomAttribute, + attributeName: 'tier', + operator: { type: 'NotEquals', value: 'platinum' }, + } as any, + ); + const [fromUserPropertyPath] = builder.segmentNodeToStateSubQuery( + segment, + { + id: 'n2', + type: SegmentNodeType.UserProperty, + path: 'customAttributes.tier', + operator: { type: 'NotEquals', value: 'platinum' }, + } as any, + ); + + expect(fromUserPropertyPath.condition).toBe(fromCustomAttributeNode.condition); + expect(fromUserPropertyPath.argMaxValue).toBe( + fromCustomAttributeNode.argMaxValue, + ); + // both include every contact up front (the fix this test locks in)… + expect(fromUserPropertyPath.condition).toBe('1 = 1'); + }); + + it('a NotExists condition includes a contact who never triggered the attribute event', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.CustomAttribute, + attributeName: 'tier', + operator: { type: 'NotExists', value: '' }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + // Doesn't filter contact_events down to just this attribute's events — + // a contact with zero events for it still gets a row and defaults to + // matching (only flipped to non-matching if they currently have a value). + expect(subQuery.condition).toBe('1 = 1'); + expect(subQuery.argMaxValue).toContain("THEN 'false'"); + expect(subQuery.argMaxValue).toContain("ELSE 'true'"); + }); + + it('an Exists condition still only matches contacts with an event (no regression)', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.CustomAttribute, + attributeName: 'tier', + operator: { type: 'Exists', value: '' }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).toContain('contact.custom_attribute.changed'); + expect(subQuery.condition).not.toBe('1 = 1'); + }); +}); From 42d53a434c1b660d33813eb39d36bbe8b5902c38 Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Thu, 20 Aug 2026 10:23:40 -0300 Subject: [PATCH 09/29] style(segments): trim comments to the non-obvious part (CRM-60) --- ...egment-clickhouse-query-builder.service.ts | 41 ++++--------------- .../segment-computation.live-path.spec.ts | 7 ---- 2 files changed, 9 insertions(+), 39 deletions(-) diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index 2a24cf3..14b5840 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -35,20 +35,12 @@ export class SegmentClickHouseQueryBuilderService { SegmentClickHouseQueryBuilderService.name, ); - /** - * Escapes a user-controlled string for safe interpolation inside a SQL - * string literal (single-quoted). - */ private escapeSql(value: unknown): string { return SegmentQueryUtils.sanitizeStringValue(String(value ?? '')); } - /** - * Coerces a user-controlled value to a safe numeric literal for - * interpolation outside of quotes. Non-finite input becomes the SQL - * literal `null`, so the comparison evaluates to NULL/false instead of - * splicing arbitrary text into the query. - */ + // Non-finite input becomes the literal `null` instead of raw text, so an + // unquoted numeric comparison can't be used to splice in arbitrary SQL. private escapeNumeric(value: unknown): string { const num = Number(value); return Number.isFinite(num) ? String(num) : 'null'; @@ -791,22 +783,11 @@ export class SegmentClickHouseQueryBuilderService { } } - /** - * Builds the sub-query for a custom attribute condition, shared by the - * dedicated CustomAttribute node and the legacy UserProperty - * `customAttributes[.]` path. Custom attributes are stored as delta - * events (`contact.custom_attribute.changed`/`custom_attribute_changed` - * with `{ attributeName, attributeValue, changeType }`), never as a flat - * `traits` key, so a contact only gets a row in the state table for - * events matching this attributeName. - * - * NotEquals/NotContains/NotExists are "negative" conditions that must - * also match contacts who never had an event for this attribute — a - * contact with no event trivially satisfies "not equal to X" or "has no - * value". The per-event condition alone can't express that, so these - * three include every contact up front and use a subquery to flip back - * to false the ones for whom the underlying positive comparison holds. - */ + // Shared by the CustomAttribute node and the legacy UserProperty + // customAttributes[.] path. NotEquals/NotContains/NotExists also + // need to match contacts with no event for the attribute (e.g. "not equal + // to X" is trivially true for them), so those three include every contact + // up front and flip back to false via a subquery on the positive match. private buildCustomAttributeSubQuery( stateId: string, segment: Segment, @@ -874,14 +855,10 @@ export class SegmentClickHouseQueryBuilderService { ]; } - // Positive conditions (Equals, Contains, Exists, etc.): a contact - // without a matching event correctly has no row and is excluded. The - // custom-attribute change is an identify-DTO event: the CRM stores the - // canonical dotted name and the payload in the `traits` column, not - // `properties`. Accept both event-name forms; read from traits. + // Positive conditions: a contact with no matching event correctly has + // no row and is excluded. const condition = `event_name IN ('contact.custom_attribute.changed', 'custom_attribute_changed') AND JSONExtractString(traits, 'attributeName') = '${escapedAttributeName}'`; - // Get the current value using argMax - handle removed attributes as empty const argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( diff --git a/src/modules/segments/services/segment-computation.live-path.spec.ts b/src/modules/segments/services/segment-computation.live-path.spec.ts index 419973e..d69d4c3 100644 --- a/src/modules/segments/services/segment-computation.live-path.spec.ts +++ b/src/modules/segments/services/segment-computation.live-path.spec.ts @@ -53,9 +53,6 @@ describe('EVO-1901 live segment recompute SQL builder', () => { const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); - // Selects the attribute's change events, accepting both the canonical - // and the legacy event-name form (same as the dedicated CustomAttribute - // node — the two entry points now share one builder)… expect(subQuery.condition).toContain('contact.custom_attribute.changed'); expect(subQuery.condition).toContain('custom_attribute_changed'); expect(subQuery.condition).toContain( @@ -262,7 +259,6 @@ describe('custom attribute sub-query is shared between both entry points', () => expect(fromUserPropertyPath.argMaxValue).toBe( fromCustomAttributeNode.argMaxValue, ); - // both include every contact up front (the fix this test locks in)… expect(fromUserPropertyPath.condition).toBe('1 = 1'); }); @@ -277,9 +273,6 @@ describe('custom attribute sub-query is shared between both entry points', () => const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); - // Doesn't filter contact_events down to just this attribute's events — - // a contact with zero events for it still gets a row and defaults to - // matching (only flipped to non-matching if they currently have a value). expect(subQuery.condition).toBe('1 = 1'); expect(subQuery.argMaxValue).toContain("THEN 'false'"); expect(subQuery.argMaxValue).toContain("ELSE 'true'"); From 9fe6f83b7445f7c24a22d4d4eea16ab63c7e3eea Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Thu, 20 Aug 2026 15:39:39 -0300 Subject: [PATCH 10/29] fix(segments): fail closed on the remaining user-controlled SQL points (CRM-60) Review follow-up on the escaping pass: timesOperator/times from the Performed node, the node id embedded in state_id, prototype-chain hits on the message event map, and LIKE wildcards in Contains values. --- ...egment-clickhouse-query-builder.service.ts | 47 +++++++++----- .../segment-computation.live-path.spec.ts | 62 +++++++++++++++++++ .../segment-query-execution.service.ts | 14 ++++- 3 files changed, 108 insertions(+), 15 deletions(-) diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index 14b5840..317a4ef 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -46,6 +46,14 @@ export class SegmentClickHouseQueryBuilderService { return Number.isFinite(num) ? String(num) : 'null'; } + // LIKE patterns treat %, _ and \ specially; escape them so a user value + // only ever matches itself as a substring. + private escapeLike(value: unknown): string { + return this.escapeSql( + String(value ?? '').replace(/[\\%_]/g, (ch) => `\\${ch}`), + ); + } + /** * Convert segment nodes to state sub-queries using modular builders */ @@ -235,6 +243,7 @@ export class SegmentClickHouseQueryBuilderService { // Para campos imutáveis: aplicar condição diretamente const extractFunc = `JSONExtractString(traits, '${this.escapeSql(extractPath)}')`; const escapedValue = this.escapeSql(value); + const likeValue = this.escapeLike(value); const numericValue = this.escapeNumeric(value); switch (operator) { @@ -245,10 +254,10 @@ export class SegmentClickHouseQueryBuilderService { condition = `${extractFunc} != '${escapedValue}'`; break; case 'Contains': - condition = `${extractFunc} LIKE '%${escapedValue}%'`; + condition = `${extractFunc} LIKE '%${likeValue}%'`; break; case 'NotContains': - condition = `${extractFunc} NOT LIKE '%${escapedValue}%'`; + condition = `${extractFunc} NOT LIKE '%${likeValue}%'`; break; case 'GreaterThan': condition = `toFloat64OrNull(${extractFunc}) > ${numericValue}`; @@ -373,6 +382,7 @@ export class SegmentClickHouseQueryBuilderService { const operator = prop.operator?.type || 'Equals'; const path = this.escapeSql(prop.path); const escapedValue = this.escapeSql(value); + const likeValue = this.escapeLike(value); const numericValue = this.escapeNumeric(value); switch (operator) { @@ -381,9 +391,9 @@ export class SegmentClickHouseQueryBuilderService { case 'NotEquals': return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; case 'Contains': - return `JSONExtractString(properties, '${path}') LIKE '%${escapedValue}%'`; + return `JSONExtractString(properties, '${path}') LIKE '%${likeValue}%'`; case 'NotContains': - return `JSONExtractString(properties, '${path}') NOT LIKE '%${escapedValue}%'`; + return `JSONExtractString(properties, '${path}') NOT LIKE '%${likeValue}%'`; case 'GreaterThan': return `toFloat64OrNull(JSONExtractString(properties, '${path}')) > ${numericValue}`; case 'GreaterThanOrEqual': @@ -483,6 +493,7 @@ export class SegmentClickHouseQueryBuilderService { const operator = prop.operator?.type || 'Equals'; const path = this.escapeSql(prop.path); const escapedValue = this.escapeSql(value); + const likeValue = this.escapeLike(value); switch (operator) { case 'Equals': @@ -490,9 +501,9 @@ export class SegmentClickHouseQueryBuilderService { case 'NotEquals': return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; case 'Contains': - return `JSONExtractString(properties, '${path}') LIKE '%${escapedValue}%'`; + return `JSONExtractString(properties, '${path}') LIKE '%${likeValue}%'`; case 'NotContains': - return `JSONExtractString(properties, '${path}') NOT LIKE '%${escapedValue}%'`; + return `JSONExtractString(properties, '${path}') NOT LIKE '%${likeValue}%'`; case 'Exists': return `JSONExtractString(properties, '${path}') != ''`; default: @@ -555,8 +566,11 @@ export class SegmentClickHouseQueryBuilderService { MessageClicked: `${messageType}_clicked`, MessageFailed: `${messageType}_failed`, }; - const resolvedEvent = - eventMap[messageNode.event] ?? this.escapeSql(messageNode.event); + // Own-key check: a prototype name like 'toString' must not resolve + // an inherited function into the SQL literal. + const resolvedEvent = Object.hasOwn(eventMap, messageNode.event) + ? eventMap[messageNode.event] + : this.escapeSql(messageNode.event); condition = `event_name = '${resolvedEvent}'`; } @@ -813,7 +827,7 @@ export class SegmentClickHouseQueryBuilderService { operator === 'NotEquals' ? `= '${escapedValue}'` : operator === 'NotContains' - ? `LIKE '%${escapedValue}%'` + ? `LIKE '%${this.escapeLike(value)}%'` : `!= ''`; // NotExists: flip back to false when a current value exists return [ @@ -910,6 +924,7 @@ export class SegmentClickHouseQueryBuilderService { const { operator, extractPath } = subQuery.validationInfo; const value = this.escapeSql(subQuery.validationInfo.value); + const likeValue = this.escapeLike(subQuery.validationInfo.value); const numericValue = this.escapeNumeric(subQuery.validationInfo.value); const baseValue = subQuery.argMaxValue; @@ -934,13 +949,13 @@ export class SegmentClickHouseQueryBuilderService { ); return notEqualsValidation; case 'Contains': - const containsStringValidation = `argMaxState(if(${baseValue} LIKE '%${value}%', '1', ''), ce.occurred_at)`; + const containsStringValidation = `argMaxState(if(${baseValue} LIKE '%${likeValue}%', '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string Contains validation for ${subQuery.stateId}: ${containsStringValidation}`, ); return containsStringValidation; case 'NotContains': - const notContainsStringValidation = `argMaxState(if(${baseValue} NOT LIKE '%${value}%', '1', ''), ce.occurred_at)`; + const notContainsStringValidation = `argMaxState(if(${baseValue} NOT LIKE '%${likeValue}%', '1', ''), ce.occurred_at)`; this.logger.debug( `Generated string NotContains validation for ${subQuery.stateId}: ${notContainsStringValidation}`, ); @@ -994,7 +1009,7 @@ export class SegmentClickHouseQueryBuilderService { /** * Get ClickHouse operator equivalent */ - getClickHouseOperator(operator: string): string { + getClickHouseOperator(operator: string): string | null { const operatorMap: Record = { GreaterThanOrEqual: '>=', GreaterThan: '>', @@ -1004,13 +1019,17 @@ export class SegmentClickHouseQueryBuilderService { NotEquals: '!=', }; - return operatorMap[operator] || operator; + // Fail closed: an unmapped operator is user input and must never reach + // the SQL raw. Callers turn null into a no-match comparison. + return operatorMap[operator] ?? null; } /** * Generate consistent state ID */ generateStateId(segment: Segment, nodeId: string): string { - return `${segment.id}_${nodeId}`; + // node.id comes from the user-authored definition and this id is inlined + // into SQL literals downstream; strip quote/backslash so it can't break one. + return `${segment.id}_${String(nodeId ?? '').replace(/['\\]/g, '')}`; } } diff --git a/src/modules/segments/services/segment-computation.live-path.spec.ts b/src/modules/segments/services/segment-computation.live-path.spec.ts index d69d4c3..8350b8d 100644 --- a/src/modules/segments/services/segment-computation.live-path.spec.ts +++ b/src/modules/segments/services/segment-computation.live-path.spec.ts @@ -293,3 +293,65 @@ describe('custom attribute sub-query is shared between both entry points', () => expect(subQuery.condition).not.toBe('1 = 1'); }); }); + +describe('remaining user-controlled interpolation points fail closed (CRM-60 review)', () => { + const builder = new SegmentClickHouseQueryBuilderService(); + + it('maps known times operators and refuses an unmapped one instead of returning it raw', () => { + expect(builder.getClickHouseOperator('GreaterThanOrEqual')).toBe('>='); + expect(builder.getClickHouseOperator(`= 0 OR 1=1 --`)).toBeNull(); + }); + + it('strips quote and backslash from the node id embedded in the state id', () => { + const segment = { id: 'seg-1' } as any; + + expect(builder.generateStateId(segment, `n1' OR '1'='1`)).toBe( + 'seg-1_n1 OR 1=1', + ); + }); + + it('a RandomBucket percent of 0 selects an empty bucket; a non-numeric one falls back to 50%', () => { + const segment = { id: 'seg-1' } as any; + + const [zeroBucket] = builder.segmentNodeToStateSubQuery(segment, { + id: 'n1', + type: SegmentNodeType.RandomBucket, + percent: 0, + } as any); + const [defaultBucket] = builder.segmentNodeToStateSubQuery(segment, { + id: 'n2', + type: SegmentNodeType.RandomBucket, + percent: 'abc', + } as any); + + expect(zeroBucket.condition).toContain('< 0'); + expect(defaultBucket.condition).toContain('< 50'); + }); + + it('does not resolve prototype properties when mapping a message event name', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.WhatsApp, + event: 'toString', + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).toBe(`event_name = 'toString'`); + }); + + it('escapes LIKE wildcards in a Contains value so % only matches itself', () => { + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.UserProperty, + path: 'plan', + operator: { type: 'Contains', value: '50%' }, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).toContain(`LIKE '%50\\\\%%'`); + }); +}); diff --git a/src/modules/segments/services/segment-query-execution.service.ts b/src/modules/segments/services/segment-query-execution.service.ts index ec2004c..1c0e7fe 100644 --- a/src/modules/segments/services/segment-query-execution.service.ts +++ b/src/modules/segments/services/segment-query-execution.service.ts @@ -65,6 +65,18 @@ export class SegmentQueryExecutionService { } for (const subQuery of subQueryData) { + // Both values come from the user-authored definition: an unmapped + // operator or non-numeric times must fail closed (match nothing), + // never reach the SQL raw. + const countOperator = subQuery.timesOperator + ? this.queryBuilder.getClickHouseOperator(subQuery.timesOperator) + : null; + const expectedTimes = Number(subQuery.expectedTimes); + const countComparison = + countOperator && Number.isFinite(expectedTimes) + ? `event_count ${countOperator} ${expectedTimes}` + : '0'; + const query = subQuery.useCountQuery && subQuery.timesOperator && @@ -77,7 +89,7 @@ export class SegmentQueryExecutionService { state_id, contact_or_anonymous_id, argMaxState( - CASE WHEN event_count ${this.queryBuilder.getClickHouseOperator(subQuery.timesOperator)} ${subQuery.expectedTimes} + CASE WHEN ${countComparison} THEN 'true' ELSE 'false' END, From b8e95d3993067840ea8b479f38c89584e3ef76f6 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Thu, 20 Aug 2026 18:03:51 -0300 Subject: [PATCH 11/29] =?UTF-8?q?refactor(CRM-209):=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20tighten=20comments,=20encode=20inbox=5Fid,=20contex?= =?UTF-8?q?tual=20404=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/shared/crm-client/crm-client.service.spec.ts | 7 ++----- src/shared/crm-client/crm-client.service.ts | 11 ++++------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/shared/crm-client/crm-client.service.spec.ts b/src/shared/crm-client/crm-client.service.spec.ts index 00da372..ff758a3 100644 --- a/src/shared/crm-client/crm-client.service.spec.ts +++ b/src/shared/crm-client/crm-client.service.spec.ts @@ -383,11 +383,8 @@ describe('CrmClientService', () => { }); }); - // CRM-209: pins the HTTP contract the Journey/Campaign template node depends on. - // The EVO-1716 cutover removed the inbox-nested GET route; getInboxMessageTemplates - // must call the FLAT endpoint with inbox_id as a query param. Mocking the client - // method in the node spec can't catch a URL drift — this can (the nested URL now - // 404s → resolveTemplate sees success:false → the node silently skips the send). + // CRM-209: pins the flat-endpoint URL the Journey/Campaign template node depends + // on — node specs mock this client, so only this test catches a URL drift. describe('getInboxMessageTemplates — Journey/Campaign template node contract', () => { it('GETs the flat /message_templates?inbox_id=... endpoint, not the removed nested route', async () => { fetchMock.mockResolvedValueOnce( diff --git a/src/shared/crm-client/crm-client.service.ts b/src/shared/crm-client/crm-client.service.ts index e23607f..adbf387 100644 --- a/src/shared/crm-client/crm-client.service.ts +++ b/src/shared/crm-client/crm-client.service.ts @@ -659,7 +659,7 @@ export class CrmClientService { } if (response.status === 404) { - throw new Error('CRM Resource not found (conversation)'); + throw new Error(`CRM Resource not found (${context.nodeType})`); } if (response.status === 422) { @@ -840,12 +840,9 @@ export class CrmClientService { async getInboxMessageTemplates( inboxId: string, ): Promise> { - // CRM-209: the EVO-1716 cutover removed the inbox-nested GET route; the flat - // endpoint serves it with an inbox_id filter (proven by the CRM's - // message_templates_service_token_spec). The nested URL now 404s, so - // resolveTemplate saw `success:false` and the journey/campaign template node - // silently skipped the send. - const url = `${this.baseURL}/api/v1/message_templates?inbox_id=${inboxId}&active=true&per_page=-1`; + // CRM-209: EVO-1716 removed the inbox-nested GET route — the flat endpoint + // with an inbox_id filter is the only server-side listing left. + const url = `${this.baseURL}/api/v1/message_templates?inbox_id=${encodeURIComponent(inboxId)}&active=true&per_page=-1`; return this.executeRequest( url, { method: 'GET' }, From c16aa8d21a792d0a5c7a3701dc680f4721a614d8 Mon Sep 17 00:00:00 2001 From: Nickolas Oliveira Date: Fri, 21 Aug 2026 12:20:41 -0300 Subject: [PATCH 12/29] =?UTF-8?q?fix(segments):=20casa=20os=20nomes=20can?= =?UTF-8?q?=C3=B4nicos=20de=20evento=20de=20contato=20no=20builder=20e=20n?= =?UTF-8?q?o=20cache=20de=20deletados=20(CRM-215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O CRM emite contact.label.added/removed (labelId em traits) e contact.deleted; o builder filtrava label_added/label_removed lendo properties e contact_deleted, então segmento por etiqueta computava 0 membros e contato excluído seguia contando. - Fonte única dos nomes (canônico + legado) e do subselect de deletados em queries/contact-event-names.ts, reusada pelo builder, pelo cache e pelo regex de reescrita da execução (que dependia do texto literal do CASE). - Remove queries/contact-exclusion-queries.ts (sem chamador). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RYhKfFRYqqdNnFTL7FSVpS --- .../segments/queries/contact-event-names.ts | 42 +++++++ .../queries/contact-exclusion-queries.ts | 76 ------------ .../deleted-contacts-cache.service.ts | 9 +- .../segment-canonical-event-names.spec.ts | 114 ++++++++++++++++++ ...egment-clickhouse-query-builder.service.ts | 84 ++++--------- .../segment-query-execution.service.ts | 5 +- 6 files changed, 185 insertions(+), 145 deletions(-) create mode 100644 src/modules/segments/queries/contact-event-names.ts delete mode 100644 src/modules/segments/queries/contact-exclusion-queries.ts create mode 100644 src/modules/segments/services/segment-canonical-event-names.spec.ts diff --git a/src/modules/segments/queries/contact-event-names.ts b/src/modules/segments/queries/contact-event-names.ts new file mode 100644 index 0000000..999c598 --- /dev/null +++ b/src/modules/segments/queries/contact-event-names.ts @@ -0,0 +1,42 @@ +/** + * Canonical contact event names as emitted by the CRM (`EvoFlow::ContactEventsListener`), + * plus the legacy underscore spellings older producers used. Query builders must accept + * both until the central event-name normalization lands (tracked separately); matching a + * single spelling silently returns zero rows (CRM-215). + */ +export const DELETED_CONTACT_EVENT_NAMES = [ + 'contact.deleted', + 'contact_deleted', +] as const; +export const LABEL_ADDED_EVENT_NAMES = [ + 'contact.label.added', + 'label_added', +] as const; +export const LABEL_REMOVED_EVENT_NAMES = [ + 'contact.label.removed', + 'label_removed', +] as const; + +export function sqlStringList(names: readonly string[]): string { + return names.map((n) => `'${n}'`).join(', '); +} + +/** + * Single source of the "deleted contacts" subselect. Every CASE in the segment SQL builder + * and the deleted-contacts cache embed this exact text, and + * `SegmentQueryExecutionService` rewrites it by regex — keep it one line so the match is stable. + */ +export const DELETED_CONTACTS_SUBQUERY = + `SELECT DISTINCT contact_or_anonymous_id FROM contact_events ` + + `WHERE event_name IN (${sqlStringList(DELETED_CONTACT_EVENT_NAMES)}) ` + + `GROUP BY contact_or_anonymous_id HAVING argMax(occurred_at, occurred_at) > 0`; + +function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** Matches the deleted-contacts CASE branch the builder emits, whitespace-tolerant. */ +export const DELETED_CONTACTS_CASE_BRANCH_REGEX = new RegExp( + `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '[^']*'`, + 'g', +); diff --git a/src/modules/segments/queries/contact-exclusion-queries.ts b/src/modules/segments/queries/contact-exclusion-queries.ts deleted file mode 100644 index a43384e..0000000 --- a/src/modules/segments/queries/contact-exclusion-queries.ts +++ /dev/null @@ -1,76 +0,0 @@ -/** - * Contact exclusion query templates for handling deleted contacts - * These CASE statements check for contact_deleted events to exclude deleted contacts - */ - -export class ContactExclusionQueries { - /** - * Generates CASE statement to exclude deleted contacts. - * @param contactIdAlias - The column reference used for contact_id in the - * outer query (e.g., 'c.contact_id', 'ce.contact_id', 'contact_id'). Must - * be a bare identifier or qualified identifier — no expressions, no - * literals — so it can be safely interpolated into the SQL. - */ - static getDeletedContactExclusion(contactIdAlias: string): string { - if (!/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/.test(contactIdAlias)) { - throw new Error( - `Invalid contactIdAlias: expected a SQL identifier (optionally qualified), got "${contactIdAlias}"`, - ); - } - return ` - CASE - WHEN ( - SELECT COUNT(*) - FROM evo_campaign.contact_events ce_del - WHERE ce_del.contact_id = ${contactIdAlias} - AND ce_del.event_name = 'contact_deleted' - ) > 0 THEN 0 - ELSE 1 - END = 1`; - } - - /** - * Generates argMax subquery to get latest contact state excluding deleted - */ - static getLatestContactStateExclusion(): string { - return ` - argMax( - CASE - WHEN ce.event_name = 'contact_deleted' THEN 0 - ELSE 1 - END, - ce.occurred_at - ) = 1`; - } - - /** - * Common WHERE clause for excluding deleted contacts in event-based queries. - * `contact_id IS NOT NULL` in the subquery is required: a single NULL row - * makes `NOT IN` evaluate to NULL for every contact and silently empties the - * outer result. - */ - static getEventBasedExclusionClause(): string { - return ` - AND contact_id NOT IN ( - SELECT DISTINCT contact_id - FROM evo_campaign.contact_events - WHERE event_name = 'contact_deleted' - AND contact_id IS NOT NULL - )`; - } - - /** - * Generates exclusion for performed/lastPerformed queries. Same - * NULL-safety requirement on the NOT IN subquery as - * getEventBasedExclusionClause. - */ - static getPerformedEventExclusion(): string { - return ` - AND ce.contact_id NOT IN ( - SELECT contact_id - FROM evo_campaign.contact_events - WHERE event_name = 'contact_deleted' - AND contact_id IS NOT NULL - )`; - } -} diff --git a/src/modules/segments/services/deleted-contacts-cache.service.ts b/src/modules/segments/services/deleted-contacts-cache.service.ts index fb98cae..67e7daf 100644 --- a/src/modules/segments/services/deleted-contacts-cache.service.ts +++ b/src/modules/segments/services/deleted-contacts-cache.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@nestjs/common'; +import { DELETED_CONTACTS_SUBQUERY } from '../queries/contact-event-names'; import { ClickHouseService } from '../../processing/clickhouse/clickhouse.service'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; @@ -41,13 +42,7 @@ export class DeletedContactsCacheService { } private async fetchDeletedContactsFromClickHouse(): Promise> { - const query = ` - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 - `; + const query = DELETED_CONTACTS_SUBQUERY; const result = await this.clickhouseService.query({ query }); diff --git a/src/modules/segments/services/segment-canonical-event-names.spec.ts b/src/modules/segments/services/segment-canonical-event-names.spec.ts new file mode 100644 index 0000000..a0f2616 --- /dev/null +++ b/src/modules/segments/services/segment-canonical-event-names.spec.ts @@ -0,0 +1,114 @@ +import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query-builder.service'; +import { SegmentNodeType } from '../entities/segment.entity'; +import { + DELETED_CONTACTS_CASE_BRANCH_REGEX, + DELETED_CONTACTS_SUBQUERY, +} from '../queries/contact-event-names'; + +/** + * CRM-215 — the CRM emits dotted canonical event names (`contact.label.added`, + * `contact.deleted`) with the label id in `traits`; the builder filtered the legacy + * underscore spelling and read `properties`, so Label segments computed 0 members and + * deleted contacts were never excluded. Both spellings are accepted until the central + * normalization lands. + */ +describe('CRM-215 segment SQL matches the canonical contact event names', () => { + const builder = new SegmentClickHouseQueryBuilderService(); + const segment = { id: 'seg-1' } as any; + + it('Label has: filters both label spellings and reads labelId from traits', () => { + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'has', + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.condition).toContain("'contact.label.added'"); + expect(subQuery.condition).toContain("'contact.label.removed'"); + expect(subQuery.condition).toContain("'label_added'"); + expect(subQuery.condition).toContain( + "JSONExtractString(traits, 'labelId') = 'lbl-1'", + ); + expect(subQuery.condition).not.toContain( + "JSONExtractString(properties, 'labelId')", + ); + expect(subQuery.argMaxValue).toContain( + "if(event_name IN ('contact.label.added', 'label_added'), 'true', 'false')", + ); + }); + + it('Label not_has: the exclusion subselect uses the same canonical names and traits', () => { + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'not_has', + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + expect(subQuery.argMaxValue).toContain("'contact.label.added'"); + expect(subQuery.argMaxValue).toContain( + "JSONExtractString(traits, 'labelId') = 'lbl-1'", + ); + expect(subQuery.argMaxValue).not.toContain('properties'); + expect(subQuery.argMaxValue).toContain( + "HAVING argMax(if(event_name IN ('contact.label.added', 'label_added'), 'true', 'false'), occurred_at) = 'true'", + ); + }); + + it('every deleted-contacts guard matches contact.deleted (and the legacy spelling)', () => { + const nodes = [ + { + id: 'n2', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'has', + }, + { + id: 'n3', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'not_has', + }, + { + id: 'n4', + type: SegmentNodeType.UserProperty, + path: 'customAttributes.tier', + operator: { type: 'Equals', value: 'x' }, + value: 'x', + }, + ] as any[]; + + for (const node of nodes) { + const serialized = JSON.stringify( + builder.segmentNodeToStateSubQuery(segment, node), + ); + expect(serialized).toContain( + "event_name IN ('contact.deleted', 'contact_deleted')", + ); + expect(serialized).not.toContain("event_name = 'contact_deleted'"); + } + }); + + it('the execution-time rewrite still recognizes the deleted-contacts CASE branch', () => { + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'has', + } as any; + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + + const rewritten = String(subQuery.argMaxValue).replace( + DELETED_CONTACTS_CASE_BRANCH_REGEX, + `WHEN 1=0 THEN 'false'`, + ); + + expect(rewritten).toContain(`WHEN 1=0 THEN 'false'`); + expect(rewritten).not.toContain(DELETED_CONTACTS_SUBQUERY); + }); +}); diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index 317a4ef..416ac5f 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -8,6 +8,12 @@ import { } from '../entities/segment.entity'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; import { SegmentQueryUtils } from '../utils/segment-query.utils'; +import { + DELETED_CONTACTS_SUBQUERY, + LABEL_ADDED_EVENT_NAMES, + LABEL_REMOVED_EVENT_NAMES, + sqlStringList, +} from '../queries/contact-event-names'; interface StateSubQuery { stateId: string; @@ -293,11 +299,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE JSONExtractString(traits, '${this.escapeSql(extractPath)}') END @@ -311,11 +313,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE toString(occurred_at) END @@ -326,11 +324,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE JSONExtractString(traits, '${this.escapeSql(userPropNode.path)}') END @@ -451,11 +445,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE toString(occurred_at) END @@ -523,11 +513,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE toString(occurred_at) END @@ -581,11 +567,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' ELSE toString(occurred_at) END @@ -633,11 +615,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN 'false' ELSE 'true' END @@ -665,6 +643,8 @@ export class SegmentClickHouseQueryBuilderService { } const labelId = this.escapeSql(labelNode.labelId); + const LABEL_ADDED_IN = sqlStringList(LABEL_ADDED_EVENT_NAMES); + const LABEL_EVENTS_IN = sqlStringList([...LABEL_ADDED_EVENT_NAMES, ...LABEL_REMOVED_EVENT_NAMES]); switch (labelNode.condition) { case 'has': @@ -672,17 +652,13 @@ export class SegmentClickHouseQueryBuilderService { return [ { stateId, - condition: `(event_name = 'label_added' OR event_name = 'label_removed') AND JSONExtractString(properties, 'labelId') = '${labelId}'`, + condition: `event_name IN (${LABEL_EVENTS_IN}) AND JSONExtractString(traits, 'labelId') = '${labelId}'`, argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN 'false' - ELSE if(event_name = 'label_added', 'true', 'false') + ELSE if(event_name IN (${LABEL_ADDED_IN}), 'true', 'false') END `, uniqValue: `message_id`, @@ -710,19 +686,15 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN 'false' WHEN contact_or_anonymous_id IN ( SELECT DISTINCT contact_or_anonymous_id FROM contact_events - WHERE (event_name = 'label_added' OR event_name = 'label_removed') - AND JSONExtractString(properties, 'labelId') = '${labelId}' + WHERE event_name IN (${LABEL_EVENTS_IN}) + AND JSONExtractString(traits, 'labelId') = '${labelId}' GROUP BY contact_or_anonymous_id - HAVING argMax(if(event_name = 'label_added', 'true', 'false'), occurred_at) = 'true' + HAVING argMax(if(event_name IN (${LABEL_ADDED_IN}), 'true', 'false'), occurred_at) = 'true' ) THEN 'false' ELSE 'true' END @@ -837,11 +809,7 @@ export class SegmentClickHouseQueryBuilderService { argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN 'false' WHEN contact_or_anonymous_id IN ( SELECT DISTINCT contact_or_anonymous_id @@ -876,11 +844,7 @@ export class SegmentClickHouseQueryBuilderService { const argMaxValue = ` CASE WHEN contact_or_anonymous_id IN ( - SELECT DISTINCT contact_or_anonymous_id - FROM contact_events - WHERE event_name = 'contact_deleted' - GROUP BY contact_or_anonymous_id - HAVING argMax(occurred_at, occurred_at) > 0 + ${DELETED_CONTACTS_SUBQUERY} ) THEN '' WHEN JSONExtractString(traits, 'changeType') = 'removed' THEN '' ELSE JSONExtractString(traits, 'attributeValue') diff --git a/src/modules/segments/services/segment-query-execution.service.ts b/src/modules/segments/services/segment-query-execution.service.ts index 1c0e7fe..780bb21 100644 --- a/src/modules/segments/services/segment-query-execution.service.ts +++ b/src/modules/segments/services/segment-query-execution.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@nestjs/common'; import { ClickHouseService } from '../../processing/clickhouse/clickhouse.service'; import { Segment } from '../entities/segment.entity'; import { DeletedContactsCacheService } from './deleted-contacts-cache.service'; +import { DELETED_CONTACTS_CASE_BRANCH_REGEX } from '../queries/contact-event-names'; import { SegmentMetricsService } from '../metrics/segment-metrics.service'; import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query-builder.service'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; @@ -183,7 +184,7 @@ export class SegmentQueryExecutionService { if (deletedContacts.size === 0) { return query.replace( - /WHEN contact_or_anonymous_id IN \([^)]*SELECT[^)]*contact_deleted[^)]*\) THEN '[^']*'/g, + DELETED_CONTACTS_CASE_BRANCH_REGEX, `WHEN 1=0 THEN 'false'`, ); } @@ -194,7 +195,7 @@ export class SegmentQueryExecutionService { const deletedContactsList = deletedContactsArray.join(','); const optimizedQuery = query.replace( - /WHEN contact_or_anonymous_id IN \([^)]*SELECT[^)]*contact_deleted[^)]*\) THEN '[^']*'/g, + DELETED_CONTACTS_CASE_BRANCH_REGEX, `WHEN contact_or_anonymous_id IN (${deletedContactsList}) THEN 'false'`, ); From 630507f36c84ee7f759564ed497e28f1b6d16ae8 Mon Sep 17 00:00:00 2001 From: Nickolas Oliveira Date: Fri, 21 Aug 2026 17:18:14 -0300 Subject: [PATCH 13/29] =?UTF-8?q?fix(segments):=20exclus=C3=A3o=20de=20con?= =?UTF-8?q?tato=20n=C3=A3o=20depende=20mais=20do=20cache=20de=20deletados?= =?UTF-8?q?=20estar=20quente=20(CRM-215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O recompute é incremental e o cache de deletados tem TTL de 5 min: se a única janela que continha o contact.deleted rodava com o cache vazio, o CASE virava WHEN 1=0 e o contato seguia no segmento até um recálculo completo. - Cache vazio mantém o subselect real (otimização nunca substitui correção). - A API de eventos sinaliza a ingestão de contact.deleted via EventEmitter (sem dependência de módulo Nest; só constantes compartilhadas) e o cache invalida o snapshot e ignora o cache por 15 s — a ingestão no ClickHouse é assíncrona e um fetch imediato re-cachearia um conjunto ainda sem a exclusão. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RYhKfFRYqqdNnFTL7FSVpS --- .../events.service.contact-deleted.spec.ts | 38 ++++++++++ src/modules/events/events.service.ts | 20 ++++- .../segments/queries/contact-event-names.ts | 28 +++++++ .../deleted-contacts-cache.service.ts | 23 +++++- .../segment-canonical-event-names.spec.ts | 76 +++++++++++++++++++ .../segment-query-execution.service.ts | 24 ++---- 6 files changed, 189 insertions(+), 20 deletions(-) create mode 100644 src/modules/events/events.service.contact-deleted.spec.ts diff --git a/src/modules/events/events.service.contact-deleted.spec.ts b/src/modules/events/events.service.contact-deleted.spec.ts new file mode 100644 index 0000000..f8682ab --- /dev/null +++ b/src/modules/events/events.service.contact-deleted.spec.ts @@ -0,0 +1,38 @@ +import { EventsService } from './events.service'; +import { CONTACT_DELETED_INGESTED_EVENT } from '../segments/queries/contact-event-names'; + +/** + * CRM-215 — ingesting a deleted-contact event must tell the deleted-contacts cache to + * drop its snapshot, otherwise the next incremental recompute can evaluate the deletion + * window with a stale empty cache and keep the contact in every segment. + */ +describe('EventsService.identifyEvent (contact deleted signal)', () => { + const processing = { + processEvent: jest + .fn() + .mockResolvedValue({ messageId: 'm1', status: 'ok' }), + }; + const emitter = { emit: jest.fn() }; + const service = new EventsService(processing as any, emitter as any); + + beforeEach(() => emitter.emit.mockClear()); + + it.each(['contact.deleted', 'contact_deleted'])( + 'emits the signal for %s', + async (eventName) => { + await service.identifyEvent({ contactId: 'c-1', eventName } as any); + expect(emitter.emit).toHaveBeenCalledWith( + CONTACT_DELETED_INGESTED_EVENT, + { contactId: 'c-1' }, + ); + }, + ); + + it('stays quiet for any other identify event', async () => { + await service.identifyEvent({ + contactId: 'c-1', + eventName: 'contact.updated', + } as any); + expect(emitter.emit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/modules/events/events.service.ts b/src/modules/events/events.service.ts index dcc60d4..7bf7416 100644 --- a/src/modules/events/events.service.ts +++ b/src/modules/events/events.service.ts @@ -1,4 +1,9 @@ import { Injectable, BadRequestException } from '@nestjs/common'; +import { EventEmitter2 } from '@nestjs/event-emitter'; +import { + CONTACT_DELETED_INGESTED_EVENT, + DELETED_CONTACT_EVENT_NAMES, +} from '../segments/queries/contact-event-names'; import { TrackEventDto, IdentifyEventDto, @@ -15,7 +20,10 @@ import { CustomLoggerService } from 'src/common/services/custom-logger.service'; export class EventsService { private readonly logger = new CustomLoggerService(EventsService.name); - constructor(private processingService: ProcessingService) {} + constructor( + private processingService: ProcessingService, + private readonly eventEmitter: EventEmitter2, + ) {} async trackEvent( dto: TrackEventDto, @@ -79,6 +87,16 @@ export class EventsService { throw new BadRequestException(result.error); } + if ( + (DELETED_CONTACT_EVENT_NAMES as readonly string[]).includes( + eventData.eventName ?? '', + ) + ) { + this.eventEmitter.emit(CONTACT_DELETED_INGESTED_EVENT, { + contactId: dto.contactId, + }); + } + return { messageId: result.messageId, status: result.status }; } diff --git a/src/modules/segments/queries/contact-event-names.ts b/src/modules/segments/queries/contact-event-names.ts index 999c598..67de2ae 100644 --- a/src/modules/segments/queries/contact-event-names.ts +++ b/src/modules/segments/queries/contact-event-names.ts @@ -40,3 +40,31 @@ export const DELETED_CONTACTS_CASE_BRANCH_REGEX = new RegExp( `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '[^']*'`, 'g', ); + +/** + * In-process signal emitted by the events API when a deleted-contact event is ingested, + * so the deleted-contacts cache drops its snapshot before the next segment recompute. + * Without it the incremental recompute could evaluate the deletion window with a stale + * (empty) cache and keep the contact assigned until a full recompute (CRM-215). + */ +export const CONTACT_DELETED_INGESTED_EVENT = + 'segments.contact-deleted.ingested'; + +/** + * Replaces the deleted-contacts subselect in a state query with the cached id list. + * It is an optimization only: with an EMPTY cache the real subselect is kept, because + * "no deleted contacts cached" is not the same as "no deleted contacts". + */ +export function applyDeletedContactsOptimization( + query: string, + deletedContacts: ReadonlySet, +): string { + if (deletedContacts.size === 0) return query; + const list = Array.from(deletedContacts) + .map((id) => `'${id.replace(/'/g, "''")}'`) + .join(','); + return query.replace( + DELETED_CONTACTS_CASE_BRANCH_REGEX, + `WHEN contact_or_anonymous_id IN (${list}) THEN 'false'`, + ); +} diff --git a/src/modules/segments/services/deleted-contacts-cache.service.ts b/src/modules/segments/services/deleted-contacts-cache.service.ts index 67e7daf..c90cd31 100644 --- a/src/modules/segments/services/deleted-contacts-cache.service.ts +++ b/src/modules/segments/services/deleted-contacts-cache.service.ts @@ -1,5 +1,9 @@ import { Injectable } from '@nestjs/common'; -import { DELETED_CONTACTS_SUBQUERY } from '../queries/contact-event-names'; +import { OnEvent } from '@nestjs/event-emitter'; +import { + CONTACT_DELETED_INGESTED_EVENT, + DELETED_CONTACTS_SUBQUERY, +} from '../queries/contact-event-names'; import { ClickHouseService } from '../../processing/clickhouse/clickhouse.service'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; @@ -12,13 +16,19 @@ export class DeletedContactsCacheService { private cached: Set | null = null; private expiresAt: number = 0; private readonly CACHE_TTL = 300000; // 5 minutes + // Ingest → Kafka → ClickHouse MV is async: a fetch right after the deleted-contact + // signal may not see the row yet and would re-cache a stale set for CACHE_TTL. During + // this window every call queries ClickHouse and nothing is cached (CRM-215). + private readonly BYPASS_AFTER_DELETE_MS = 15000; + private bypassCacheUntil = 0; constructor(private readonly clickhouseService: ClickHouseService) {} async getDeletedContacts(): Promise> { const now = Date.now(); + const bypass = now < this.bypassCacheUntil; - if (this.cached && now < this.expiresAt) { + if (!bypass && this.cached && now < this.expiresAt) { this.logger.debug('Deleted contacts cache hit'); return this.cached; } @@ -28,7 +38,8 @@ export class DeletedContactsCacheService { try { const deletedContacts = await this.fetchDeletedContactsFromClickHouse(); this.cached = deletedContacts; - this.expiresAt = now + this.CACHE_TTL; + // A set fetched inside the bypass window may be incomplete: let it expire with the window. + this.expiresAt = bypass ? this.bypassCacheUntil : now + this.CACHE_TTL; this.logger.debug(`Cached ${deletedContacts.size} deleted contacts`); return deletedContacts; } catch (error) { @@ -56,6 +67,12 @@ export class DeletedContactsCacheService { return deletedContacts; } + @OnEvent(CONTACT_DELETED_INGESTED_EVENT) + onContactDeletedIngested(): void { + this.invalidateCache(); + this.bypassCacheUntil = Date.now() + this.BYPASS_AFTER_DELETE_MS; + } + invalidateCache(): void { this.cached = null; this.expiresAt = 0; diff --git a/src/modules/segments/services/segment-canonical-event-names.spec.ts b/src/modules/segments/services/segment-canonical-event-names.spec.ts index a0f2616..abac9fd 100644 --- a/src/modules/segments/services/segment-canonical-event-names.spec.ts +++ b/src/modules/segments/services/segment-canonical-event-names.spec.ts @@ -1,9 +1,11 @@ import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query-builder.service'; import { SegmentNodeType } from '../entities/segment.entity'; import { + applyDeletedContactsOptimization, DELETED_CONTACTS_CASE_BRANCH_REGEX, DELETED_CONTACTS_SUBQUERY, } from '../queries/contact-event-names'; +import { DeletedContactsCacheService } from './deleted-contacts-cache.service'; /** * CRM-215 — the CRM emits dotted canonical event names (`contact.label.added`, @@ -112,3 +114,77 @@ describe('CRM-215 segment SQL matches the canonical contact event names', () => expect(rewritten).not.toContain(DELETED_CONTACTS_SUBQUERY); }); }); + +describe('CRM-215 deleted-contacts cache never trades correctness for speed', () => { + const builder = new SegmentClickHouseQueryBuilderService(); + const segment = { id: 'seg-1' } as any; + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: 'lbl-1', + condition: 'not_has', + } as any; + + it('keeps the real subselect when the cache is empty (empty cache ≠ no deleted contacts)', () => { + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + const sql = String(subQuery.argMaxValue); + + const out = applyDeletedContactsOptimization(sql, new Set()); + + expect(out).toBe(sql); + expect(out).toContain(DELETED_CONTACTS_SUBQUERY); + expect(out).not.toContain('WHEN 1=0'); + }); + + it('inlines the cached ids (escaped) when the cache has entries', () => { + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + const sql = String(subQuery.argMaxValue); + + const out = applyDeletedContactsOptimization( + sql, + new Set(['c-1', "x' OR '1'='1"]), + ); + + expect(out).not.toContain(DELETED_CONTACTS_SUBQUERY); + expect(out).toContain( + "WHEN contact_or_anonymous_id IN ('c-1','x'' OR ''1''=''1') THEN 'false'", + ); + }); + + it('bypasses the cache for a short window after the signal (ClickHouse ingest is async)', async () => { + const fetches: Set[] = [ + new Set(['stale']), + new Set(['stale', 'fresh']), + ]; + const clickhouse = { query: jest.fn() } as any; + const cache = new DeletedContactsCacheService(clickhouse); + (cache as any).fetchDeletedContactsFromClickHouse = jest.fn(() => + Promise.resolve(fetches.shift() ?? new Set()), + ); + + expect(await cache.getDeletedContacts()).toEqual(new Set(['stale'])); + expect(await cache.getDeletedContacts()).toEqual(new Set(['stale'])); // cache hit + + cache.onContactDeletedIngested(); + + expect(await cache.getDeletedContacts()).toEqual( + new Set(['stale', 'fresh']), + ); // re-queried + expect( + (cache as any).fetchDeletedContactsFromClickHouse, + ).toHaveBeenCalledTimes(2); + expect((cache as any).expiresAt).toBeLessThanOrEqual( + (cache as any).bypassCacheUntil, + ); + }); + + it('drops the cached snapshot when a deleted-contact event is ingested', () => { + const cache = new DeletedContactsCacheService({} as any); + (cache as any).cached = new Set(['c-1']); + (cache as any).expiresAt = Number.MAX_SAFE_INTEGER; + + cache.onContactDeletedIngested(); + + expect((cache as any).cached).toBeNull(); + }); +}); diff --git a/src/modules/segments/services/segment-query-execution.service.ts b/src/modules/segments/services/segment-query-execution.service.ts index 780bb21..8970c69 100644 --- a/src/modules/segments/services/segment-query-execution.service.ts +++ b/src/modules/segments/services/segment-query-execution.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { ClickHouseService } from '../../processing/clickhouse/clickhouse.service'; import { Segment } from '../entities/segment.entity'; import { DeletedContactsCacheService } from './deleted-contacts-cache.service'; -import { DELETED_CONTACTS_CASE_BRANCH_REGEX } from '../queries/contact-event-names'; +import { applyDeletedContactsOptimization } from '../queries/contact-event-names'; import { SegmentMetricsService } from '../metrics/segment-metrics.service'; import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query-builder.service'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; @@ -174,7 +174,8 @@ export class SegmentQueryExecutionService { operation: 'deleted_contacts_cache_lookup', }); - const deletedContacts = await this.deletedContactsCache.getDeletedContacts(); + const deletedContacts = + await this.deletedContactsCache.getDeletedContacts(); if (deletedContacts.size > 0) { this.metrics.recordCacheHit(); @@ -182,23 +183,14 @@ export class SegmentQueryExecutionService { this.metrics.recordCacheMiss(); } + const optimizedQuery = applyDeletedContactsOptimization( + query, + deletedContacts, + ); if (deletedContacts.size === 0) { - return query.replace( - DELETED_CONTACTS_CASE_BRANCH_REGEX, - `WHEN 1=0 THEN 'false'`, - ); + return optimizedQuery; } - const deletedContactsArray = Array.from(deletedContacts).map( - (id) => `'${id}'`, - ); - const deletedContactsList = deletedContactsArray.join(','); - - const optimizedQuery = query.replace( - DELETED_CONTACTS_CASE_BRANCH_REGEX, - `WHEN contact_or_anonymous_id IN (${deletedContactsList}) THEN 'false'`, - ); - this.logger.debug( `Optimized query: replaced nested subqueries with ${deletedContacts.size} cached deleted contacts`, ); From a2e2028343062f78e33e2fa66c9326cf703ec0c4 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sat, 22 Aug 2026 11:33:42 -0300 Subject: [PATCH 14/29] fix(segments): a otimizacao de deletados preserva o sentinela do ramo CASE (CRM-215) Achado de review na propria CRM-215. applyDeletedContactsOptimization casava `THEN ''` mas reescrevia sempre `THEN 'false'`. A maioria dos nos (LastPerformed, Performed sem contagem, WhatsApp/Web/SMS, UserProperty nao-argMax, e qualquer operador Exists) marca o contato deletado com o sentinela VAZIO, e a associacao e `argMaxMerge(last_value) != ''` -- reescrever para um literal nao-vazio devolvia o contato deletado ao segmento, justamente o AC que o card veio fechar. Regressao nova: o regex da develop (`[^)]*`) nao atravessava o `)` de `argMax(occurred_at, occurred_at)` e nunca casava, entao a reescrita era codigo morto. A validacao manual do card so exercitou nos Label, a unica familia com `validationInfo: Equals 'true'` e ramo ja `'false'`, portanto imune. - captura o literal do ramo e o reemite; replacer em funcao para que `$` dentro de um id nao seja lido como referencia de grupo - routing-config: adiciona `contact.deleted` canonico, que caia no fallback SYSTEM/low em vez de LIFECYCLE (mesmo defeito ja corrigido para o dotted de custom attribute) - remove o ramo morto de size === 0 no optimizeQueryWithDeletedContactsCache - testes: sentinela vazio preservado nos tres tipos de no afetados, `$` no id, e classificacao das duas grafias do evento de exclusao --- .../events/constants/routing-config.spec.ts | 13 ++++++ .../events/constants/routing-config.ts | 8 ++-- .../segments/queries/contact-event-names.ts | 15 +++++-- .../segment-canonical-event-names.spec.ts | 43 +++++++++++++++++++ .../segment-query-execution.service.ts | 12 +++--- 5 files changed, 79 insertions(+), 12 deletions(-) diff --git a/src/modules/events/constants/routing-config.spec.ts b/src/modules/events/constants/routing-config.spec.ts index 2e9348a..d4ead98 100644 --- a/src/modules/events/constants/routing-config.spec.ts +++ b/src/modules/events/constants/routing-config.spec.ts @@ -18,3 +18,16 @@ describe('EventRoutingConfigHelper.getEventClassification — custom attribute ( ).toBe(EventClassification.LIFECYCLE); }); }); + +// CRM-215: same defect for the deletion event — the CRM emits `contact.deleted`, +// which fell through to the SYSTEM fallback and scored low priority. +describe('EventRoutingConfigHelper.getEventClassification — contact deleted (CRM-215)', () => { + it.each(['contact.deleted', 'contact_deleted'])( + 'classifies %s as LIFECYCLE', + (eventName) => { + expect(EventRoutingConfigHelper.getEventClassification(eventName)).toBe( + EventClassification.LIFECYCLE, + ); + }, + ); +}); diff --git a/src/modules/events/constants/routing-config.ts b/src/modules/events/constants/routing-config.ts index 8d9cb89..7a93d8b 100644 --- a/src/modules/events/constants/routing-config.ts +++ b/src/modules/events/constants/routing-config.ts @@ -21,10 +21,12 @@ export const LIFECYCLE_EVENTS = { SEGMENT_ENTERED: 'segment_entered', SEGMENT_EXITED: 'segment_exited', CUSTOM_ATTRIBUTE_CHANGED: 'custom_attribute_changed', - // Canonical dotted name actually emitted by the CRM (EVO-1839). Required because - // getEventClassification matches by substring, so the dotted form is not covered - // by the legacy underscore entry above. + // Canonical dotted names actually emitted by the CRM (EVO-1839, CRM-215). Required + // because getEventClassification matches by substring, so the dotted forms are not + // covered by the legacy underscore entries above — without them the event falls back + // to SYSTEM/low priority instead of LIFECYCLE. CUSTOM_ATTRIBUTE_CHANGED_DOTTED: 'contact.custom_attribute.changed', + CONTACT_DELETED_DOTTED: 'contact.deleted', } as const; // System events diff --git a/src/modules/segments/queries/contact-event-names.ts b/src/modules/segments/queries/contact-event-names.ts index 67de2ae..acacd32 100644 --- a/src/modules/segments/queries/contact-event-names.ts +++ b/src/modules/segments/queries/contact-event-names.ts @@ -35,9 +35,13 @@ function escapeRegExp(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } -/** Matches the deleted-contacts CASE branch the builder emits, whitespace-tolerant. */ +/** + * Matches the deleted-contacts CASE branch the builder emits, whitespace-tolerant. + * Group 1 is the branch's result literal, which differs per node type and must be + * preserved by any rewrite (see `applyDeletedContactsOptimization`). + */ export const DELETED_CONTACTS_CASE_BRANCH_REGEX = new RegExp( - `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '[^']*'`, + `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '([^']*)'`, 'g', ); @@ -63,8 +67,13 @@ export function applyDeletedContactsOptimization( const list = Array.from(deletedContacts) .map((id) => `'${id.replace(/'/g, "''")}'`) .join(','); + // Keep the branch's own result literal: most node types mark a deleted contact with the + // empty sentinel, and membership is `argMaxMerge(last_value) != ''` — rewriting it to a + // non-empty literal puts the contact back in the segment. A replacer function is used so + // `$` inside an id is not read as a capture reference. return query.replace( DELETED_CONTACTS_CASE_BRANCH_REGEX, - `WHEN contact_or_anonymous_id IN (${list}) THEN 'false'`, + (_match, sentinel: string) => + `WHEN contact_or_anonymous_id IN (${list}) THEN '${sentinel}'`, ); } diff --git a/src/modules/segments/services/segment-canonical-event-names.spec.ts b/src/modules/segments/services/segment-canonical-event-names.spec.ts index abac9fd..2409174 100644 --- a/src/modules/segments/services/segment-canonical-event-names.spec.ts +++ b/src/modules/segments/services/segment-canonical-event-names.spec.ts @@ -151,6 +151,49 @@ describe('CRM-215 deleted-contacts cache never trades correctness for speed', () ); }); + // Most node types mark a deleted contact with the EMPTY sentinel and membership is + // `argMaxMerge(last_value) != ''`: rewriting it to 'false' put the contact back in. + it.each([ + [SegmentNodeType.LastPerformed, { event: 'order_placed' }], + [SegmentNodeType.Performed, { event: 'order_placed' }], + [SegmentNodeType.WhatsApp, { event: 'MessageSent' }], + ])( + '%s: keeps the empty sentinel of the deleted-contacts branch', + (type, extra) => { + const emptySentinelNode = { id: 'n-empty', type, ...extra } as any; + const [subQuery] = builder.segmentNodeToStateSubQuery( + segment, + emptySentinelNode, + ); + const lastValueSql = builder.generateArgMaxValidation(subQuery); + expect(lastValueSql).toContain("THEN ''"); + + const out = applyDeletedContactsOptimization( + lastValueSql, + new Set(['deleted-1']), + ); + + expect(out).toContain( + "WHEN contact_or_anonymous_id IN ('deleted-1') THEN ''", + ); + expect(out).not.toContain(DELETED_CONTACTS_SUBQUERY); + }, + ); + + it('does not read `$` inside a contact id as a capture reference', () => { + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + const sql = String(subQuery.argMaxValue); + + const out = applyDeletedContactsOptimization( + sql, + new Set(['a$&b', 'c$1d']), + ); + + expect(out).toContain( + "WHEN contact_or_anonymous_id IN ('a$&b','c$1d') THEN 'false'", + ); + }); + it('bypasses the cache for a short window after the signal (ClickHouse ingest is async)', async () => { const fetches: Set[] = [ new Set(['stale']), diff --git a/src/modules/segments/services/segment-query-execution.service.ts b/src/modules/segments/services/segment-query-execution.service.ts index 8970c69..c572d8d 100644 --- a/src/modules/segments/services/segment-query-execution.service.ts +++ b/src/modules/segments/services/segment-query-execution.service.ts @@ -183,17 +183,17 @@ export class SegmentQueryExecutionService { this.metrics.recordCacheMiss(); } + // An empty set leaves the query untouched: the real subselect stays in place. const optimizedQuery = applyDeletedContactsOptimization( query, deletedContacts, ); - if (deletedContacts.size === 0) { - return optimizedQuery; - } - this.logger.debug( - `Optimized query: replaced nested subqueries with ${deletedContacts.size} cached deleted contacts`, - ); + if (deletedContacts.size > 0) { + this.logger.debug( + `Optimized query: replaced nested subqueries with ${deletedContacts.size} cached deleted contacts`, + ); + } return optimizedQuery; } From d342213a6751cbb14c67fde4cbd5a088990ab7d9 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sat, 22 Aug 2026 11:45:40 -0300 Subject: [PATCH 15/29] fix(segments): condicao de Etiqueta casa por labelId OU labelName (CRM-215) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Achado 5 do review. A migracao titulo→id do PR #318 so roda quando alguem abre e salva o segmento no editor, e nao ha backfill: toda definicao gravada pelo editor antigo guarda o TITULO da etiqueta, entao o AC1 valia so para segmento novo ou reeditado. O evento ja carrega os dois lados -- o handle_label_change do CRM monta `traits = { labelName:, labelId:, source: }` -- entao aceitar as duas grafias na camada de query faz a definicao antiga voltar a casar sozinha, sem migracao e sem depender de reabrir o segmento. Mesma postura de "aceitar ambos" ja adotada para os nomes de evento. Backfill de verdade sairia caro e frágil: as definicoes moram no Postgres do evo-flow, mas o mapa titulo→id mora no CRM -- a migration teria que chamar a API do CRM. --- .../segment-canonical-event-names.spec.ts | 21 +++++++++++++++++++ ...egment-clickhouse-query-builder.service.ts | 10 +++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/modules/segments/services/segment-canonical-event-names.spec.ts b/src/modules/segments/services/segment-canonical-event-names.spec.ts index 2409174..971a778 100644 --- a/src/modules/segments/services/segment-canonical-event-names.spec.ts +++ b/src/modules/segments/services/segment-canonical-event-names.spec.ts @@ -62,6 +62,27 @@ describe('CRM-215 segment SQL matches the canonical contact event names', () => ); }); + // Definitions saved by the old editor hold the title; the event carries both traits, + // so both spellings must match and no backfill is needed. + it.each(['has', 'not_has'])( + 'Label %s: matches the stored value against labelId OR labelName', + (condition) => { + const node = { + id: 'n1', + type: SegmentNodeType.Label, + labelId: 'VIP', + condition, + } as any; + + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); + const sql = `${subQuery.condition} ${subQuery.argMaxValue}`; + + expect(sql).toContain( + "(JSONExtractString(traits, 'labelId') = 'VIP' OR JSONExtractString(traits, 'labelName') = 'VIP')", + ); + }, + ); + it('every deleted-contacts guard matches contact.deleted (and the legacy spelling)', () => { const nodes = [ { diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index 416ac5f..eccbbcc 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -645,6 +645,12 @@ export class SegmentClickHouseQueryBuilderService { const labelId = this.escapeSql(labelNode.labelId); const LABEL_ADDED_IN = sqlStringList(LABEL_ADDED_EVENT_NAMES); const LABEL_EVENTS_IN = sqlStringList([...LABEL_ADDED_EVENT_NAMES, ...LABEL_REMOVED_EVENT_NAMES]); + // Definitions saved by the old editor hold the label TITLE instead of its id, and + // there is no backfill. Every contact.label.* event carries both in traits, so + // match either — a stored title keeps working without reopening the segment (CRM-215). + const LABEL_MATCH = + `(JSONExtractString(traits, 'labelId') = '${labelId}'` + + ` OR JSONExtractString(traits, 'labelName') = '${labelId}')`; switch (labelNode.condition) { case 'has': @@ -652,7 +658,7 @@ export class SegmentClickHouseQueryBuilderService { return [ { stateId, - condition: `event_name IN (${LABEL_EVENTS_IN}) AND JSONExtractString(traits, 'labelId') = '${labelId}'`, + condition: `event_name IN (${LABEL_EVENTS_IN}) AND ${LABEL_MATCH}`, argMaxValue: ` CASE WHEN contact_or_anonymous_id IN ( @@ -692,7 +698,7 @@ export class SegmentClickHouseQueryBuilderService { SELECT DISTINCT contact_or_anonymous_id FROM contact_events WHERE event_name IN (${LABEL_EVENTS_IN}) - AND JSONExtractString(traits, 'labelId') = '${labelId}' + AND ${LABEL_MATCH} GROUP BY contact_or_anonymous_id HAVING argMax(if(event_name IN (${LABEL_ADDED_IN}), 'true', 'false'), occurred_at) = 'true' ) THEN 'false' From cd0d45c16ba79616a440e5bcc0de365d0fc63601 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sat, 22 Aug 2026 12:14:09 -0300 Subject: [PATCH 16/29] refactor(segments): escapa a lista de nomes e nao compartilha o regex /g (CRM-215) Achados 9 e 10 do review, ambos no arquivo novo da propria PR. - sqlStringList dobrava aspas de ninguem: hoje so recebe constantes, mas e exportado e uma string com aspas quebraria o literal. - o regex do ramo CASE era uma constante exportada com flag /g, e .test() nela carrega lastIndex entre chamadas. Virou funcao que devolve instancia nova. --- .../segments/queries/contact-event-names.ts | 15 +++++++++------ .../segment-canonical-event-names.spec.ts | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/modules/segments/queries/contact-event-names.ts b/src/modules/segments/queries/contact-event-names.ts index acacd32..98cbc93 100644 --- a/src/modules/segments/queries/contact-event-names.ts +++ b/src/modules/segments/queries/contact-event-names.ts @@ -18,7 +18,7 @@ export const LABEL_REMOVED_EVENT_NAMES = [ ] as const; export function sqlStringList(names: readonly string[]): string { - return names.map((n) => `'${n}'`).join(', '); + return names.map((n) => `'${n.replace(/'/g, "''")}'`).join(', '); } /** @@ -39,11 +39,14 @@ function escapeRegExp(s: string): string { * Matches the deleted-contacts CASE branch the builder emits, whitespace-tolerant. * Group 1 is the branch's result literal, which differs per node type and must be * preserved by any rewrite (see `applyDeletedContactsOptimization`). + * A fresh instance per call: a shared /g regex carries `lastIndex` between callers. */ -export const DELETED_CONTACTS_CASE_BRANCH_REGEX = new RegExp( - `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '([^']*)'`, - 'g', -); +export function deletedContactsCaseBranchRegex(): RegExp { + return new RegExp( + `WHEN contact_or_anonymous_id IN \\(\\s*${escapeRegExp(DELETED_CONTACTS_SUBQUERY)}\\s*\\) THEN '([^']*)'`, + 'g', + ); +} /** * In-process signal emitted by the events API when a deleted-contact event is ingested, @@ -72,7 +75,7 @@ export function applyDeletedContactsOptimization( // non-empty literal puts the contact back in the segment. A replacer function is used so // `$` inside an id is not read as a capture reference. return query.replace( - DELETED_CONTACTS_CASE_BRANCH_REGEX, + deletedContactsCaseBranchRegex(), (_match, sentinel: string) => `WHEN contact_or_anonymous_id IN (${list}) THEN '${sentinel}'`, ); diff --git a/src/modules/segments/services/segment-canonical-event-names.spec.ts b/src/modules/segments/services/segment-canonical-event-names.spec.ts index 971a778..886471e 100644 --- a/src/modules/segments/services/segment-canonical-event-names.spec.ts +++ b/src/modules/segments/services/segment-canonical-event-names.spec.ts @@ -2,7 +2,7 @@ import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query import { SegmentNodeType } from '../entities/segment.entity'; import { applyDeletedContactsOptimization, - DELETED_CONTACTS_CASE_BRANCH_REGEX, + deletedContactsCaseBranchRegex, DELETED_CONTACTS_SUBQUERY, } from '../queries/contact-event-names'; import { DeletedContactsCacheService } from './deleted-contacts-cache.service'; @@ -127,7 +127,7 @@ describe('CRM-215 segment SQL matches the canonical contact event names', () => const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node); const rewritten = String(subQuery.argMaxValue).replace( - DELETED_CONTACTS_CASE_BRANCH_REGEX, + deletedContactsCaseBranchRegex(), `WHEN 1=0 THEN 'false'`, ); From 554ff57ab853f0b3e5693766622af6d5ec98dd57 Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Sun, 23 Aug 2026 14:20:44 -0300 Subject: [PATCH 17/29] fix(journeys): gatilho de webhook casa o evento exato, nao o prefixo (CRM-256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebhookTrigger casava qualquer eventName comecando por `webhook.`. O pipeline de entregabilidade de e-mail grava cada callback de provedor em contact_events como `webhook.` e a materialized view events_to_journey_triggers_mv publica tudo no mesmo barramento, entao abertura/clique/bounce iniciava toda jornada com gatilho Webhook — com contact_id vazio, porque a linha de analytics nao resolve contato. - Match exato em `webhook.journey_trigger`, o nome que POST /api/v1/journeys/trigger/:journeyId emite; `eventName` na config do trigger sobrepoe o padrao. - Quando o evento traz `journeyId` nas properties, ele precisa ser o desta jornada — senao o webhook de uma jornada dispararia todas as outras. A MV segue sem WHERE de proposito: os outros sete tipos de gatilho leem do mesmo topico. --- .../services/triggers/webhook.trigger.spec.ts | 92 +++++++++++++++++++ .../services/triggers/webhook.trigger.ts | 71 ++++++++++---- 2 files changed, 145 insertions(+), 18 deletions(-) create mode 100644 src/modules/journeys/services/triggers/webhook.trigger.spec.ts diff --git a/src/modules/journeys/services/triggers/webhook.trigger.spec.ts b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts new file mode 100644 index 0000000..a5cbcc0 --- /dev/null +++ b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts @@ -0,0 +1,92 @@ +import { WebhookTrigger } from './webhook.trigger'; +import { JourneyTriggerEvent } from '../journey-trigger-processor.service'; + +describe('WebhookTrigger', () => { + let trigger: WebhookTrigger; + + const journey = { id: 'journey-1' }; + + const event = ( + eventName: string, + properties: Record = {}, + ): JourneyTriggerEvent => ({ + messageId: 'm1', + contactId: 'c1', + eventName, + eventType: 'track', + properties: JSON.stringify(properties), + timestamp: '2026-08-23T00:00:00.000Z', + }); + + const webhookTrigger = (metadata: Record = {}) => ({ + type: 'Webhook', + metadata, + }); + + beforeEach(() => { + trigger = new WebhookTrigger(); + jest + .spyOn((trigger as any).logger, 'debug') + .mockImplementation(() => undefined); + }); + + it('matches the event emitted by the journey trigger endpoint', () => { + const result = trigger.matches( + event('webhook.journey_trigger', { journeyId: journey.id }), + webhookTrigger(), + journey, + ); + expect(result.matches).toBe(true); + }); + + it.each([ + 'webhook.sendgrid', + 'webhook.resend', + 'webhook.ses', + 'webhook.unknown', + ])('does NOT match the e-mail deliverability event %s', (eventName) => { + const result = trigger.matches(event(eventName), webhookTrigger(), journey); + expect(result.matches).toBe(false); + }); + + it('does NOT match a webhook addressed to another journey', () => { + const result = trigger.matches( + event('webhook.journey_trigger', { journeyId: 'journey-2' }), + webhookTrigger(), + journey, + ); + expect(result.matches).toBe(false); + }); + + it('matches when the event carries no journeyId', () => { + const result = trigger.matches( + event('webhook.journey_trigger'), + webhookTrigger(), + journey, + ); + expect(result.matches).toBe(true); + }); + + it('honours an eventName configured on the trigger', () => { + const configured = webhookTrigger({ eventName: 'webhook.sendgrid' }); + + expect( + trigger.matches(event('webhook.sendgrid'), configured, journey).matches, + ).toBe(true); + expect( + trigger.matches(event('webhook.journey_trigger'), configured, journey) + .matches, + ).toBe(false); + }); + + it('does not throw on unparseable event properties', () => { + const broken: JourneyTriggerEvent = { + ...event('webhook.journey_trigger'), + properties: '{not json', + }; + + expect(trigger.matches(broken, webhookTrigger(), journey).matches).toBe( + true, + ); + }); +}); diff --git a/src/modules/journeys/services/triggers/webhook.trigger.ts b/src/modules/journeys/services/triggers/webhook.trigger.ts index a288603..0b12cd5 100644 --- a/src/modules/journeys/services/triggers/webhook.trigger.ts +++ b/src/modules/journeys/services/triggers/webhook.trigger.ts @@ -2,6 +2,12 @@ import { Injectable } from '@nestjs/common'; import { BaseTrigger, TriggerMatchResult } from './base.trigger'; import { JourneyTriggerEvent } from '../journey-trigger-processor.service'; +// Name emitted by POST /api/v1/journeys/trigger/:journeyId. Matching the whole +// `webhook.` prefix is not an option: the e-mail deliverability pipeline writes +// every provider callback to `contact_events` as `webhook.` +// (sendgrid, resend, ses, ...), and those share the journey-trigger bus. +const JOURNEY_WEBHOOK_EVENT_NAME = 'webhook.journey_trigger'; + @Injectable() export class WebhookTrigger extends BaseTrigger { constructor() { @@ -10,30 +16,59 @@ export class WebhookTrigger extends BaseTrigger { matches( event: JourneyTriggerEvent, - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars - trigger: any, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - journey: any, + trigger: unknown, + journey: unknown, ): TriggerMatchResult { - // For webhook triggers, accept events that start with 'webhook.' - const isWebhookEvent = event.eventName.startsWith('webhook.'); + const config = this.getTriggerConfig(trigger) as { eventName?: string }; + const node = trigger as { eventName?: string }; + const targetEventName = + config.eventName || node.eventName || JOURNEY_WEBHOOK_EVENT_NAME; + + if (event.eventName !== targetEventName) { + const result: TriggerMatchResult = { + matches: false, + reason: `Event name mismatch: ${event.eventName} !== ${targetEventName}`, + metadata: { eventName: event.eventName, targetEventName }, + }; + this.logMatch(event, journey, result); + return result; + } + + const journeyId = (journey as { id: string }).id; + const addressedJourneyId = this.getAddressedJourneyId(event); - this.logger.debug( - `🔍 Webhook trigger check: ${event.eventName} starts with 'webhook.' = ${isWebhookEvent}`, - ); + if (addressedJourneyId && addressedJourneyId !== journeyId) { + const result: TriggerMatchResult = { + matches: false, + reason: `Webhook is addressed to journey ${addressedJourneyId}, not ${journeyId}`, + metadata: { eventName: event.eventName, addressedJourneyId }, + }; + this.logMatch(event, journey, result); + return result; + } const result: TriggerMatchResult = { - matches: isWebhookEvent, - reason: isWebhookEvent - ? `Event name starts with 'webhook.': ${event.eventName}` - : `Event name does not start with 'webhook.': ${event.eventName}`, - metadata: { - eventName: event.eventName, - isWebhookEvent, - }, + matches: true, + reason: `Event name matches: ${targetEventName}`, + metadata: { eventName: event.eventName, targetEventName }, }; - this.logMatch(event, journey, result); return result; } + + private getAddressedJourneyId(event: JourneyTriggerEvent): string | null { + try { + const properties = JSON.parse(event.properties || '{}') as { + journeyId?: string; + }; + return properties.journeyId || null; + } catch (error) { + this.logger.debug( + `Could not read journeyId from event properties: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return null; + } + } } From 12aa413ef2fc487e906e3f53814e27b39026e157 Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Sun, 23 Aug 2026 14:30:27 -0300 Subject: [PATCH 18/29] fix(journeys): evento sem contato nao inicia execucao de jornada (CRM-271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit analyzeEventForJourneyTriggers seguia adiante com contactId vazio: consultava sessoes em espera pela chave '', casava gatilhos e chamava triggerJourneyExecution, que iniciava workflow Temporal com contactId '' e workflowId degenerado (journey--contact--). A guarda de reentrada e o claim de idempotencia tambem passavam a ser chaveados por contato vazio, entao todos os eventos sem contato compartilhavam a mesma chave. O barramento journey-triggers recebe toda linha de contact_events, e nem todo produtor resolve contato — as linhas de entregabilidade de e-mail gravam contact_id vazio. Descarte agora acontece antes de qualquer trabalho contact-scoped, com WARN nomeando evento e messageId em vez de sumir em silencio. --- .../journey-trigger-processor.service.spec.ts | 74 +++++++++++++++++++ .../journey-trigger-processor.service.ts | 16 ++++ 2 files changed, 90 insertions(+) diff --git a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts index fad25e0..7276d90 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts @@ -370,3 +370,77 @@ describe('JourneyTriggerProcessor consumer gating (EVO-1764 A1)', () => { expect(warmActiveJourneysCache).not.toHaveBeenCalled(); }); }); + +describe('JourneyTriggerProcessor contact-less events', () => { + let processor: JourneyTriggerProcessor; + let findActive: jest.Mock; + let checkWaitingSessions: jest.Mock; + let triggerJourneyExecution: jest.Mock; + + const event = (contactId?: string) => + ({ + messageId: 'm-1', + contactId, + eventName: 'webhook.sendgrid', + eventType: 'track', + properties: '{}', + traits: '{}', + timestamp: '2026-08-23T00:00:00.000Z', + }) as any; + + const analyze = (contactId?: string) => + (processor as any).analyzeEventForJourneyTriggers(event(contactId)); + + beforeEach(async () => { + findActive = jest.fn().mockResolvedValue([{ id: 'journey-1', name: 'J1' }]); + processor = new JourneyTriggerProcessor( + { findActive } as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + ['log', 'warn', 'error'].forEach((m) => + jest + .spyOn((processor as any).logger, m) + .mockImplementation(() => undefined), + ); + await new Promise((resolve) => setImmediate(resolve)); + + checkWaitingSessions = jest.fn().mockResolvedValue(undefined); + triggerJourneyExecution = jest.fn().mockResolvedValue(undefined); + (processor as any).checkWaitingSessions = checkWaitingSessions; + (processor as any).triggerJourneyExecution = triggerJourneyExecution; + (processor as any).matchesJourneyTrigger = jest + .fn() + .mockResolvedValue(true); + }); + + it.each([ + ['empty', ''], + ['whitespace only', ' '], + ['absent', undefined], + ])('does not dispatch a workflow when contactId is %s', async (_, id) => { + await analyze(id); + + expect(triggerJourneyExecution).not.toHaveBeenCalled(); + expect(checkWaitingSessions).not.toHaveBeenCalled(); + expect(findActive).not.toHaveBeenCalled(); + }); + + it('logs the skip instead of dropping the event silently', async () => { + await analyze(''); + + expect((processor as any).logger.warn).toHaveBeenCalledWith( + expect.stringContaining('no contactId'), + expect.objectContaining({ eventName: 'webhook.sendgrid' }), + ); + }); + + it('still dispatches for an event that carries a contact', async () => { + await analyze('contact-1'); + + expect(checkWaitingSessions).toHaveBeenCalledTimes(1); + expect(triggerJourneyExecution).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/modules/journeys/services/journey-trigger-processor.service.ts b/src/modules/journeys/services/journey-trigger-processor.service.ts index 4a056c4..651b40e 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.ts @@ -281,6 +281,22 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { `🔍 Analyzing event for journey triggers: ${event.eventName}`, ); + // Sessions, the re-entry guard and the dedup claim are all keyed by + // contact: a contact-less event can only dispatch a workflow nobody can + // act on, and collapses every such event onto the same cache key. The + // e-mail deliverability rows on this bus carry an empty contact_id. + if (!event.contactId || event.contactId.trim() === '') { + this.logger.warn( + `⏭️ Skipping event ${event.eventName} — no contactId, nothing contact-scoped can run`, + { + messageId: event.messageId, + eventName: event.eventName, + anonymousId: event.anonymousId, + }, + ); + return; + } + // 1. First, check if event satisfies any waiting sessions await this.checkWaitingSessions(event); From 791ad6c0e267b089e21b03d96a6797039bb65acf Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Sun, 23 Aug 2026 18:39:34 -0300 Subject: [PATCH 19/29] refactor(journeys): simplifica o gatilho de webhook e alinha a leitura de config (CRM-256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retorno de code review sobre o commit anterior. O ramo que comparava properties.journeyId com journey.id saiu. Ele nunca executava — nenhum produtor publica webhook.journey_trigger no barramento, ja que o endpoint manual chama startJourney direto — e quebrava um contrato que os outros sete handlers respeitam: ao avaliar condicoes de espera o processador chama matches() com um journey vazio, entao qualquer decisao baseada em journey.id tornava a espera insatisfazivel. Com isso somem tambem o JSON.parse que falhava aberto (payload malformado passava a casar toda jornada) e o log de erro em nivel debug. A resolucao do nome do evento passa a seguir a mesma ordem do EventTrigger (metadata, no, conditions), trata nome em branco como nao configurado e nao estoura com trigger nulo. Antes, um no configurado via conditions.eventName era ignorado em silencio — a mesma classe de bug, invertida. Os testes cobrem as duas fontes novas de config, o call site de condicao de espera e as assercoes de reason/metadata, que antes nao eram verificadas. --- docs/journey-manual-trigger.md | 8 + .../services/triggers/webhook.trigger.spec.ts | 142 +++++++++++++----- .../services/triggers/webhook.trigger.ts | 74 +++++---- 3 files changed, 148 insertions(+), 76 deletions(-) diff --git a/docs/journey-manual-trigger.md b/docs/journey-manual-trigger.md index c804612..1c1d345 100644 --- a/docs/journey-manual-trigger.md +++ b/docs/journey-manual-trigger.md @@ -64,3 +64,11 @@ The emitted event uses `eventName: "webhook.journey_trigger"`, "processedAt": "2026-06-05T20:36:42.898Z" } ``` + +## Why the Webhook trigger node never matches on the event bus + +`WebhookTrigger` (the handler behind the Webhook trigger node) matches the exact event name `webhook.journey_trigger`, not the `webhook.` prefix — the e-mail deliverability pipeline writes every provider callback to `contact_events` as `webhook.` (`sendgrid`, `resend`, `ses`, ...) and those share the `journey-triggers` bus, so a prefix match started journeys off unrelated traffic. + +In practice the handler matches nothing: this endpoint bypasses trigger matching (see above), so no producer publishes `webhook.journey_trigger` onto the bus. That is expected — the node still works, because the endpoint starts the named journey directly. + +Anything that starts publishing `webhook.journey_trigger` onto the bus must address a single journey itself. The matcher is per-journey and has no journey context to compare against (wait conditions are evaluated with an empty journey), so an unaddressed event would start **every** journey holding a Webhook trigger. diff --git a/src/modules/journeys/services/triggers/webhook.trigger.spec.ts b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts index a5cbcc0..60f84d8 100644 --- a/src/modules/journeys/services/triggers/webhook.trigger.spec.ts +++ b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts @@ -8,7 +8,7 @@ describe('WebhookTrigger', () => { const event = ( eventName: string, - properties: Record = {}, + properties: Record = {}, ): JourneyTriggerEvent => ({ messageId: 'm1', contactId: 'c1', @@ -18,7 +18,7 @@ describe('WebhookTrigger', () => { timestamp: '2026-08-23T00:00:00.000Z', }); - const webhookTrigger = (metadata: Record = {}) => ({ + const webhookTrigger = (metadata: Record = {}) => ({ type: 'Webhook', metadata, }); @@ -26,17 +26,28 @@ describe('WebhookTrigger', () => { beforeEach(() => { trigger = new WebhookTrigger(); jest - .spyOn((trigger as any).logger, 'debug') + .spyOn( + (trigger as unknown as { logger: { debug: () => void } }).logger, + 'debug', + ) .mockImplementation(() => undefined); }); it('matches the event emitted by the journey trigger endpoint', () => { const result = trigger.matches( - event('webhook.journey_trigger', { journeyId: journey.id }), + event('webhook.journey_trigger'), webhookTrigger(), journey, ); - expect(result.matches).toBe(true); + + expect(result).toMatchObject({ + matches: true, + reason: 'Event name matches: webhook.journey_trigger', + metadata: { + eventName: 'webhook.journey_trigger', + targetEventName: 'webhook.journey_trigger', + }, + }); }); it.each([ @@ -46,47 +57,102 @@ describe('WebhookTrigger', () => { 'webhook.unknown', ])('does NOT match the e-mail deliverability event %s', (eventName) => { const result = trigger.matches(event(eventName), webhookTrigger(), journey); - expect(result.matches).toBe(false); - }); - it('does NOT match a webhook addressed to another journey', () => { - const result = trigger.matches( - event('webhook.journey_trigger', { journeyId: 'journey-2' }), - webhookTrigger(), - journey, - ); - expect(result.matches).toBe(false); + expect(result).toMatchObject({ + matches: false, + reason: `Event name mismatch: ${eventName} !== webhook.journey_trigger`, + metadata: { eventName, targetEventName: 'webhook.journey_trigger' }, + }); }); - it('matches when the event carries no journeyId', () => { - const result = trigger.matches( - event('webhook.journey_trigger'), - webhookTrigger(), - journey, - ); - expect(result.matches).toBe(true); - }); + describe('target event name resolution', () => { + it('honours an eventName configured in metadata', () => { + const configured = webhookTrigger({ eventName: 'webhook.sendgrid' }); + + expect( + trigger.matches(event('webhook.sendgrid'), configured, journey).matches, + ).toBe(true); + expect( + trigger.matches(event('webhook.journey_trigger'), configured, journey) + .matches, + ).toBe(false); + }); + + it('honours an eventName set directly on the node', () => { + const node = { type: 'Webhook', eventName: 'webhook.custom' }; - it('honours an eventName configured on the trigger', () => { - const configured = webhookTrigger({ eventName: 'webhook.sendgrid' }); + expect( + trigger.matches(event('webhook.custom'), node, journey).matches, + ).toBe(true); + }); - expect( - trigger.matches(event('webhook.sendgrid'), configured, journey).matches, - ).toBe(true); - expect( - trigger.matches(event('webhook.journey_trigger'), configured, journey) - .matches, - ).toBe(false); + // EventTrigger reads this path too; a handler that ignored it would leave the + // node silently never firing. + it('honours an eventName under conditions, like EventTrigger does', () => { + const node = { + type: 'Webhook', + conditions: { eventName: 'webhook.custom' }, + }; + + expect( + trigger.matches(event('webhook.custom'), node, journey).matches, + ).toBe(true); + }); + + it.each([ + ['blank', ' '], + ['empty', ''], + ])('treats a %s configured eventName as unset', (_label, eventName) => { + const result = trigger.matches( + event('webhook.journey_trigger'), + webhookTrigger({ eventName }), + journey, + ); + + expect(result.matches).toBe(true); + }); + + it('trims a configured eventName', () => { + expect( + trigger.matches( + event('webhook.custom'), + webhookTrigger({ eventName: ' webhook.custom ' }), + journey, + ).matches, + ).toBe(true); + }); }); - it('does not throw on unparseable event properties', () => { - const broken: JourneyTriggerEvent = { - ...event('webhook.journey_trigger'), - properties: '{not json', - }; + describe('call-site robustness', () => { + // journey-trigger-processor.service.ts calls handlers with `{}` as the journey + // when it evaluates wait conditions; matching must not depend on journey.id. + it('matches with the empty journey the wait-condition call site passes', () => { + const waitConditions = { + eventType: 'webhook', + eventName: 'webhook.journey_trigger', + }; + + expect( + trigger.matches( + // The manual-trigger emitter always stamps journeyId into properties. + event('webhook.journey_trigger', { journeyId: 'journey-9' }), + waitConditions, + {}, + ).matches, + ).toBe(true); + }); - expect(trigger.matches(broken, webhookTrigger(), journey).matches).toBe( - true, + it.each([ + ['null', null], + ['undefined', undefined], + ])( + 'falls back to the default event name for a %s trigger', + (_label, node) => { + expect( + trigger.matches(event('webhook.journey_trigger'), node, journey) + .matches, + ).toBe(true); + }, ); }); }); diff --git a/src/modules/journeys/services/triggers/webhook.trigger.ts b/src/modules/journeys/services/triggers/webhook.trigger.ts index 0b12cd5..f46fb5d 100644 --- a/src/modules/journeys/services/triggers/webhook.trigger.ts +++ b/src/modules/journeys/services/triggers/webhook.trigger.ts @@ -19,56 +19,54 @@ export class WebhookTrigger extends BaseTrigger { trigger: unknown, journey: unknown, ): TriggerMatchResult { - const config = this.getTriggerConfig(trigger) as { eventName?: string }; - const node = trigger as { eventName?: string }; - const targetEventName = - config.eventName || node.eventName || JOURNEY_WEBHOOK_EVENT_NAME; + const targetEventName = this.resolveTargetEventName(trigger); if (event.eventName !== targetEventName) { - const result: TriggerMatchResult = { + return this.decide(event, journey, { matches: false, reason: `Event name mismatch: ${event.eventName} !== ${targetEventName}`, metadata: { eventName: event.eventName, targetEventName }, - }; - this.logMatch(event, journey, result); - return result; + }); } - const journeyId = (journey as { id: string }).id; - const addressedJourneyId = this.getAddressedJourneyId(event); - - if (addressedJourneyId && addressedJourneyId !== journeyId) { - const result: TriggerMatchResult = { - matches: false, - reason: `Webhook is addressed to journey ${addressedJourneyId}, not ${journeyId}`, - metadata: { eventName: event.eventName, addressedJourneyId }, - }; - this.logMatch(event, journey, result); - return result; - } - - const result: TriggerMatchResult = { + return this.decide(event, journey, { matches: true, reason: `Event name matches: ${targetEventName}`, metadata: { eventName: event.eventName, targetEventName }, - }; - this.logMatch(event, journey, result); - return result; + }); } - private getAddressedJourneyId(event: JourneyTriggerEvent): string | null { - try { - const properties = JSON.parse(event.properties || '{}') as { - journeyId?: string; - }; - return properties.journeyId || null; - } catch (error) { - this.logger.debug( - `Could not read journeyId from event properties: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - return null; + // Same resolution order EventTrigger uses, so both handlers read an identical + // node shape identically. A blank configured name counts as unset. + private resolveTargetEventName(trigger: unknown): string { + const config = this.getTriggerConfig(trigger ?? {}) as { + eventName?: string; + }; + const node = (trigger ?? {}) as { + eventName?: string; + conditions?: { eventName?: string }; + }; + + for (const candidate of [ + config.eventName, + node.eventName, + node.conditions?.eventName, + ]) { + const name = candidate?.trim(); + if (name) { + return name; + } } + + return JOURNEY_WEBHOOK_EVENT_NAME; + } + + private decide( + event: JourneyTriggerEvent, + journey: unknown, + result: TriggerMatchResult, + ): TriggerMatchResult { + this.logMatch(event, journey, result); + return result; } } From 4e1a9a91b968d228cd92b15f1c4c5f56e633068b Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Sun, 23 Aug 2026 18:44:05 -0300 Subject: [PATCH 20/29] refactor(journeys): rebaixa o log do descarte e reforca o guard no dispatch (CRM-271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retorno de code review sobre o commit anterior. O descarte era logado em warn. Evento sem contato nao e anomalia: e a maior parte deste barramento — todo callback de entrega de e-mail e todo clique anonimo chegam assim. Em warn, o alerta de operacao dispararia continuamente. Passa a debug por evento, com o total corrido em info a cada mil descartes, para o volume continuar visivel sem virar ruido de alerta. O comentario anterior justificava o guard pelo dedup, que na verdade e chaveado por (jornada, contato, messageId) e portanto nunca colide entre eventos sem contato. O custo real e outro: getSessionsByContact carrega todas as sessoes em cache antes de filtrar, e uma sessao aberta sob contato vazio e devolvida a qualquer outro evento sem contato. O guard tambem passa a cobrir evento sem nome — nenhum dos oito handlers casa sem nome — e triggerJourneyExecution ganha a mesma checagem, em error, ja que chegar la sem contato significa que a barreira de entrada foi contornada. Os testes passam a exercitar processMessage, que e por onde o consumidor entra, e a construcao do processador virou fabrica compartilhada, sem a espera por setImmediate que cada bloco repetia. --- .../journey-trigger-processor.service.spec.ts | 232 +++++++++++------- .../journey-trigger-processor.service.ts | 70 +++++- 2 files changed, 204 insertions(+), 98 deletions(-) diff --git a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts index 7276d90..a029d06 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts @@ -13,31 +13,42 @@ jest.mock( { virtual: true }, ); +// The constructor kicks off initializeSingletonCacheService as fire-and-forget. +// Stubbing it keeps construction synchronous and I/O-free, so a test can install +// its own cache mock without first waiting a tick for the real one to land. +const createProcessor = ( + journeysService: any = {}, +): JourneyTriggerProcessor => { + jest + .spyOn( + JourneyTriggerProcessor.prototype as any, + 'initializeSingletonCacheService', + ) + .mockResolvedValue(undefined); + + const processor = new JourneyTriggerProcessor( + journeysService as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + + ['log', 'warn', 'error', 'debug'].forEach((method) => + jest + .spyOn((processor as any).logger, method) + .mockImplementation(() => undefined), + ); + + return processor; +}; + describe('JourneyTriggerProcessor.checkForActiveOrWaitingSessions (EVO-1691)', () => { let processor: JourneyTriggerProcessor; let getSessionsByContact: jest.Mock; beforeEach(async () => { - processor = new JourneyTriggerProcessor( - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - ); - jest - .spyOn((processor as any).logger, 'log') - .mockImplementation(() => undefined); - jest - .spyOn((processor as any).logger, 'warn') - .mockImplementation(() => undefined); - jest - .spyOn((processor as any).logger, 'error') - .mockImplementation(() => undefined); - - // Let the fire-and-forget initializeSingletonCacheService settle, then swap - // in a controllable cache mock. - await new Promise((resolve) => setImmediate(resolve)); + processor = createProcessor(); getSessionsByContact = jest.fn(); (processor as any).sessionCacheService = { getSessionsByContact }; }); @@ -94,19 +105,7 @@ describe('JourneyTriggerProcessor dispatch fail-fast guard (EVO-1764)', () => { (processor as any).triggerJourneyExecution(event, journey); beforeEach(async () => { - processor = new JourneyTriggerProcessor( - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - ); - ['log', 'warn', 'error'].forEach((m) => - jest - .spyOn((processor as any).logger, m) - .mockImplementation(() => undefined), - ); - await new Promise((resolve) => setImmediate(resolve)); + processor = createProcessor(); handle = { firstExecutionRunId: 'run-1', @@ -205,19 +204,7 @@ describe('JourneyTriggerProcessor messageId idempotency (EVO-1896)', () => { (processor as any).triggerJourneyExecution(event, journey); beforeEach(async () => { - processor = new JourneyTriggerProcessor( - {} as any, - {} as any, - {} as any, - {} as any, - {} as any, - ); - ['log', 'warn', 'error'].forEach((m) => - jest - .spyOn((processor as any).logger, m) - .mockImplementation(() => undefined), - ); - await new Promise((resolve) => setImmediate(resolve)); + processor = createProcessor(); workflowStart = jest.fn().mockResolvedValue({ firstExecutionRunId: 'run-1', @@ -286,19 +273,7 @@ describe('JourneyTriggerProcessor consumer gating (EVO-1764 A1)', () => { beforeEach(async () => { warmActiveJourneysCache = jest.fn().mockResolvedValue(0); - processor = new JourneyTriggerProcessor( - { warmActiveJourneysCache } as any, - {} as any, - {} as any, - {} as any, - {} as any, - ); - ['log', 'warn', 'error'].forEach((m) => - jest - .spyOn((processor as any).logger, m) - .mockImplementation(() => undefined), - ); - await new Promise((resolve) => setImmediate(resolve)); + processor = createProcessor({ warmActiveJourneysCache }); initializeKafkaConsumer = jest.fn().mockResolvedValue(undefined); startConsuming = jest.fn().mockResolvedValue(undefined); @@ -377,35 +352,33 @@ describe('JourneyTriggerProcessor contact-less events', () => { let checkWaitingSessions: jest.Mock; let triggerJourneyExecution: jest.Mock; - const event = (contactId?: string) => + const event = (overrides: Record = {}) => ({ messageId: 'm-1', - contactId, + contactId: 'contact-1', eventName: 'webhook.sendgrid', eventType: 'track', properties: '{}', traits: '{}', timestamp: '2026-08-23T00:00:00.000Z', + ...overrides, }) as any; - const analyze = (contactId?: string) => - (processor as any).analyzeEventForJourneyTriggers(event(contactId)); + const analyze = (overrides: Record = {}) => + (processor as any).analyzeEventForJourneyTriggers(event(overrides)); - beforeEach(async () => { + // Same entry point the Kafka consumer uses, so the guard is exercised where it + // actually sits rather than by reaching into the private method. + const consume = (overrides: Record = {}) => + (processor as any).processMessage({ + topic: 'journey-triggers', + partition: 0, + message: { value: Buffer.from(JSON.stringify(event(overrides))) }, + }); + + beforeEach(() => { findActive = jest.fn().mockResolvedValue([{ id: 'journey-1', name: 'J1' }]); - processor = new JourneyTriggerProcessor( - { findActive } as any, - {} as any, - {} as any, - {} as any, - {} as any, - ); - ['log', 'warn', 'error'].forEach((m) => - jest - .spyOn((processor as any).logger, m) - .mockImplementation(() => undefined), - ); - await new Promise((resolve) => setImmediate(resolve)); + processor = createProcessor({ findActive }); checkWaitingSessions = jest.fn().mockResolvedValue(undefined); triggerJourneyExecution = jest.fn().mockResolvedValue(undefined); @@ -416,31 +389,120 @@ describe('JourneyTriggerProcessor contact-less events', () => { .mockResolvedValue(true); }); + const expectSkipped = () => { + expect(triggerJourneyExecution).not.toHaveBeenCalled(); + expect(checkWaitingSessions).not.toHaveBeenCalled(); + expect(findActive).not.toHaveBeenCalled(); + }; + it.each([ ['empty', ''], ['whitespace only', ' '], ['absent', undefined], ])('does not dispatch a workflow when contactId is %s', async (_, id) => { - await analyze(id); + await analyze({ contactId: id }); - expect(triggerJourneyExecution).not.toHaveBeenCalled(); - expect(checkWaitingSessions).not.toHaveBeenCalled(); - expect(findActive).not.toHaveBeenCalled(); + expectSkipped(); }); - it('logs the skip instead of dropping the event silently', async () => { - await analyze(''); + // No handler can match a nameless event: all eight compare eventName against a + // concrete string, so letting one through only buys a full session-cache scan. + it.each([ + ['empty', ''], + ['whitespace only', ' '], + ['absent', undefined], + ])('does not dispatch a workflow when eventName is %s', async (_, name) => { + await analyze({ eventName: name }); + + expectSkipped(); + }); - expect((processor as any).logger.warn).toHaveBeenCalledWith( + it('skips before any lookup when the event arrives from the topic', async () => { + await consume({ contactId: '' }); + + expectSkipped(); + }); + + it('logs the skip at debug — this is routine traffic, not an anomaly', async () => { + await analyze({ contactId: '' }); + + expect((processor as any).logger.debug).toHaveBeenCalledWith( expect.stringContaining('no contactId'), expect.objectContaining({ eventName: 'webhook.sendgrid' }), ); + expect((processor as any).logger.warn).not.toHaveBeenCalled(); + }); + + it('reports a running total so the volume stays visible', async () => { + for (let i = 0; i < 1000; i++) { + await analyze({ contactId: '' }); + } + + expect((processor as any).logger.log).toHaveBeenCalledWith( + expect.stringContaining('1000 events skipped so far'), + ); }); it('still dispatches for an event that carries a contact', async () => { - await analyze('contact-1'); + await analyze(); expect(checkWaitingSessions).toHaveBeenCalledTimes(1); expect(triggerJourneyExecution).toHaveBeenCalledTimes(1); }); + + it('still dispatches an event consumed from the topic', async () => { + await consume(); + + expect(triggerJourneyExecution).toHaveBeenCalledTimes(1); + }); +}); + +describe('JourneyTriggerProcessor dispatch guard for contact-less events', () => { + let processor: JourneyTriggerProcessor; + let checkForActiveOrWaitingSessions: jest.Mock; + let getTemporalClient: jest.Mock; + + const journey = { id: 'journey-1', name: 'J1' }; + + const dispatch = (contactId?: string) => + (processor as any).triggerJourneyExecution( + { + messageId: 'm-1', + contactId, + eventName: 'evt', + eventType: 'track', + properties: '{}', + timestamp: '2026-08-23T00:00:00.000Z', + }, + journey, + ); + + beforeEach(() => { + processor = createProcessor(); + checkForActiveOrWaitingSessions = jest.fn().mockResolvedValue(false); + getTemporalClient = jest.fn(); + (processor as any).checkForActiveOrWaitingSessions = + checkForActiveOrWaitingSessions; + (processor as any).getTemporalClient = getTemporalClient; + }); + + it.each([ + ['empty', ''], + ['whitespace only', ' '], + ['absent', undefined], + ])('refuses to dispatch when contactId is %s', async (_, contactId) => { + await dispatch(contactId); + + expect(checkForActiveOrWaitingSessions).not.toHaveBeenCalled(); + expect(getTemporalClient).not.toHaveBeenCalled(); + }); + + it('logs at error — getting here means the intake guard was bypassed', async () => { + await dispatch(''); + + expect((processor as any).logger.error).toHaveBeenCalledWith( + expect.stringContaining('intake guard was bypassed'), + expect.objectContaining({ journeyId: 'journey-1' }), + ); + }); }); diff --git a/src/modules/journeys/services/journey-trigger-processor.service.ts b/src/modules/journeys/services/journey-trigger-processor.service.ts index 651b40e..1b1314d 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.ts @@ -28,6 +28,8 @@ import { BaseTrigger, } from './triggers'; +const SKIPPED_EVENT_LOG_INTERVAL = 1000; + export interface JourneyTriggerEvent { messageId: string; contactId: string; @@ -45,6 +47,7 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { private readonly logger = new CustomLoggerService( JourneyTriggerProcessor.name, ); + private skippedUndispatchableEvents = 0; private consumer: Consumer | null = null; private kafka: Kafka | null = null; private readonly config = getProcessingConfig(); @@ -281,19 +284,7 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { `🔍 Analyzing event for journey triggers: ${event.eventName}`, ); - // Sessions, the re-entry guard and the dedup claim are all keyed by - // contact: a contact-less event can only dispatch a workflow nobody can - // act on, and collapses every such event onto the same cache key. The - // e-mail deliverability rows on this bus carry an empty contact_id. - if (!event.contactId || event.contactId.trim() === '') { - this.logger.warn( - `⏭️ Skipping event ${event.eventName} — no contactId, nothing contact-scoped can run`, - { - messageId: event.messageId, - eventName: event.eventName, - anonymousId: event.anonymousId, - }, - ); + if (!this.isDispatchable(event)) { return; } @@ -334,6 +325,45 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { } } + /** + * Events that can neither start nor resume anything. Letting one through costs + * a full session-cache scan (getSessionsByContact reads every session, then + * filters) and can open a session under an empty contact id that every later + * contact-less event then finds. Deliverability callbacks and anonymous link + * clicks arrive without a contact and are the bulk of this bus, so the skip is + * routine: debug per event, info only for the running total. + */ + private isDispatchable(event: JourneyTriggerEvent): boolean { + const missing = !event.contactId?.trim() + ? 'contactId' + : !event.eventName?.trim() + ? 'eventName' + : null; + + if (!missing) { + return true; + } + + this.skippedUndispatchableEvents += 1; + + this.logger.debug( + `⏭️ Skipping event ${event.eventName} — no ${missing}, nothing contact-scoped can run`, + { + messageId: event.messageId, + eventName: event.eventName, + anonymousId: event.anonymousId, + }, + ); + + if (this.skippedUndispatchableEvents % SKIPPED_EVENT_LOG_INTERVAL === 0) { + this.logger.log( + `⏭️ ${this.skippedUndispatchableEvents} events skipped so far — no contactId or no eventName`, + ); + } + + return false; + } + private async matchesJourneyTrigger( event: JourneyTriggerEvent, journey: any, @@ -688,6 +718,20 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { event: JourneyTriggerEvent, journey: any, ): Promise { + // Unlike the intake guard above, reaching here without a contact IS an + // anomaly: every path into this method is supposed to have filtered already. + if (!event.contactId?.trim()) { + this.logger.error( + '❌ Refusing to dispatch without a contactId — the intake guard was bypassed', + { + journeyId: journey.id, + eventName: event.eventName, + messageId: event.messageId, + }, + ); + return; + } + this.logger.log( `🚀 Triggering journey execution: ${journey.id} (${journey.name}) for contact ${event.contactId}`, ); From e1591e568c494714032ed829aeeee654d0558b0e Mon Sep 17 00:00:00 2001 From: Matheus Pastorini Date: Sun, 23 Aug 2026 21:12:10 -0300 Subject: [PATCH 21/29] fix(segments): whereProperties precisa ler traits em evento de contato (CRM-241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toda condição whereProperties de nó Performed / LastPerformed era montada como JSONExtractString(properties, ''), mas os eventos de contato que o CRM emite chegam ao ClickHouse como `identify`: o payload inteiro vai em `traits` e a coluna `properties` fica `{}` (EvoFlow::PayloadBuilder.build_identify não monta a chave `properties`). O filtro nunca casava linha nenhuma, e falhava em SILÊNCIO nos dois sentidos — o SQL era válido, rodava sem erro e simplesmente lia a coluna errada: - Equals / Contains / Exists -> '' = 'VIP' sempre falso -> segmento vazio - NotEquals / NotContains -> '' != 'VIP' sempre verdade -> o filtro NÃO filtra e todo mundo entra. Numa campanha, é público ERRADO, não vazio. A extração passa a escolher a COLUNA e extrair uma vez: JSONExtractString(if(JSONHas(properties, 'p'), properties, traits), 'p') `JSONHas`, e não `!= ''`, porque uma chave presente com valor vazio é resposta legítima do produtor: cair para `traits` nesse caso trocaria um vazio deliberado pelo valor de outra fonte e quebraria NotExists. Prefere `properties` e só cai para `traits` quando a chave não existe, então evento `track` — que grava `properties` e deixa `traits` em `{}` — produz SQL equivalente ao de antes. Escolher a coluna (em vez de extrair das duas e comparar) mantém em duas operações JSON por linha. `template_id` do nó WhatsApp/Web/SMS ficou intacto de propósito: não é whereProperties e lê `properties` legitimamente. Performed e LastPerformed carregavam switches DUPLICADOS para montar essa condição, e eles estavam fora de sincronia: o do LastPerformed não listava `GreaterThan`, `GreaterThanOrEqual`, `LessThan`, `LessThanOrEqual` nem `NotExists`, que caíam no `default` e viravam IGUALDADE — também em silêncio, porque o SQL seguia válido ("score > 10" selecionava score == 10). Em vez de completar a lista da cópia, o filtro virou um método só (`buildEventPropertyCondition`) usado pelos dois nós: a causa era a duplicação. Operador desconhecido continua caindo em igualdade, mas agora loga warning em vez de degradar calado. Testes: - Spec do builder (35 exemplos) fixando o SQL por operador, o escaping de path e valor, a não-regressão do template_id, e um bloco de PARIDADE que exige condição idêntica entre Performed e LastPerformed nos 10 operadores — é o teste que quebra se a cópia for recriada. - Teste de integração (test/segment-where-properties.e2e-spec.ts, 8 exemplos) que EXECUTA a condição gerada contra um ClickHouse real e assere os CONTATOS retornados, que é o critério de aceite do card. O spec unitário não pega este bug: a expressão antiga é SQL válido, o que falhava era o casamento. Por isso um dos exemplos roda a expressão ANTIGA contra as mesmas linhas e prova que ela erra nas duas direções — o teste não é tautológico. Provas negativas: revertendo a leitura de traits, 18 dos 20 exemplos originais falham e 6 dos 8 do e2e; reintroduzindo o switch reduzido do LastPerformed, falham exatamente os 9 que cobrem os operadores numéricos e a paridade. Notas do teste de integração: - FALHA se o ClickHouse não estiver acessível, em vez de se auto-pular. Um skip condicional deixaria a suíte VERDE sem ter exercitado nada — o mesmo modo de falhar silencioso que este commit corrige. (E `it.skip` por flag nem funcionaria: o Jest avalia isso na coleta, antes do beforeAll.) - Usa uma tabela ESPELHO (`CREATE TABLE ... AS contact_events`), criada e destruída pelo teste. `clickhouse.service.ts` cria a `events_to_journey_triggers_mv` escrevendo num engine Kafka SEMPRE, inclusive onde o broker é RabbitMQ (`BROKER_TYPE=rabbitmq`, o compose community); sem Kafka atendendo, o INSERT na tabela real fica preso até o timeout. A espelho mantém o teste determinístico nos dois ambientes, com schema idêntico e sem tocar na tabela de produção. - `async_insert: 0` fixado no cliente: o ClickHouse do community traz `async_insert=1` no users.xml e o do ecosystem fica no default 0; com `wait_for_async_insert=1` (ligado nos dois) o cliente ficaria preso ao flush do buffer. Validado nos dois ambientes, 8/8 em cada: community :18123 ClickHouse 26.7 broker rabbitmq ecosystem :18124 ClickHouse 25.8 broker kafka Além do e2e, a expressão foi conferida direto na `contact_events` REAL do ecosystem (sem espelho): properties={} / traits={"labelName":"VIP"}, expressão antiga devolve vazio e a corrigida devolve VIP — o bug e o fix reproduzidos na tabela de produção, com Kafka e MVs ativos. Suíte completa: 1099 exemplos passando. As 4 suites que falham são pré-existentes (campaigns.controller + 3 nodes temporal/evoai) e falham igual sem este commit. tsc --noEmit limpo. --- ...t-clickhouse-query-builder.service.spec.ts | 226 ++++++++++++++++ ...egment-clickhouse-query-builder.service.ts | 130 +++++---- test/segment-where-properties.e2e-spec.ts | 246 ++++++++++++++++++ 3 files changed, 546 insertions(+), 56 deletions(-) create mode 100644 src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts create mode 100644 test/segment-where-properties.e2e-spec.ts diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts new file mode 100644 index 0000000..d8a516c --- /dev/null +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts @@ -0,0 +1,226 @@ +import { SegmentClickHouseQueryBuilderService } from './segment-clickhouse-query-builder.service'; +import { Segment, SegmentNodeType } from '../entities/segment.entity'; + +/** + * CRM-241: whereProperties filters used to read only the `properties` column. + * Contact events reach ClickHouse as `identify`, which puts the whole payload + * in `traits` and leaves `properties` at `{}` — so those filters matched + * nothing, and failed silently in both directions (Equals returned an empty + * segment, NotEquals let every contact through). + * + * Verified against a real event posted through POST /api/v1/events/identify: + * properties: {} + * traits: {"labelName":"VIP","labelId":"lb-99","source":"crm"} + * With the old expression, `= 'VIP'` was 0 and `!= 'VIP'` was 1. With the new + * one, 1 and 0 respectively — while a `track` row (properties filled, traits + * `{}`) evaluated identically under both. + * + * This spec pins the SQL. That the SQL actually MATCHES the right rows is proved + * by test/segment-where-properties.e2e-spec.ts, which runs it against a live + * ClickHouse (green on both environments: community 26.7 and ecosystem 25.8). + */ +describe('SegmentClickHouseQueryBuilderService — whereProperties (CRM-241)', () => { + const service = new SegmentClickHouseQueryBuilderService(); + const segment = { id: 'seg-241' } as Segment; + + /** The extraction the builder is expected to emit for a given path. */ + const extract = (path: string) => + `JSONExtractString(if(JSONHas(properties, '${path}'), ` + + `properties, traits), '${path}')`; + + const performed = (properties: unknown[], event = 'contact.label.added') => + service.segmentNodeToStateSubQuery(segment, { + id: 'n1', + type: SegmentNodeType.Performed, + event, + properties, + } as any)[0].condition; + + const lastPerformed = ( + whereProperties: unknown[], + event = 'contact.label.added', + ) => + service.segmentNodeToStateSubQuery(segment, { + id: 'n2', + type: SegmentNodeType.LastPerformed, + event, + whereProperties, + } as any)[0].condition; + + const prop = (path: string, type: string, value?: string) => ({ + path, + operator: { type, ...(value === undefined ? {} : { value }) }, + }); + + describe('Performed', () => { + it('falls back to traits for a label filter', () => { + const condition = performed([prop('labelName', 'Equals', 'VIP')]); + + expect(condition).toContain(`${extract('labelName')} = 'VIP'`); + // The bug: reading properties alone never matched an identify row. + expect(condition).not.toContain( + `JSONExtractString(properties, 'labelName') = 'VIP'`, + ); + }); + + it('falls back to traits for a custom attribute filter', () => { + const condition = performed( + [prop('attributeName', 'Equals', 'plano')], + 'contact.custom_attribute.changed', + ); + + expect(condition).toContain(`${extract('attributeName')} = 'plano'`); + }); + + // The negative operators are the dangerous half: on an identify row the old + // expression compared '' against the value, which was always true — the + // filter did not filter, and a campaign reached the wrong audience. + it.each([ + ['NotEquals', `${extract('labelName')} != 'VIP'`], + ['NotContains', `${extract('labelName')} NOT LIKE '%VIP%'`], + ])('%s filters instead of passing everyone through', (type, expected) => { + expect(performed([prop('labelName', type, 'VIP')])).toContain(expected); + }); + + it.each([ + ['Contains', `${extract('labelName')} LIKE '%VIP%'`], + ['Exists', `${extract('labelName')} != ''`], + ['NotExists', `${extract('labelName')} = ''`], + ])('%s uses the same extraction', (type, expected) => { + expect(performed([prop('labelName', type, 'VIP')])).toContain(expected); + }); + + it.each([ + ['GreaterThan', '>'], + ['GreaterThanOrEqual', '>='], + ['LessThan', '<'], + ['LessThanOrEqual', '<='], + ])('%s wraps the extraction in toFloat64OrNull', (type, sqlOp) => { + expect(performed([prop('score', type, '10')])).toContain( + `toFloat64OrNull(${extract('score')}) ${sqlOp} 10`, + ); + }); + + it('applies the fallback to every property when several are combined', () => { + const condition = performed([ + prop('labelName', 'Equals', 'VIP'), + prop('source', 'Equals', 'crm'), + ]); + + expect(condition).toContain(`${extract('labelName')} = 'VIP'`); + expect(condition).toContain(`${extract('source')} = 'crm'`); + }); + }); + + describe('LastPerformed', () => { + it('falls back to traits for a label filter', () => { + const condition = lastPerformed([prop('labelName', 'Equals', 'VIP')]); + + expect(condition).toContain(`${extract('labelName')} = 'VIP'`); + expect(condition).not.toContain( + `JSONExtractString(properties, 'labelName') = 'VIP'`, + ); + }); + + it.each([ + ['NotEquals', `${extract('labelName')} != 'VIP'`], + ['NotContains', `${extract('labelName')} NOT LIKE '%VIP%'`], + ['Contains', `${extract('labelName')} LIKE '%VIP%'`], + ['Exists', `${extract('labelName')} != ''`], + ])('%s uses the same extraction', (type, expected) => { + expect(lastPerformed([prop('labelName', type, 'VIP')])).toContain( + expected, + ); + }); + + // CRM-241: the LastPerformed switch was a smaller copy of the Performed one + // and did not list these operators — they fell through to `default` and became + // EQUALITY, with valid SQL and no sign of error. `score > 10` selected + // score == 10. + it.each([ + ['GreaterThan', '>'], + ['GreaterThanOrEqual', '>='], + ['LessThan', '<'], + ['LessThanOrEqual', '<='], + ])('%s really compares, instead of degrading to equality', (type, sqlOp) => { + const condition = lastPerformed([prop('score', type, '10')]); + + expect(condition).toContain( + `toFloat64OrNull(${extract('score')}) ${sqlOp} 10`, + ); + expect(condition).not.toContain(`${extract('score')} = '10'`); + }); + + it('NotExists is handled explicitly, not by the default branch', () => { + expect(lastPerformed([prop('labelName', 'NotExists')])).toContain( + `${extract('labelName')} = ''`, + ); + }); + }); + + // The duplicated switch was the root cause of the drift. Now that there is only + // one, both nodes must emit exactly the same condition for the same property — + // this block fails if anyone recreates the copy. + describe('Performed and LastPerformed must not diverge', () => { + it.each([ + ['Equals'], + ['NotEquals'], + ['Contains'], + ['NotContains'], + ['Exists'], + ['NotExists'], + ['GreaterThan'], + ['GreaterThanOrEqual'], + ['LessThan'], + ['LessThanOrEqual'], + ])('%s produces the same condition on both nodes', (type) => { + const p = prop('score', type, '10'); + const doPerformed = performed([p]).replace( + `event_name = 'contact.label.added' AND `, + '', + ); + const doLast = lastPerformed([p]).replace( + `event_name = 'contact.label.added' AND `, + '', + ); + + expect(doLast).toBe(doPerformed); + }); + }); + + describe('non-contact events keep reading properties', () => { + // A track event (campaign/message) fills `properties` and leaves `traits` + // at `{}`. JSONHas is true there, so the fallback never fires and the SQL + // is equivalent to what the builder emitted before this fix. + it('templateId on a WhatsApp node still reads properties directly', () => { + const condition = service.segmentNodeToStateSubQuery(segment, { + id: 'n3', + type: SegmentNodeType.WhatsApp, + templateId: 'tpl-42', + } as any)[0].condition; + + expect(condition).toContain( + `JSONExtractString(properties, 'template_id') = 'tpl-42'`, + ); + expect(condition).not.toContain('JSONHas('); + }); + }); + + describe('escaping', () => { + // The escaping moved inside the extraction helper; make sure it did not + // get dropped on the way. The path is interpolated three times now, so a + // regression here would be three injection points instead of one. + it('sanitizes a quote in the property path', () => { + const condition = performed([prop("label'--", 'Equals', 'VIP')]); + + expect(condition).not.toContain(`'label'--'`); + expect(condition).toContain('JSONHas('); + }); + + it('sanitizes a quote in the value', () => { + const condition = performed([prop('labelName', 'Equals', "VIP'--")]); + + expect(condition).not.toContain(`= 'VIP'--'`); + }); + }); +}); diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index eccbbcc..c9b93db 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -60,6 +60,77 @@ export class SegmentClickHouseQueryBuilderService { ); } + // CRM-241: an event property can arrive in either column. Contact events are + // emitted as `identify`, so the whole payload lands in `traits` and + // `properties` stays `{}` (EvoFlow::PayloadBuilder.build_identify never + // builds a `properties` key). Reading only `properties` made every + // whereProperties filter miss, silently and in both directions: Equals + // returned an empty segment, NotEquals let everyone through. + // + // Prefer `properties`, fall back to `traits` only when the key is absent, so + // `track` events — which write `properties` and leave `traits` at `{}` — + // produce the exact same SQL they did before. + // + // JSONHas rather than `!= ''`: a key that is present but empty is a real + // answer from the producer. Falling back on it would swap a deliberate empty + // value for an unrelated one, and would break NotExists. + // + // The `if` picks the COLUMN and extracts once, instead of extracting from both + // and choosing between the results — two JSON operations per row instead of + // three, on a filter that runs over the whole event table. + private extractEventProperty(path: unknown): string { + const escapedPath = this.escapeSql(path); + return ( + `JSONExtractString(if(JSONHas(properties, '${escapedPath}'), ` + + `properties, traits), '${escapedPath}')` + ); + } + + // CRM-241: one event-property filter, for Performed AND LastPerformed. The two + // carried DUPLICATED switches that drifted apart: LastPerformed's listed neither + // the numeric operators nor NotExists, so `GreaterThan` fell through to `default` + // and became EQUALITY — silently, because the SQL stayed valid. A single method + // removes the possibility of them diverging again. + private buildEventPropertyCondition(prop: any): string { + const value = prop?.operator?.value || ''; + const operator = prop?.operator?.type || 'Equals'; + const extract = this.extractEventProperty(prop?.path); + const escapedValue = this.escapeSql(value); + const likeValue = this.escapeLike(value); + const numericValue = this.escapeNumeric(value); + + switch (operator) { + case 'Equals': + return `${extract} = '${escapedValue}'`; + case 'NotEquals': + return `${extract} != '${escapedValue}'`; + case 'Contains': + return `${extract} LIKE '%${likeValue}%'`; + case 'NotContains': + return `${extract} NOT LIKE '%${likeValue}%'`; + case 'GreaterThan': + return `toFloat64OrNull(${extract}) > ${numericValue}`; + case 'GreaterThanOrEqual': + return `toFloat64OrNull(${extract}) >= ${numericValue}`; + case 'LessThan': + return `toFloat64OrNull(${extract}) < ${numericValue}`; + case 'LessThanOrEqual': + return `toFloat64OrNull(${extract}) <= ${numericValue}`; + case 'Exists': + return `${extract} != ''`; + case 'NotExists': + return `${extract} = ''`; + default: + // An unknown operator still degrades to equality, as before — but it now + // says so, instead of degrading in silence. + this.logger.warn( + `Unknown property operator '${operator}' on path '${prop?.path}'; ` + + `falling back to equality.`, + ); + return `${extract} = '${escapedValue}'`; + } + } + /** * Convert segment nodes to state sub-queries using modular builders */ @@ -370,40 +441,8 @@ export class SegmentClickHouseQueryBuilderService { // Adicionar condições de propriedades se houver if (performedNode.properties && performedNode.properties.length > 0) { - const propertyConditions = performedNode.properties.map( - (prop: any) => { - const value = prop.operator?.value || ''; - const operator = prop.operator?.type || 'Equals'; - const path = this.escapeSql(prop.path); - const escapedValue = this.escapeSql(value); - const likeValue = this.escapeLike(value); - const numericValue = this.escapeNumeric(value); - - switch (operator) { - case 'Equals': - return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; - case 'NotEquals': - return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; - case 'Contains': - return `JSONExtractString(properties, '${path}') LIKE '%${likeValue}%'`; - case 'NotContains': - return `JSONExtractString(properties, '${path}') NOT LIKE '%${likeValue}%'`; - case 'GreaterThan': - return `toFloat64OrNull(JSONExtractString(properties, '${path}')) > ${numericValue}`; - case 'GreaterThanOrEqual': - return `toFloat64OrNull(JSONExtractString(properties, '${path}')) >= ${numericValue}`; - case 'LessThan': - return `toFloat64OrNull(JSONExtractString(properties, '${path}')) < ${numericValue}`; - case 'LessThanOrEqual': - return `toFloat64OrNull(JSONExtractString(properties, '${path}')) <= ${numericValue}`; - case 'Exists': - return `JSONExtractString(properties, '${path}') != ''`; - case 'NotExists': - return `JSONExtractString(properties, '${path}') = ''`; - default: - return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; - } - }, + const propertyConditions = performedNode.properties.map((prop: any) => + this.buildEventPropertyCondition(prop), ); condition += ` AND (${propertyConditions.join(' AND ')})`; @@ -478,28 +517,7 @@ export class SegmentClickHouseQueryBuilderService { lastPerformedNode.whereProperties.length > 0 ) { const propertyConditions = lastPerformedNode.whereProperties.map( - (prop: any) => { - const value = prop.operator?.value || ''; - const operator = prop.operator?.type || 'Equals'; - const path = this.escapeSql(prop.path); - const escapedValue = this.escapeSql(value); - const likeValue = this.escapeLike(value); - - switch (operator) { - case 'Equals': - return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; - case 'NotEquals': - return `JSONExtractString(properties, '${path}') != '${escapedValue}'`; - case 'Contains': - return `JSONExtractString(properties, '${path}') LIKE '%${likeValue}%'`; - case 'NotContains': - return `JSONExtractString(properties, '${path}') NOT LIKE '%${likeValue}%'`; - case 'Exists': - return `JSONExtractString(properties, '${path}') != ''`; - default: - return `JSONExtractString(properties, '${path}') = '${escapedValue}'`; - } - }, + (prop: any) => this.buildEventPropertyCondition(prop), ); condition += ` AND (${propertyConditions.join(' AND ')})`; diff --git a/test/segment-where-properties.e2e-spec.ts b/test/segment-where-properties.e2e-spec.ts new file mode 100644 index 0000000..4924ce1 --- /dev/null +++ b/test/segment-where-properties.e2e-spec.ts @@ -0,0 +1,246 @@ +import { createClient, ClickHouseClient } from '@clickhouse/client'; +import { SegmentClickHouseQueryBuilderService } from '../src/modules/segments/services/segment-clickhouse-query-builder.service'; +import { + Segment, + SegmentNodeType, +} from '../src/modules/segments/entities/segment.entity'; + +/** + * CRM-241 — end-to-end proof of the card's acceptance criteria: a Performed node + * filtered by `labelName` / `attributeName` must RETURN THE RIGHT CONTACTS, not + * merely produce the expected SQL string. + * + * The unit spec (segment-clickhouse-query-builder.service.spec.ts) pins the SQL. + * It cannot prove the SQL matches any row — and that is exactly where the bug + * lived: the old expression was valid SQL, ran without error, and silently matched + * nothing because it read the wrong column. + * + * Here the condition the builder generates is executed against a real ClickHouse, + * over rows shaped the way the pipeline actually writes them: + * - contact event → arrives as `identify`: payload in `traits`, `properties` = {} + * - campaign event → arrives as `track`: payload in `properties`, `traits` = {} + * + * Requires a ClickHouse — and FAILS if none is reachable, instead of skipping. + * Runs the same on both environments; point the URL at whichever is up: + * CLICKHOUSE_URL=http://localhost:18123 npm run test:e2e -- segment-where-properties # community + * CLICKHOUSE_URL=http://localhost:18124 npm run test:e2e -- segment-where-properties # ecosystem + * + * The scenario lives in a mirror table the test creates and drops, so nothing is + * read from or written to the real `contact_events`. + */ +const URL = process.env.CLICKHOUSE_URL || 'http://localhost:18123'; +const DB = process.env.CLICKHOUSE_DB || 'evo_campaign'; +const USER = process.env.CLICKHOUSE_USERNAME || 'default'; +const PASS = process.env.CLICKHOUSE_PASSWORD || 'password'; + +// MIRROR table, created with `AS contact_events` — same schema, without the +// materialized views attached to it. +// +// The reason is concrete: `clickhouse.service.ts` always creates +// `events_to_journey_triggers_mv` writing into `journey_trigger_kafka_queue` (a +// Kafka engine), including on an environment whose broker is RabbitMQ +// (`BROKER_TYPE=rabbitmq`, the community compose). With no Kafka broker answering, +// every INSERT into the real table hangs until the timeout — verified over plain +// HTTP, outside the test. On the ecosystem, which runs an actual Kafka, the same +// INSERT answers in ~2s. +// +// The mirror keeps the test deterministic on BOTH environments and preserves +// isolation: what it has to prove is the EXPRESSION the builder generates against +// realistically shaped rows, and the copied schema is identical to production's. +// +// Verified against ClickHouse 26.7 (community, :18123) and 25.8 (ecosystem, :18124). +const TABLE = `crm241_e2e_${Date.now()}_${process.pid}`; +const TAG = 'crm241'; // id prefix for the scenario rows, for readability only + +const VIP = `${TAG}-vip`; +const COMUM = `${TAG}-comum`; +const CAMPANHA = `${TAG}-campanha`; + +describe('CRM-241 whereProperties matches real rows in ClickHouse (e2e)', () => { + let client: ClickHouseClient; + const builder = new SegmentClickHouseQueryBuilderService(); + const segment = { id: 'seg-241-e2e' } as Segment; + + beforeAll(async () => { + client = createClient({ + url: URL, + database: DB, + username: USER, + password: PASS, + request_timeout: 60_000, + clickhouse_settings: { + // The two environments differ here: the community compose ships + // `async_insert=1` in users.xml, the ecosystem one stays at the default 0. + // With `async_insert=1` (and `wait_for_async_insert=1`, on in both) the + // client is held waiting for the buffer flush instead of writing straight + // through. Pinning 0 makes the INSERT synchronous and the test + // deterministic wherever it runs. + async_insert: 0, + }, + }); + + try { + await client.query({ query: 'SELECT 1', format: 'JSONEachRow' }); + } catch (error) { + // Fail, do not skip. A conditional skip here would be worse than having no + // test at all: the suite would go GREEN without exercising anything — the + // same silent-failure mode CRM-241 fixes. (And a flag-driven `it.skip` would + // not even work: Jest evaluates that at collection time, before this hook.) + throw new Error( + `[CRM-241 e2e] ClickHouse unreachable at ${URL} (db=${DB}, user=${USER}): ` + + `${(error as Error).message}. Bring the compose up, or point ` + + `CLICKHOUSE_URL/CLICKHOUSE_USERNAME/CLICKHOUSE_PASSWORD at a live one.`, + ); + } + + await client.command({ + query: `CREATE TABLE ${DB}.${TABLE} AS ${DB}.contact_events`, + }); + + const now = new Date().toISOString().replace('T', ' ').substring(0, 23); + const row = ( + id: string, + type: 'identify' | 'track', + event: string, + properties: object, + traits: object, + ) => ({ + contact_id: id, + contact_or_anonymous_id: id, + event_type: type, + event_name: event, + properties: JSON.stringify(properties), + traits: JSON.stringify(traits), + message_id: `${id}-${event}`, + occurred_at: now, + processing_time: now, + message_raw: '{}', + }); + + await client.insert({ + table: `${DB}.${TABLE}`, + format: 'JSONEachRow', + values: [ + // Exactly how the CRM emits it: identify, everything in traits, properties + // empty. + row(VIP, 'identify', 'contact.label.added', {}, { labelName: 'VIP', labelId: 'lb-1' }), + row(COMUM, 'identify', 'contact.label.added', {}, { labelName: 'Comum', labelId: 'lb-2' }), + row( + COMUM, + 'identify', + 'contact.custom_attribute.changed', + {}, + { attributeName: 'plano', attributeValue: 'free' }, + ), + // Campaign event: track, everything in properties, traits empty. This is + // what proves reading `properties` did NOT regress. + row(CAMPANHA, 'track', 'whatsapp_sent', { template_id: 'tpl-42' }, {}), + ], + }); + }, 120_000); + + afterAll(async () => { + await client?.command({ query: `DROP TABLE IF EXISTS ${DB}.${TABLE}` }); + await client?.close(); + }); + + /** Runs the condition the builder produced and returns the contacts it selects. */ + const contactsFor = async (node: object): Promise => { + const [subQuery] = builder.segmentNodeToStateSubQuery(segment, node as any); + const rs = await client.query({ + query: + `SELECT DISTINCT contact_or_anonymous_id AS id FROM ${DB}.${TABLE} ` + + `WHERE ${subQuery.condition} ORDER BY id`, + format: 'JSONEachRow', + }); + return (await rs.json<{ id: string }>()).map((r) => r.id); + }; + + const performed = (event: string, properties: object[]) => ({ + id: 'n1', + type: SegmentNodeType.Performed, + event, + properties, + }); + + const prop = (path: string, type: string, value?: string) => ({ + path, + operator: { type, ...(value === undefined ? {} : { value }) }, + }); + + it('labelName = VIP returns only the VIP contact', async () => { + const ids = await contactsFor( + performed('contact.label.added', [prop('labelName', 'Equals', 'VIP')]), + ); + expect(ids).toEqual([VIP]); + }); + + it('attributeName = plano returns the contact holding the attribute', async () => { + const ids = await contactsFor( + performed('contact.custom_attribute.changed', [ + prop('attributeName', 'Equals', 'plano'), + ]), + ); + expect(ids).toEqual([COMUM]); + }); + + // The dangerous half of the bug: the old expression compared '' against the + // value, which was always true — the filter did not filter, and a campaign went + // out to the wrong audience. + it('labelName != VIP excludes the VIP instead of letting everyone through', async () => { + const ids = await contactsFor( + performed('contact.label.added', [prop('labelName', 'NotEquals', 'VIP')]), + ); + expect(ids).toEqual([COMUM]); + expect(ids).not.toContain(VIP); + }); + + it('labelName containing "VI" returns the VIP', async () => { + const ids = await contactsFor( + performed('contact.label.added', [prop('labelName', 'Contains', 'VI')]), + ); + expect(ids).toEqual([VIP]); + }); + + it('Exists on labelName returns both labelled contacts', async () => { + const ids = await contactsFor( + performed('contact.label.added', [prop('labelName', 'Exists')]), + ); + expect(ids).toEqual([COMUM, VIP]); + }); + + it('NotExists on labelName returns nobody who carries the label', async () => { + const ids = await contactsFor( + performed('contact.label.added', [prop('labelName', 'NotExists')]), + ); + expect(ids).toEqual([]); + }); + + // Non-regression: a track event is still read from `properties`. + it('template_id on a campaign event still matches', async () => { + const ids = await contactsFor( + performed('whatsapp_sent', [prop('template_id', 'Equals', 'tpl-42')]), + ); + expect(ids).toEqual([CAMPANHA]); + }); + + // Proof that this test is NOT tautological: the old expression, run against the + // very same rows, returns the wrong set in both directions. + it('the old expression gets it wrong both ways on these same rows', async () => { + const old = (path: string) => `JSONExtractString(properties, '${path}')`; + const run = async (where: string) => { + const rs = await client.query({ + query: + `SELECT DISTINCT contact_or_anonymous_id AS id FROM ${DB}.${TABLE} ` + + `WHERE event_name = 'contact.label.added' AND (${where}) ORDER BY id`, + format: 'JSONEachRow', + }); + return (await rs.json<{ id: string }>()).map((r) => r.id); + }; + + // Equals: used to return nothing — an empty segment. + expect(await run(`${old('labelName')} = 'VIP'`)).toEqual([]); + // NotEquals: used to return everyone, including the VIP that should be out. + expect(await run(`${old('labelName')} != 'VIP'`)).toEqual([COMUM, VIP]); + }); +}); From d3be84df7866aa38e959eec5a5356b3dfc6f5299 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sun, 23 Aug 2026 21:31:53 -0300 Subject: [PATCH 22/29] =?UTF-8?q?refactor(segments):=20unifica=20o=20filtr?= =?UTF-8?q?o=20de=20propriedade=20de=20evento=20e=20alcan=C3=A7a=20os=20pr?= =?UTF-8?q?ocessadores=20real-time=20(CRM-241)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Achados do code review do PR #118. O filtro de propriedade de evento vira um só, em `SegmentQueryUtils` (`extractEventProperty` + `buildEventPropertyCondition`). O builder do ClickHouse delega: os 35 exemplos do spec seguem passando sem qualquer ajuste, o que prova que o SQL emitido é idêntico. `atomic-processor.service.ts` (2 sítios) e `batch-processor.service.ts` carregavam mais duas cópias do MESMO filtro, lendo só `properties` — ou seja, o bug que o CRM-241 corrige, numa quarta e numa quinta superfície. São alcançáveis com `SEGMENT_COMPUTATION_TYPE=real-time`. Além da coluna errada, interpolavam `path`/`value`/`event`/`labelId` crus, sem escaping, e a cópia do batch lia `prop.key`/`prop.value` — shape que o front nunca emite, então o filtro resolvia para a string literal "undefined". As três passam a usar o builder compartilhado. O e2e passa a ser opt-in (`SEGMENT_E2E=1`), como o `tenant-isolation.e2e-spec.ts` da mesma pasta, para não somar uma suite falhando ao `npm run test:e2e`. A conexão vem das envs que o projeto já usa (`CLICKHOUSE_HOST`/`PORT`/`DATABASE`) em vez de `CLICKHOUSE_URL`/`CLICKHOUSE_DB` com default 18123, que não bate com o compose do próprio repo (8123). Ligado, ele continua FALHANDO — não pulando — se o servidor não responder. O `afterAll` disparava um segundo erro sem contexto quando o `beforeAll` falhava (o client já estava atribuído; `createClient` é lazy): agora só limpa o que criou, e varre tabelas espelho deixadas por uma run interrompida. Comentários novos em inglês e enxutos (CLAUDE.md), e o comentário que dizia que o path é interpolado três vezes estava errado: são duas (JSONHas + JSONExtractString). Testes: spec novo com 7 exemplos cobrindo os três sítios do módulo de processing (fallback para traits, escaping de aspas em path/valor/evento, e a não-regressão do shape errado do batch). Suíte completa: 1079 passando (era 1072); as 4 suites que falham são as mesmas pré-existentes (campaigns.controller + 3 nodes temporal/evoai). e2e 8/8 contra ClickHouse real, `tsc --noEmit` limpo. --- .../services/atomic-processor.service.ts | 28 +++--- .../services/batch-processor.service.ts | 17 ++-- .../segment-property-conditions.spec.ts | 97 +++++++++++++++++++ ...t-clickhouse-query-builder.service.spec.ts | 6 +- ...egment-clickhouse-query-builder.service.ts | 88 +++-------------- .../segments/utils/segment-query.utils.ts | 81 ++++++++++++++++ test/segment-where-properties.e2e-spec.ts | 58 ++++++++--- 7 files changed, 263 insertions(+), 112 deletions(-) create mode 100644 src/modules/processing/services/segment-property-conditions.spec.ts diff --git a/src/modules/processing/services/atomic-processor.service.ts b/src/modules/processing/services/atomic-processor.service.ts index b0c2914..c47b892 100644 --- a/src/modules/processing/services/atomic-processor.service.ts +++ b/src/modules/processing/services/atomic-processor.service.ts @@ -14,6 +14,7 @@ import { import { SegmentCircuitBreakerService } from '../../segments/services/segment-circuit-breaker.service'; import { SegmentMetricsService } from '../../segments/metrics/segment-metrics.service'; import { SegmentCacheService } from '../../cache/services/segment-cache.service'; +import { SegmentQueryUtils } from '../../segments/utils/segment-query.utils'; import { CustomLoggerService } from 'src/common/services/custom-logger.service'; export interface EventData { @@ -597,17 +598,15 @@ export class AtomicSegmentProcessor { switch (node.type) { case 'Performed': const performedNode = node as PerformedSegmentNode; - const eventName = performedNode.event; + const eventName = SegmentQueryUtils.sanitizeStringValue( + String(performedNode.event ?? ''), + ); - // Follow EXACT same logic as working batch system in performed-segment-builder.ts let condition = `event_name = '${eventName}'`; - // Add property conditions - follow exact batch logic if (performedNode.properties && performedNode.properties.length > 0) { const propertyConditions = performedNode.properties - .map((prop) => { - return `JSONExtractString(properties, '${prop.path}') = '${prop.operator.value}'`; - }) + .map((prop) => SegmentQueryUtils.buildEventPropertyCondition(prop)) .join(' AND '); condition = `event_name = '${eventName}' AND (${propertyConditions})`; @@ -617,7 +616,7 @@ export class AtomicSegmentProcessor { case 'Label': const labelNode = node as LabelSegmentNode; - return `JSONExtractString(properties, 'label') ${labelNode.condition === 'has' ? '=' : '!='} '${labelNode.labelId}'`; + return `JSONExtractString(properties, 'label') ${labelNode.condition === 'has' ? '=' : '!='} '${SegmentQueryUtils.sanitizeStringValue(String(labelNode.labelId ?? ''))}'`; case 'Everyone': return '1 = 1'; // Always matches @@ -661,18 +660,19 @@ export class AtomicSegmentProcessor { switch (node.type) { case 'Performed': const performedNode = node as PerformedSegmentNode; - const eventName = performedNode.event; - const times = performedNode.times || 1; + const eventName = SegmentQueryUtils.sanitizeStringValue( + String(performedNode.event ?? ''), + ); + const times = SegmentQueryUtils.sanitizeNumericValue( + performedNode.times || 1, + ); const operator = this.mapTimesOperator(performedNode.timesOperator); let condition = `countIf(event_name = '${eventName}') ${operator} ${times}`; - // Add property conditions if (performedNode.properties && performedNode.properties.length > 0) { const propertyConditions = performedNode.properties - .map((prop) => { - return `JSONExtractString(properties, '${prop.path}') = '${prop.operator.value}'`; - }) + .map((prop) => SegmentQueryUtils.buildEventPropertyCondition(prop)) .join(' AND '); condition = `countIf(event_name = '${eventName}' AND ${propertyConditions}) ${operator} ${times}`; @@ -682,7 +682,7 @@ export class AtomicSegmentProcessor { case 'Label': const labelNode = node as LabelSegmentNode; - return `JSONExtractString(properties, 'label') ${labelNode.condition === 'has' ? '=' : '!='} '${labelNode.labelId}'`; + return `JSONExtractString(properties, 'label') ${labelNode.condition === 'has' ? '=' : '!='} '${SegmentQueryUtils.sanitizeStringValue(String(labelNode.labelId ?? ''))}'`; case 'Everyone': return '1 = 1'; // Always true diff --git a/src/modules/processing/services/batch-processor.service.ts b/src/modules/processing/services/batch-processor.service.ts index 779646a..a12ace8 100644 --- a/src/modules/processing/services/batch-processor.service.ts +++ b/src/modules/processing/services/batch-processor.service.ts @@ -10,6 +10,7 @@ import { ClickHouseService } from '../clickhouse/clickhouse.service'; import { SegmentCircuitBreakerService } from '../../segments/services/segment-circuit-breaker.service'; import { SegmentMetricsService } from '../../segments/metrics/segment-metrics.service'; import { SegmentCacheService } from '../../cache/services/segment-cache.service'; +import { SegmentQueryUtils } from '../../segments/utils/segment-query.utils'; import { AtomicSegmentProcessor } from './atomic-processor.service'; import { IntelligentDebouncerService } from './intelligent-debouncer.service'; import { DeadLetterQueueService } from './dead-letter-queue.service'; @@ -719,29 +720,33 @@ export class BatchProcessorService implements OnModuleInit, OnModuleDestroy { const node = definition.nodes[0]; if (node.type === 'Performed') { - const eventName = node.event || node.value; + const eventName = SegmentQueryUtils.sanitizeStringValue( + String(node.event || node.value || ''), + ); let logic = `ce.event_name = '${eventName}'`; // Add property filters if (node.properties) { for (const prop of node.properties) { - logic += ` AND JSONExtractString(ce.properties, '${prop.key}') = '${prop.value}'`; + logic += ` AND ${SegmentQueryUtils.buildEventPropertyCondition(prop, 'ce')}`; } } // Add time window if (node.withinSeconds && node.withinSeconds > 0) { - logic += ` AND ce.occurred_at >= now() - INTERVAL ${node.withinSeconds} SECOND`; + logic += ` AND ce.occurred_at >= now() - INTERVAL ${SegmentQueryUtils.sanitizeNumericValue(node.withinSeconds)} SECOND`; } return logic; } if (node.type === 'Label') { - const labelId = node.labelId || node.value; + const labelId = SegmentQueryUtils.sanitizeStringValue( + String(node.labelId || node.value || ''), + ); return `EXISTS( - SELECT 1 FROM contact_labels cl - WHERE cl.contact_id = ce.contact_or_anonymous_id + SELECT 1 FROM contact_labels cl + WHERE cl.contact_id = ce.contact_or_anonymous_id AND cl.label_id = '${labelId}' )`; } diff --git a/src/modules/processing/services/segment-property-conditions.spec.ts b/src/modules/processing/services/segment-property-conditions.spec.ts new file mode 100644 index 0000000..9175c89 --- /dev/null +++ b/src/modules/processing/services/segment-property-conditions.spec.ts @@ -0,0 +1,97 @@ +import { AtomicSegmentProcessor } from './atomic-processor.service'; +import { BatchProcessorService } from './batch-processor.service'; +import { SegmentQueryUtils } from '../../segments/utils/segment-query.utils'; + +/** + * CRM-241 follow-up: the real-time processors carried their own copies of the + * event-property filter, reading only `properties` and interpolating the path + * and the value without escaping. Both now go through the shared builder. + * + * The builders are private and use no injected dependency, so the tests call + * them on a bare prototype instead of booting the Nest module. + */ +const atomic = Object.create( + AtomicSegmentProcessor.prototype, +) as AtomicSegmentProcessor; +const batch = Object.create( + BatchProcessorService.prototype, +) as BatchProcessorService; + +const performedNode = (properties: unknown[]) => ({ + id: 'n1', + type: 'Performed', + event: 'contact.label.added', + properties, +}); + +const labelFilter = { + path: 'labelName', + operator: { type: 'Equals', value: 'VIP' }, +}; + +describe('CRM-241 real-time processors share the event-property builder', () => { + const expected = (alias = '') => + `${SegmentQueryUtils.extractEventProperty('labelName', alias)} = 'VIP'`; + + it('AtomicSegmentProcessor.buildNodeLogicForBatch falls back to traits', () => { + const sql = (atomic as any).buildNodeLogicForBatch( + performedNode([labelFilter]), + ); + + expect(sql).toContain(expected()); + expect(sql).not.toContain(`JSONExtractString(properties, 'labelName')`); + }); + + it('AtomicSegmentProcessor.buildNodeLogic falls back to traits', () => { + const sql = (atomic as any).buildNodeLogic(performedNode([labelFilter])); + + expect(sql).toContain(expected()); + expect(sql).not.toContain(`JSONExtractString(properties, 'labelName')`); + }); + + it('BatchProcessorService.buildSegmentLogic falls back to traits, aliased', () => { + const sql = (batch as any).buildSegmentLogic({ + entryNode: { type: 'And' }, + nodes: [performedNode([labelFilter])], + }); + + expect(sql).toContain(expected('ce')); + expect(sql).not.toContain(`JSONExtractString(ce.properties, 'labelName')`); + }); + + // The batch copy read `prop.key`/`prop.value`, a shape the frontend never + // emits, so its filter resolved to the literal string "undefined". + it('BatchProcessorService no longer reads the wrong property shape', () => { + const sql = (batch as any).buildSegmentLogic({ + entryNode: { type: 'And' }, + nodes: [performedNode([labelFilter])], + }); + + expect(sql).not.toContain('undefined'); + }); + + it.each([ + ['buildNodeLogicForBatch'], + ['buildNodeLogic'], + ])('%s escapes a quote in the path and in the value', (method) => { + const sql = (atomic as any)[method]( + performedNode([ + { path: "label'--", operator: { type: 'Equals', value: "VIP'--" } }, + ]), + ); + + expect(sql).toContain(`label''--`); + expect(sql).toContain(`VIP''--`); + }); + + it('escapes a quote in the event name', () => { + const sql = (atomic as any).buildNodeLogicForBatch({ + id: 'n1', + type: 'Performed', + event: "evt'--", + properties: [], + }); + + expect(sql).toBe(`event_name = 'evt''--'`); + }); +}); diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts index d8a516c..3ed8624 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.spec.ts @@ -207,9 +207,9 @@ describe('SegmentClickHouseQueryBuilderService — whereProperties (CRM-241)', ( }); describe('escaping', () => { - // The escaping moved inside the extraction helper; make sure it did not - // get dropped on the way. The path is interpolated three times now, so a - // regression here would be three injection points instead of one. + // The escaping moved into the shared helper; make sure it was not dropped on + // the way. The path is interpolated twice now (JSONHas + JSONExtractString), + // so a regression here would be two injection points instead of one. it('sanitizes a quote in the property path', () => { const condition = performed([prop("label'--", 'Equals', 'VIP')]); diff --git a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts index c9b93db..7843fb9 100644 --- a/src/modules/segments/services/segment-clickhouse-query-builder.service.ts +++ b/src/modules/segments/services/segment-clickhouse-query-builder.service.ts @@ -45,90 +45,28 @@ export class SegmentClickHouseQueryBuilderService { return SegmentQueryUtils.sanitizeStringValue(String(value ?? '')); } - // Non-finite input becomes the literal `null` instead of raw text, so an - // unquoted numeric comparison can't be used to splice in arbitrary SQL. private escapeNumeric(value: unknown): string { - const num = Number(value); - return Number.isFinite(num) ? String(num) : 'null'; + return SegmentQueryUtils.sanitizeNumericValue(value); } - // LIKE patterns treat %, _ and \ specially; escape them so a user value - // only ever matches itself as a substring. private escapeLike(value: unknown): string { - return this.escapeSql( - String(value ?? '').replace(/[\\%_]/g, (ch) => `\\${ch}`), - ); + return SegmentQueryUtils.sanitizeLikeValue(value); } - // CRM-241: an event property can arrive in either column. Contact events are - // emitted as `identify`, so the whole payload lands in `traits` and - // `properties` stays `{}` (EvoFlow::PayloadBuilder.build_identify never - // builds a `properties` key). Reading only `properties` made every - // whereProperties filter miss, silently and in both directions: Equals - // returned an empty segment, NotEquals let everyone through. - // - // Prefer `properties`, fall back to `traits` only when the key is absent, so - // `track` events — which write `properties` and leave `traits` at `{}` — - // produce the exact same SQL they did before. - // - // JSONHas rather than `!= ''`: a key that is present but empty is a real - // answer from the producer. Falling back on it would swap a deliberate empty - // value for an unrelated one, and would break NotExists. - // - // The `if` picks the COLUMN and extracts once, instead of extracting from both - // and choosing between the results — two JSON operations per row instead of - // three, on a filter that runs over the whole event table. - private extractEventProperty(path: unknown): string { - const escapedPath = this.escapeSql(path); - return ( - `JSONExtractString(if(JSONHas(properties, '${escapedPath}'), ` + - `properties, traits), '${escapedPath}')` - ); - } - - // CRM-241: one event-property filter, for Performed AND LastPerformed. The two - // carried DUPLICATED switches that drifted apart: LastPerformed's listed neither - // the numeric operators nor NotExists, so `GreaterThan` fell through to `default` - // and became EQUALITY — silently, because the SQL stayed valid. A single method - // removes the possibility of them diverging again. + // CRM-241: one event-property filter, for Performed AND LastPerformed, shared + // with the real-time processors that carried their own drifted copies. It picks + // the column at query time because contact events are emitted as `identify`, + // which fills `traits` and leaves `properties` at `{}`. See SegmentQueryUtils. private buildEventPropertyCondition(prop: any): string { - const value = prop?.operator?.value || ''; - const operator = prop?.operator?.type || 'Equals'; - const extract = this.extractEventProperty(prop?.path); - const escapedValue = this.escapeSql(value); - const likeValue = this.escapeLike(value); - const numericValue = this.escapeNumeric(value); - - switch (operator) { - case 'Equals': - return `${extract} = '${escapedValue}'`; - case 'NotEquals': - return `${extract} != '${escapedValue}'`; - case 'Contains': - return `${extract} LIKE '%${likeValue}%'`; - case 'NotContains': - return `${extract} NOT LIKE '%${likeValue}%'`; - case 'GreaterThan': - return `toFloat64OrNull(${extract}) > ${numericValue}`; - case 'GreaterThanOrEqual': - return `toFloat64OrNull(${extract}) >= ${numericValue}`; - case 'LessThan': - return `toFloat64OrNull(${extract}) < ${numericValue}`; - case 'LessThanOrEqual': - return `toFloat64OrNull(${extract}) <= ${numericValue}`; - case 'Exists': - return `${extract} != ''`; - case 'NotExists': - return `${extract} = ''`; - default: - // An unknown operator still degrades to equality, as before — but it now - // says so, instead of degrading in silence. + return SegmentQueryUtils.buildEventPropertyCondition( + prop, + '', + (operator, path) => this.logger.warn( - `Unknown property operator '${operator}' on path '${prop?.path}'; ` + + `Unknown property operator '${operator}' on path '${path}'; ` + `falling back to equality.`, - ); - return `${extract} = '${escapedValue}'`; - } + ), + ); } /** diff --git a/src/modules/segments/utils/segment-query.utils.ts b/src/modules/segments/utils/segment-query.utils.ts index 592c6d7..d6f4586 100644 --- a/src/modules/segments/utils/segment-query.utils.ts +++ b/src/modules/segments/utils/segment-query.utils.ts @@ -125,4 +125,85 @@ export class SegmentQueryUtils { const childDepths = node.children.map((child) => this.getMaxDepth(child)); return 1 + Math.max(...childDepths); } + + /** + * LIKE treats %, _ and \ specially; escape them so a user value only ever + * matches itself as a substring. + */ + static sanitizeLikeValue(value: unknown): string { + return this.sanitizeStringValue( + String(value ?? '').replace(/[\\%_]/g, (ch) => `\\${ch}`), + ); + } + + /** + * Non-finite input becomes the literal `null` instead of raw text, so an + * unquoted numeric comparison can't splice in arbitrary SQL. + */ + static sanitizeNumericValue(value: unknown): string { + const num = Number(value); + return Number.isFinite(num) ? String(num) : 'null'; + } + + /** + * CRM-241: an event property can arrive in either column. Contact events are + * emitted as `identify`, so the payload lands in `traits` and `properties` + * stays `{}`. Prefer `properties` and fall back only when the key is absent, + * so `track` events keep the SQL they had before. JSONHas rather than + * `!= ''`: a present-but-empty key is a real answer, and falling back on it + * would break NotExists. + */ + static extractEventProperty(path: unknown, alias = ''): string { + const prefix = alias ? `${alias}.` : ''; + const key = this.sanitizeStringValue(String(path ?? '')); + return ( + `JSONExtractString(if(JSONHas(${prefix}properties, '${key}'), ` + + `${prefix}properties, ${prefix}traits), '${key}')` + ); + } + + /** + * The single event-property filter every segment builder emits. It lives here + * because the copies in the query builder and in the processing module had + * already drifted apart, each with its own operator list. + */ + static buildEventPropertyCondition( + prop: any, + alias = '', + onUnknownOperator?: (operator: string, path: unknown) => void, + ): string { + const value = prop?.operator?.value || ''; + const operator = prop?.operator?.type || 'Equals'; + const extract = this.extractEventProperty(prop?.path, alias); + const escapedValue = this.sanitizeStringValue(String(value ?? '')); + const likeValue = this.sanitizeLikeValue(value); + const numericValue = this.sanitizeNumericValue(value); + + switch (operator) { + case 'Equals': + return `${extract} = '${escapedValue}'`; + case 'NotEquals': + return `${extract} != '${escapedValue}'`; + case 'Contains': + return `${extract} LIKE '%${likeValue}%'`; + case 'NotContains': + return `${extract} NOT LIKE '%${likeValue}%'`; + case 'GreaterThan': + return `toFloat64OrNull(${extract}) > ${numericValue}`; + case 'GreaterThanOrEqual': + return `toFloat64OrNull(${extract}) >= ${numericValue}`; + case 'LessThan': + return `toFloat64OrNull(${extract}) < ${numericValue}`; + case 'LessThanOrEqual': + return `toFloat64OrNull(${extract}) <= ${numericValue}`; + case 'Exists': + return `${extract} != ''`; + case 'NotExists': + return `${extract} = ''`; + default: + // Unknown operator still degrades to equality, but no longer silently. + onUnknownOperator?.(operator, prop?.path); + return `${extract} = '${escapedValue}'`; + } + } } diff --git a/test/segment-where-properties.e2e-spec.ts b/test/segment-where-properties.e2e-spec.ts index 4924ce1..9d69539 100644 --- a/test/segment-where-properties.e2e-spec.ts +++ b/test/segment-where-properties.e2e-spec.ts @@ -20,16 +20,27 @@ import { * - contact event → arrives as `identify`: payload in `traits`, `properties` = {} * - campaign event → arrives as `track`: payload in `properties`, `traits` = {} * - * Requires a ClickHouse — and FAILS if none is reachable, instead of skipping. - * Runs the same on both environments; point the URL at whichever is up: - * CLICKHOUSE_URL=http://localhost:18123 npm run test:e2e -- segment-where-properties # community - * CLICKHOUSE_URL=http://localhost:18124 npm run test:e2e -- segment-where-properties # ecosystem + * Opt-in, like test/tenant-isolation.e2e-spec.ts, so `npm run test:e2e` does not + * require infrastructure. Once enabled it FAILS if no ClickHouse is reachable, + * instead of skipping. The connection comes from the CLICKHOUSE_* variables the + * app already uses, so a working .env is enough; override the port for another + * environment: + * SEGMENT_E2E=1 npm run test:e2e -- segment-where-properties + * SEGMENT_E2E=1 CLICKHOUSE_PORT=18124 npm run test:e2e -- segment-where-properties * * The scenario lives in a mirror table the test creates and drops, so nothing is * read from or written to the real `contact_events`. */ -const URL = process.env.CLICKHOUSE_URL || 'http://localhost:18123'; -const DB = process.env.CLICKHOUSE_DB || 'evo_campaign'; +const ENABLED = process.env.SEGMENT_E2E === '1'; +const describeMaybe = ENABLED ? describe : describe.skip; + +const URL = + process.env.CLICKHOUSE_URL || + `${process.env.CLICKHOUSE_PROTOCOL || 'http'}://` + + `${process.env.CLICKHOUSE_HOST || 'localhost'}:` + + `${process.env.CLICKHOUSE_PORT || '8123'}`; +const DB = + process.env.CLICKHOUSE_DATABASE || process.env.CLICKHOUSE_DB || 'evo_campaign'; const USER = process.env.CLICKHOUSE_USERNAME || 'default'; const PASS = process.env.CLICKHOUSE_PASSWORD || 'password'; @@ -49,15 +60,17 @@ const PASS = process.env.CLICKHOUSE_PASSWORD || 'password'; // realistically shaped rows, and the copied schema is identical to production's. // // Verified against ClickHouse 26.7 (community, :18123) and 25.8 (ecosystem, :18124). -const TABLE = `crm241_e2e_${Date.now()}_${process.pid}`; +const TABLE_PREFIX = 'crm241_e2e_'; +const TABLE = `${TABLE_PREFIX}${Date.now()}_${process.pid}`; const TAG = 'crm241'; // id prefix for the scenario rows, for readability only const VIP = `${TAG}-vip`; const COMUM = `${TAG}-comum`; const CAMPANHA = `${TAG}-campanha`; -describe('CRM-241 whereProperties matches real rows in ClickHouse (e2e)', () => { +describeMaybe('CRM-241 whereProperties matches real rows in ClickHouse (e2e)', () => { let client: ClickHouseClient; + let mirrorCreated = false; const builder = new SegmentClickHouseQueryBuilderService(); const segment = { id: 'seg-241-e2e' } as Segment; @@ -82,20 +95,32 @@ describe('CRM-241 whereProperties matches real rows in ClickHouse (e2e)', () => try { await client.query({ query: 'SELECT 1', format: 'JSONEachRow' }); } catch (error) { - // Fail, do not skip. A conditional skip here would be worse than having no - // test at all: the suite would go GREEN without exercising anything — the - // same silent-failure mode CRM-241 fixes. (And a flag-driven `it.skip` would - // not even work: Jest evaluates that at collection time, before this hook.) + // Fail, do not skip. The suite is already opt-in, so a green run here has to + // mean the queries really executed — degrading to a skip would reproduce the + // silent-failure mode CRM-241 fixes. throw new Error( `[CRM-241 e2e] ClickHouse unreachable at ${URL} (db=${DB}, user=${USER}): ` + `${(error as Error).message}. Bring the compose up, or point ` + - `CLICKHOUSE_URL/CLICKHOUSE_USERNAME/CLICKHOUSE_PASSWORD at a live one.`, + `CLICKHOUSE_HOST/CLICKHOUSE_PORT at a live one.`, ); } + // Sweep mirrors left behind by an interrupted run. + const stale = await client.query({ + query: + `SELECT name FROM system.tables WHERE database = {db:String} ` + + `AND name LIKE {pattern:String}`, + query_params: { db: DB, pattern: `${TABLE_PREFIX}%` }, + format: 'JSONEachRow', + }); + for (const { name } of await stale.json<{ name: string }>()) { + await client.command({ query: `DROP TABLE IF EXISTS ${DB}.${name}` }); + } + await client.command({ query: `CREATE TABLE ${DB}.${TABLE} AS ${DB}.contact_events`, }); + mirrorCreated = true; const now = new Date().toISOString().replace('T', ' ').substring(0, 23); const row = ( @@ -140,7 +165,12 @@ describe('CRM-241 whereProperties matches real rows in ClickHouse (e2e)', () => }, 120_000); afterAll(async () => { - await client?.command({ query: `DROP TABLE IF EXISTS ${DB}.${TABLE}` }); + // Only clean up what was created: `createClient` is lazy, so on a failed + // connectivity check the client exists and a DROP here would bury the real + // error under a second, contextless one. + if (mirrorCreated) { + await client.command({ query: `DROP TABLE IF EXISTS ${DB}.${TABLE}` }); + } await client?.close(); }); From dbb45d353701b5ebd4d7939932b6320ae3bd4bcc Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sun, 23 Aug 2026 22:22:29 -0300 Subject: [PATCH 23/29] fix(journeys): gatilho de webhook casa so o evento fixo, sem ler config do no (CRM-256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retorno de code review sobre o #116, ja mesclado. O override de nome de evento saiu. Nenhuma UI de nó Webhook escreve esse campo — a WebhookConfiguration so tem URL, metodo e headers —, mas o editor copia data.eventName para conditions e metadata de TODO nó de gatilho (journeyFlowTriggers.ts) e o painel nao limpa o campo ao trocar o tipo (JourneyTriggerPanel.tsx:118). Um nó configurado como Evento e depois trocado para Webhook carregava o nome antigo, e o handler passava a casar esse nome em vez de webhook.journey_trigger: a jornada disparava no evento errado, agora com contato real, e o caminho legitimo deixava de casar. Com o override foram junto o .trim() sobre valor vindo de jsonb — que estoura com eventName nao-string e aborta a analise da mensagem para todas as jornadas, nao so a mal configurada — e o wrapper decide(), que so logava. O doc passa a registrar tambem o nó Aguardar evento -> Webhook: mesmo handler, roteado por eventType e avaliado com journey vazio, config sem nome de evento, logo satisfeito so por webhook.journey_trigger — hoje, por nada. Sem enableFallback essa espera nao tem timeout. Testes: as assercoes das fontes de config dao lugar a duas que provam o buraco fechado — um nó com o nome herdado nos tres lugares nao casa esse nome e continua casando webhook.journey_trigger. --- docs/journey-manual-trigger.md | 4 +- .../services/triggers/webhook.trigger.spec.ts | 96 ++++++------------- .../services/triggers/webhook.trigger.ts | 59 +++--------- 3 files changed, 44 insertions(+), 115 deletions(-) diff --git a/docs/journey-manual-trigger.md b/docs/journey-manual-trigger.md index 1c1d345..a02863c 100644 --- a/docs/journey-manual-trigger.md +++ b/docs/journey-manual-trigger.md @@ -71,4 +71,6 @@ The emitted event uses `eventName: "webhook.journey_trigger"`, In practice the handler matches nothing: this endpoint bypasses trigger matching (see above), so no producer publishes `webhook.journey_trigger` onto the bus. That is expected — the node still works, because the endpoint starts the named journey directly. -Anything that starts publishing `webhook.journey_trigger` onto the bus must address a single journey itself. The matcher is per-journey and has no journey context to compare against (wait conditions are evaluated with an empty journey), so an unaddressed event would start **every** journey holding a Webhook trigger. +The same handler backs the **Wait for event → Webhook** node: the processor routes wait conditions by their `eventType` and evaluates them with an empty journey. That wait config carries no event name either, so it is satisfied by `webhook.journey_trigger` alone — which means, today, by nothing. Such a session leaves the wait only through its fallback, and an `event` wait has one only when `enableFallback` is set — otherwise it waits indefinitely. + +Anything that starts publishing `webhook.journey_trigger` onto the bus must address a single journey itself. The matcher is per-journey and has no journey context to compare against, so an unaddressed event would start **every** journey holding a Webhook trigger. diff --git a/src/modules/journeys/services/triggers/webhook.trigger.spec.ts b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts index 60f84d8..cb176a1 100644 --- a/src/modules/journeys/services/triggers/webhook.trigger.spec.ts +++ b/src/modules/journeys/services/triggers/webhook.trigger.spec.ts @@ -6,15 +6,12 @@ describe('WebhookTrigger', () => { const journey = { id: 'journey-1' }; - const event = ( - eventName: string, - properties: Record = {}, - ): JourneyTriggerEvent => ({ + const event = (eventName: string): JourneyTriggerEvent => ({ messageId: 'm1', contactId: 'c1', eventName, eventType: 'track', - properties: JSON.stringify(properties), + properties: '{}', timestamp: '2026-08-23T00:00:00.000Z', }); @@ -65,58 +62,29 @@ describe('WebhookTrigger', () => { }); }); - describe('target event name resolution', () => { - it('honours an eventName configured in metadata', () => { - const configured = webhookTrigger({ eventName: 'webhook.sendgrid' }); - - expect( - trigger.matches(event('webhook.sendgrid'), configured, journey).matches, - ).toBe(true); + // The editor writes `eventName` onto every trigger node and never clears it + // when the type changes, so a node switched from Event to Webhook carries the + // old name in all three places a config could be read from. + describe('a leftover eventName does not retarget the handler', () => { + const switchedFromEventType = { + type: 'Webhook', + eventName: 'user.signup', + conditions: { eventName: 'user.signup' }, + metadata: { triggerType: 'webhook', eventName: 'user.signup' }, + }; + + it('ignores the leftover name', () => { expect( - trigger.matches(event('webhook.journey_trigger'), configured, journey) + trigger.matches(event('user.signup'), switchedFromEventType, journey) .matches, ).toBe(false); }); - it('honours an eventName set directly on the node', () => { - const node = { type: 'Webhook', eventName: 'webhook.custom' }; - - expect( - trigger.matches(event('webhook.custom'), node, journey).matches, - ).toBe(true); - }); - - // EventTrigger reads this path too; a handler that ignored it would leave the - // node silently never firing. - it('honours an eventName under conditions, like EventTrigger does', () => { - const node = { - type: 'Webhook', - conditions: { eventName: 'webhook.custom' }, - }; - - expect( - trigger.matches(event('webhook.custom'), node, journey).matches, - ).toBe(true); - }); - - it.each([ - ['blank', ' '], - ['empty', ''], - ])('treats a %s configured eventName as unset', (_label, eventName) => { - const result = trigger.matches( - event('webhook.journey_trigger'), - webhookTrigger({ eventName }), - journey, - ); - - expect(result.matches).toBe(true); - }); - - it('trims a configured eventName', () => { + it('still matches the journey trigger event', () => { expect( trigger.matches( - event('webhook.custom'), - webhookTrigger({ eventName: ' webhook.custom ' }), + event('webhook.journey_trigger'), + switchedFromEventType, journey, ).matches, ).toBe(true); @@ -127,32 +95,22 @@ describe('WebhookTrigger', () => { // journey-trigger-processor.service.ts calls handlers with `{}` as the journey // when it evaluates wait conditions; matching must not depend on journey.id. it('matches with the empty journey the wait-condition call site passes', () => { - const waitConditions = { - eventType: 'webhook', - eventName: 'webhook.journey_trigger', - }; + const waitConditions = { eventType: 'webhook', webhookUrl: 'https://x' }; expect( - trigger.matches( - // The manual-trigger emitter always stamps journeyId into properties. - event('webhook.journey_trigger', { journeyId: 'journey-9' }), - waitConditions, - {}, - ).matches, + trigger.matches(event('webhook.journey_trigger'), waitConditions, {}) + .matches, ).toBe(true); }); it.each([ ['null', null], ['undefined', undefined], - ])( - 'falls back to the default event name for a %s trigger', - (_label, node) => { - expect( - trigger.matches(event('webhook.journey_trigger'), node, journey) - .matches, - ).toBe(true); - }, - ); + ])('matches with a %s trigger', (_label, node) => { + expect( + trigger.matches(event('webhook.journey_trigger'), node, journey) + .matches, + ).toBe(true); + }); }); }); diff --git a/src/modules/journeys/services/triggers/webhook.trigger.ts b/src/modules/journeys/services/triggers/webhook.trigger.ts index f46fb5d..56885be 100644 --- a/src/modules/journeys/services/triggers/webhook.trigger.ts +++ b/src/modules/journeys/services/triggers/webhook.trigger.ts @@ -14,58 +14,27 @@ export class WebhookTrigger extends BaseTrigger { super('Webhook'); } + // The target name is fixed, not read off the node: the editor copies + // `eventName` onto every trigger node whatever its type, so honouring it here + // would let a name left behind by the Event type retarget this handler. matches( event: JourneyTriggerEvent, trigger: unknown, journey: unknown, ): TriggerMatchResult { - const targetEventName = this.resolveTargetEventName(trigger); - - if (event.eventName !== targetEventName) { - return this.decide(event, journey, { - matches: false, - reason: `Event name mismatch: ${event.eventName} !== ${targetEventName}`, - metadata: { eventName: event.eventName, targetEventName }, - }); - } - - return this.decide(event, journey, { - matches: true, - reason: `Event name matches: ${targetEventName}`, - metadata: { eventName: event.eventName, targetEventName }, - }); - } - - // Same resolution order EventTrigger uses, so both handlers read an identical - // node shape identically. A blank configured name counts as unset. - private resolveTargetEventName(trigger: unknown): string { - const config = this.getTriggerConfig(trigger ?? {}) as { - eventName?: string; - }; - const node = (trigger ?? {}) as { - eventName?: string; - conditions?: { eventName?: string }; + const matches = event.eventName === JOURNEY_WEBHOOK_EVENT_NAME; + + const result: TriggerMatchResult = { + matches, + reason: matches + ? `Event name matches: ${JOURNEY_WEBHOOK_EVENT_NAME}` + : `Event name mismatch: ${event.eventName} !== ${JOURNEY_WEBHOOK_EVENT_NAME}`, + metadata: { + eventName: event.eventName, + targetEventName: JOURNEY_WEBHOOK_EVENT_NAME, + }, }; - for (const candidate of [ - config.eventName, - node.eventName, - node.conditions?.eventName, - ]) { - const name = candidate?.trim(); - if (name) { - return name; - } - } - - return JOURNEY_WEBHOOK_EVENT_NAME; - } - - private decide( - event: JourneyTriggerEvent, - journey: unknown, - result: TriggerMatchResult, - ): TriggerMatchResult { this.logMatch(event, journey, result); return result; } From 2da151de9fbd29d0acdcc976816b503fd2b8219d Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Sun, 23 Aug 2026 22:44:02 -0300 Subject: [PATCH 24/29] fix(journeys): torna o descarte de evento sem contato visivel no log (CRM-271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retorno de code review sobre o PR #117. O descarte por evento era logado em debug, e CustomLoggerService.debug retorna antes do console e antes do winston — a linha nao saia em lugar nenhum. O criterio de aceite pede descarte visivel, e o que restava era o total corrido a cada mil, que nao nomeia evento algum: os 999 primeiros ficavam silenciosos. O teste que cobria isso afirmava contra o mock de logger.debug, entao seguia verde com a producao muda. O descarte passa a log/info. O volume em info nao muda: processMessage ja emite duas linhas por mensagem (uma delas com o evento inteiro serializado) e analyzeEventForJourneyTriggers uma terceira. Essa terceira anunciava analise para evento que seria descartado — a guarda subiu para antes dela, entao o evento descartado gasta uma linha a menos, nao uma a mais. Quando o que falta e o nome, a mensagem virava "Skipping event undefined"; passa a , e o contexto carrega messageId e anonymousId, que para as linhas de entregabilidade e a unica alca de volta ao evento de origem. O total corrido so imprimia em multiplos exatos do intervalo, entao ate 999 descartes se perdiam a cada restart; onModuleDestroy passa a liberar o saldo. --- .../journey-trigger-processor.service.spec.ts | 49 +++++++++++++++++-- .../journey-trigger-processor.service.ts | 36 +++++++++----- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts index a029d06..94e2607 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.spec.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.spec.ts @@ -423,16 +423,38 @@ describe('JourneyTriggerProcessor contact-less events', () => { expectSkipped(); }); - it('logs the skip at debug — this is routine traffic, not an anomaly', async () => { + // CustomLoggerService.debug is a no-op, so a skip logged there reaches no + // console and no file: it has to go out at a level that actually prints. + it('logs the skip where it is actually visible, not at warn', async () => { await analyze({ contactId: '' }); - expect((processor as any).logger.debug).toHaveBeenCalledWith( - expect.stringContaining('no contactId'), - expect.objectContaining({ eventName: 'webhook.sendgrid' }), + expect((processor as any).logger.log).toHaveBeenCalledWith( + expect.stringContaining('webhook.sendgrid — no contactId'), + expect.objectContaining({ messageId: 'm-1' }), ); + expect((processor as any).logger.debug).not.toHaveBeenCalled(); expect((processor as any).logger.warn).not.toHaveBeenCalled(); }); + it('names a nameless event instead of printing undefined', async () => { + await analyze({ eventName: '' }); + + expect((processor as any).logger.log).toHaveBeenCalledWith( + expect.stringContaining(' — no eventName'), + expect.anything(), + ); + }); + + // The guard sits ahead of it, so a skipped event must not announce an + // analysis that never runs. + it('skips before announcing the analysis', async () => { + await analyze({ contactId: '' }); + + expect((processor as any).logger.log).not.toHaveBeenCalledWith( + expect.stringContaining('Analyzing event for journey triggers'), + ); + }); + it('reports a running total so the volume stays visible', async () => { for (let i = 0; i < 1000; i++) { await analyze({ contactId: '' }); @@ -443,6 +465,25 @@ describe('JourneyTriggerProcessor contact-less events', () => { ); }); + // Below the interval the total never printed, so a restart used to drop it. + it('flushes the pending total on shutdown', async () => { + await analyze({ contactId: '' }); + + await (processor as any).onModuleDestroy(); + + expect((processor as any).logger.log).toHaveBeenCalledWith( + expect.stringContaining('1 events skipped so far'), + ); + }); + + it('stays quiet on shutdown when nothing was skipped', async () => { + await (processor as any).onModuleDestroy(); + + expect((processor as any).logger.log).not.toHaveBeenCalledWith( + expect.stringContaining('events skipped so far'), + ); + }); + it('still dispatches for an event that carries a contact', async () => { await analyze(); diff --git a/src/modules/journeys/services/journey-trigger-processor.service.ts b/src/modules/journeys/services/journey-trigger-processor.service.ts index 1b1314d..8da03d2 100644 --- a/src/modules/journeys/services/journey-trigger-processor.service.ts +++ b/src/modules/journeys/services/journey-trigger-processor.service.ts @@ -29,6 +29,7 @@ import { } from './triggers'; const SKIPPED_EVENT_LOG_INTERVAL = 1000; +const UNNAMED_EVENT = ''; export interface JourneyTriggerEvent { messageId: string; @@ -162,6 +163,12 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { } async onModuleDestroy() { + // The running total only prints every SKIPPED_EVENT_LOG_INTERVAL, so the + // remainder would be lost on every restart without this flush. + if (this.skippedUndispatchableEvents > 0) { + this.logSkippedTotal(); + } + if (this.consumer) { this.logger.log('🔄 Stopping Journey Trigger Processor...'); await this.consumer.disconnect(); @@ -280,14 +287,14 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { private async analyzeEventForJourneyTriggers(event: JourneyTriggerEvent) { try { - this.logger.log( - `🔍 Analyzing event for journey triggers: ${event.eventName}`, - ); - if (!this.isDispatchable(event)) { return; } + this.logger.log( + `🔍 Analyzing event for journey triggers: ${event.eventName}`, + ); + // 1. First, check if event satisfies any waiting sessions await this.checkWaitingSessions(event); @@ -330,8 +337,8 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { * a full session-cache scan (getSessionsByContact reads every session, then * filters) and can open a session under an empty contact id that every later * contact-less event then finds. Deliverability callbacks and anonymous link - * clicks arrive without a contact and are the bulk of this bus, so the skip is - * routine: debug per event, info only for the running total. + * clicks arrive without a contact and are the bulk of this bus, so the skip + * also reports a running total. */ private isDispatchable(event: JourneyTriggerEvent): boolean { const missing = !event.contactId?.trim() @@ -346,24 +353,29 @@ export class JourneyTriggerProcessor implements OnModuleInit, OnModuleDestroy { this.skippedUndispatchableEvents += 1; - this.logger.debug( - `⏭️ Skipping event ${event.eventName} — no ${missing}, nothing contact-scoped can run`, + // Deliverability rows carry no messageId either, so anonymousId (the + // ingestion id) is often the only handle back to the source event. + this.logger.log( + `⏭️ Skipping event ${event.eventName?.trim() || UNNAMED_EVENT} — no ${missing}, nothing contact-scoped can run`, { messageId: event.messageId, - eventName: event.eventName, anonymousId: event.anonymousId, }, ); if (this.skippedUndispatchableEvents % SKIPPED_EVENT_LOG_INTERVAL === 0) { - this.logger.log( - `⏭️ ${this.skippedUndispatchableEvents} events skipped so far — no contactId or no eventName`, - ); + this.logSkippedTotal(); } return false; } + private logSkippedTotal(): void { + this.logger.log( + `⏭️ ${this.skippedUndispatchableEvents} events skipped so far — no contactId or no eventName`, + ); + } + private async matchesJourneyTrigger( event: JourneyTriggerEvent, journey: any, From 5a0a63c91d89bcb740e5a19aa35b6d196090927f Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Mon, 24 Aug 2026 10:05:28 -0300 Subject: [PATCH 25/29] chore(journeys): remove processWebhookTrigger sem callers (CRM-257) O metodo montava um evento webhook.received completo e nunca o publicava, e sanitizeHeaders so era usado por ele. Quem lia esse trecho procurando o caminho do webhook concluia que a ingestao existia. --- src/modules/journeys/journeys.service.ts | 66 ------------------------ 1 file changed, 66 deletions(-) diff --git a/src/modules/journeys/journeys.service.ts b/src/modules/journeys/journeys.service.ts index c3eeddd..edef893 100644 --- a/src/modules/journeys/journeys.service.ts +++ b/src/modules/journeys/journeys.service.ts @@ -286,72 +286,6 @@ export class JourneysService { return true; } - async processWebhookTrigger( - payload: any, - headers: any, - ): Promise<{ success: boolean; messageId: string; processedAt: Date }> { - const messageId = uuidv4(); - const processedAt = new Date(); - - this.logger.debug('Processing webhook trigger', { - messageId, - payloadKeys: Object.keys(payload || {}), - headersCount: Object.keys(headers || {}).length, - }); - - try { - const webhookEvent = { - messageId, - eventType: 'webhook', - eventName: 'webhook.received', - contactId: payload.contactId || null, - properties: { - endpoint: '/api/v1/journeys/trigger', - data: payload, - headers: this.sanitizeHeaders(headers), - method: 'POST', - timestamp: processedAt.toISOString(), - }, - timestamp: processedAt, - }; - - this.logger.log('Webhook trigger event created successfully', { - messageId, - contactId: webhookEvent.contactId, - }); - - return { - success: true, - messageId, - processedAt, - }; - } catch (error) { - this.logger.error('Failed to process webhook trigger', { - messageId, - error: error.message, - stack: error.stack, - }); - - throw new BadRequestException('Failed to process webhook trigger'); - } - } - - private sanitizeHeaders(headers: any): Record { - if (!headers || typeof headers !== 'object') return {}; - - const sensitiveHeaders = ['authorization', 'cookie', 'x-api-key']; - const sanitized: Record = {}; - - Object.keys(headers).forEach((key) => { - const normalizedKey = key.toLowerCase(); - if (!sensitiveHeaders.includes(normalizedKey)) { - sanitized[key] = String(headers[key]); - } - }); - - return sanitized; - } - async processSpecificJourneyWebhookTrigger( journeyId: string, payload: any, From abf80ad7274788d04193d9d1da576081059b4050 Mon Sep 17 00:00:00 2001 From: Danilo Leone Date: Mon, 24 Aug 2026 10:24:32 -0300 Subject: [PATCH 26/29] docs(journeys): aponta o caminho de webhook que realmente roda (CRM-257) A remocao do metodo morto tira a pista falsa e nao deixa nada no lugar. O README passa a dizer qual e o unico gatilho de webhook do modulo e que o POST /webhooks/* e outro pipeline. --- src/modules/journeys/README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/modules/journeys/README.md b/src/modules/journeys/README.md index 5a82dfd..e3b6303 100644 --- a/src/modules/journeys/README.md +++ b/src/modules/journeys/README.md @@ -60,3 +60,11 @@ access but no Redis access. Regression guards for these guarantees live in `src/modules/cache/services/journey-session-cache.service.spec.ts` (cross-instance sharing, DB-seeding fallback, `getMultiple` In() clause). + +## Webhook entry points: which one actually runs + +There is exactly one webhook path into a journey from this service: `POST /journeys/trigger/:journeyId` → `JourneysService.processSpecificJourneyWebhookTrigger`. It requires `contact_id` in the payload, targets the named journey directly, and publishes a `webhook.journey_trigger` event. + +`POST /webhooks/*` (the `event-receiver` / `event-process` runners) is a different pipeline and does **not** start journeys by itself: it is the e-mail deliverability path — detect platform, validate signature, enrich, write to ClickHouse `contact_events`. It does not create contacts and does not talk to the CRM. + +This note exists because the module used to carry a `processWebhookTrigger` method that built a full `webhook.received` event and never published it anywhere. It was removed; reading it as "webhook ingestion works" cost real analysis time more than once. From b7ca55ef0fa2a22d719ecb391b283c22ceb2c99d Mon Sep 17 00:00:00 2001 From: daniloleonecarneiro <36852522+daniloleonecarneiro@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:44:27 -0300 Subject: [PATCH 27/29] Update src/modules/journeys/README.md Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> --- src/modules/journeys/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/journeys/README.md b/src/modules/journeys/README.md index e3b6303..0939d46 100644 --- a/src/modules/journeys/README.md +++ b/src/modules/journeys/README.md @@ -63,7 +63,7 @@ Regression guards for these guarantees live in ## Webhook entry points: which one actually runs -There is exactly one webhook path into a journey from this service: `POST /journeys/trigger/:journeyId` → `JourneysService.processSpecificJourneyWebhookTrigger`. It requires `contact_id` in the payload, targets the named journey directly, and publishes a `webhook.journey_trigger` event. +There is exactly one webhook path into a journey from this service: `POST /api/v1/journeys/trigger/:journeyId` → `JourneysService.processSpecificJourneyWebhookTrigger`. It requires `contact_id` in the payload, targets the named journey directly, and publishes a `webhook.journey_trigger` event. `POST /webhooks/*` (the `event-receiver` / `event-process` runners) is a different pipeline and does **not** start journeys by itself: it is the e-mail deliverability path — detect platform, validate signature, enrich, write to ClickHouse `contact_events`. It does not create contacts and does not talk to the CRM. From 64a649d83962d3274fdb13ea0844f631d2b0fbb1 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Mon, 24 Aug 2026 13:48:07 -0300 Subject: [PATCH 28/29] docs(journeys): corrige o verbo do gatilho manual e aponta o contrato (CRM-257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit O endpoint nao publica o webhook.journey_trigger em lugar nenhum: monta o evento e passa para startJourney como payload do workflow, sem tocar o barramento journey-triggers. Dizer "publishes" contradiz o docs/journey-manual-trigger.md, que ja documenta o caminho certo — a secao agora aponta para ele em vez de reescreve-lo. O trecho do POST /webhooks/* tambem afirmava isolamento que nao existe: a MV events_to_journey_triggers_mv encaminha toda linha de contact_events para o barramento, e o que segura os callbacks de provider sao os dois guards (contact_id vazio e match por nome exato), nao o pipeline ser outro. --- src/modules/journeys/README.md | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/modules/journeys/README.md b/src/modules/journeys/README.md index 0939d46..3fb01a7 100644 --- a/src/modules/journeys/README.md +++ b/src/modules/journeys/README.md @@ -63,8 +63,32 @@ Regression guards for these guarantees live in ## Webhook entry points: which one actually runs -There is exactly one webhook path into a journey from this service: `POST /api/v1/journeys/trigger/:journeyId` → `JourneysService.processSpecificJourneyWebhookTrigger`. It requires `contact_id` in the payload, targets the named journey directly, and publishes a `webhook.journey_trigger` event. - -`POST /webhooks/*` (the `event-receiver` / `event-process` runners) is a different pipeline and does **not** start journeys by itself: it is the e-mail deliverability path — detect platform, validate signature, enrich, write to ClickHouse `contact_events`. It does not create contacts and does not talk to the CRM. - -This note exists because the module used to carry a `processWebhookTrigger` method that built a full `webhook.received` event and never published it anywhere. It was removed; reading it as "webhook ingestion works" cost real analysis time more than once. +There is exactly one webhook path into a journey from this service: +`POST /api/v1/journeys/trigger/:journeyId` → +`JourneysService.processSpecificJourneyWebhookTrigger`. It requires +`contact_id` in the payload and starts the named journey **directly**: the +`webhook.journey_trigger` event it builds is handed to +`JourneySessionsService.startJourney` as the workflow's trigger payload. It is +never published to the `journey-triggers` bus and never goes through trigger +matching. The full contract — request body, auth, session semantics, and why +the Webhook trigger node consequently matches nothing on the bus — lives in +[`docs/journey-manual-trigger.md`](../../../docs/journey-manual-trigger.md); +keep that file the source of truth rather than restating it here. + +`POST /webhooks/*` (the `event-receiver` / `event-process` runners) is the +e-mail deliverability path: detect platform, validate signature, enrich, write +to ClickHouse `contact_events`. It does not create contacts, does not talk to +the CRM, and does not start journeys — but it is not isolated from them. +`events_to_journey_triggers_mv` forwards **every** `contact_events` row to +`journey-triggers`, so provider callbacks do land on the journey bus as +`webhook.`. Two guards drop them at the far end: the empty +`contact_id` (`JourneyTriggerProcessor.isDispatchable`, CRM-271) and +`WebhookTrigger`'s exact-name match on `webhook.journey_trigger` (CRM-256). +Resolve a real contact for those rows and the name match is the only thing +left standing between deliverability traffic and every journey holding a +Webhook trigger. + +This note exists because the module used to carry a `processWebhookTrigger` +method that built a full `webhook.received` event and never published it +anywhere. It was removed; reading it as "webhook ingestion works" cost real +analysis time more than once. From 2bbf745a188839bf7ef2b7fe9fc23ab00783cf5b Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Fri, 28 Aug 2026 18:55:06 -0300 Subject: [PATCH 29/29] docs(broker): drop the tracker link from the redelivery note The note itself documents runtime behaviour and stays; only the link to the internal issue tracker is removed. --- src/shared/broker/REDELIVERY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/broker/REDELIVERY.md b/src/shared/broker/REDELIVERY.md index 0b3b951..c11afca 100644 --- a/src/shared/broker/REDELIVERY.md +++ b/src/shared/broker/REDELIVERY.md @@ -2,7 +2,7 @@ Defense-in-depth ceiling on redeliveries so a poison message can never block a queue/partition indefinitely — independent of whether the application classified -the error as terminal (that is [EVO-1676](https://linear.app/evoai/issue/EVO-1676)'s +the error as terminal (that is EVO-1676's typed `TerminalError`). Implemented uniformly across both broker adapters via a header-based attempt counter; no RabbitMQ quorum-queue migration required.