diff --git a/packages/workflows/src/dag-executor.test.ts b/packages/workflows/src/dag-executor.test.ts index a0fcb99e29..6762139aa3 100644 --- a/packages/workflows/src/dag-executor.test.ts +++ b/packages/workflows/src/dag-executor.test.ts @@ -4674,6 +4674,76 @@ describe('executeDagWorkflow -- approval node', () => { expect(pauseCalls.length).toBe(1); }); + it('on_reject does not write node_completed for the approval gate node ID', async () => { + mockSendQueryDag.mockImplementation(function* () { + yield { type: 'assistant', content: 'Fixed based on feedback' }; + yield { type: 'result', sessionId: 'reject-no-poison-session' }; + }); + + const store = createMockStore(); + const mockDeps = createMockDeps(store); + const platform = createMockPlatform(); + + const workflowRun = makeWorkflowRun('reject-no-poison-run', { + metadata: { + approval: { + type: 'approval', + nodeId: 'review', + message: 'Approve this plan?', + onRejectPrompt: 'Fix based on: $REJECTION_REASON', + onRejectMaxAttempts: 3, + }, + rejection_reason: 'Missing edge case handling', + rejection_count: 1, + }, + }); + + await executeDagWorkflow( + mockDeps, + platform, + 'conv-approval', + testDir, + { + name: 'approval-no-poison', + nodes: [ + { + id: 'review', + approval: { + message: 'Approve this plan?', + on_reject: { prompt: 'Fix based on: $REJECTION_REASON', max_attempts: 3 }, + }, + }, + ], + }, + workflowRun, + 'claude', + undefined, + join(testDir, 'artifacts'), + join(testDir, 'logs'), + 'main', + 'docs/', + minimalConfig + ); + + // The on_reject synthetic node must NOT produce a node_completed event with + // step_name equal to the approval gate's own ID ('review'). If it did, a + // subsequent resume would find the event via getCompletedDagNodeOutputs and + // skip the approval gate entirely, bypassing the human gate. + const eventCalls = (store.createWorkflowEvent as ReturnType).mock.calls; + const nodeCompletedEvents = eventCalls.filter( + (call: unknown[]) => (call[0] as Record).event_type === 'node_completed' + ); + const completedStepNames = nodeCompletedEvents.map( + (call: unknown[]) => (call[0] as Record).step_name + ); + expect(completedStepNames).not.toContain('review'); + + // The synthetic on_reject node MUST produce a node_completed event with the + // distinct ID 'review:on_reject'. This ensures the synthetic node itself is + // recorded as completed so it is not re-run on a subsequent resume. + expect(completedStepNames.filter((n: unknown) => n === 'review:on_reject').length).toBe(1); + }); + it('on_reject cancels when max_attempts exhausted', async () => { const store = createMockStore(); const mockDeps = createMockDeps(store); diff --git a/packages/workflows/src/dag-executor.ts b/packages/workflows/src/dag-executor.ts index 419a9066f6..090049867f 100644 --- a/packages/workflows/src/dag-executor.ts +++ b/packages/workflows/src/dag-executor.ts @@ -2249,9 +2249,21 @@ async function executeApprovalNode( rejectionReason ); - // Build a synthetic PromptNode to reuse executeNodeInternal + // Build a synthetic PromptNode to reuse executeNodeInternal. + // Use a distinct ID so the node_completed event written by executeNodeInternal + // does not collide with the approval gate's own ID in getCompletedDagNodeOutputs. + // If we used node.id here, a resumed run would find the event and treat the + // approval gate as already completed, bypassing the human gate entirely. + // + // Note: executeNodeInternal also emits node_started/node_completed WorkflowEmitterEvents + // with nodeId = `${node.id}:on_reject`. These flow through SSE into the web UI, where + // WorkflowExecution.tsx builds its nodeMap from all node_* events unconditionally. + // This means a transient `${node.id}:on_reject` phantom entry may appear in the UI's + // execution view during an on_reject cycle. This is cosmetic-only — the approval gate + // still re-presents correctly and the human gate contract is preserved. A follow-up can + // filter synthetic `:on_reject` IDs from the UI's nodeMap if needed. const syntheticNode: PromptNode = { - id: node.id, + id: `${node.id}:on_reject`, prompt: substituteNodeOutputRefs(substitutedPrompt, nodeOutputs), ...(node.depends_on ? { depends_on: node.depends_on } : {}), ...(node.idle_timeout ? { idle_timeout: node.idle_timeout } : {}),