diff --git a/packages/workflows/src/dag-executor.test.ts b/packages/workflows/src/dag-executor.test.ts index a0fcb99e29..f3c1a475ef 100644 --- a/packages/workflows/src/dag-executor.test.ts +++ b/packages/workflows/src/dag-executor.test.ts @@ -4787,6 +4787,112 @@ describe('executeDagWorkflow -- approval node', () => { 1 ); }); + + it('approval message substitutes $nodeId.output.field references from upstream structured output', async () => { + // Repro for: approval gates were rendering literal "$gather-context.output.repo_name" + // instead of resolved values, breaking interactive workflows like atlas-onboard. + // Parity: prompt/bash/loop/cancel nodes already get substituteNodeOutputRefs; + // approval.message must too so the human sees concrete values. + const structuredJson = { + repo_name: 'hcr-els', + app_code: 'CCELS', + frontend_port: 3012, + }; + + const commandsDir = join(testDir, '.archon', 'commands'); + await mkdir(commandsDir, { recursive: true }); + await writeFile(join(commandsDir, 'gather-context.md'), 'Gather context: $USER_MESSAGE'); + + mockSendQueryDag.mockImplementation(function* () { + yield { type: 'assistant', content: JSON.stringify(structuredJson) }; + yield { type: 'result', sessionId: 'sid-approval-sub', structuredOutput: structuredJson }; + }); + + const store = createMockStore(); + const mockDeps = createMockDeps(store); + const platform = createMockPlatform(); + const workflowRun = makeWorkflowRun('approval-sub-run'); + + await executeDagWorkflow( + mockDeps, + platform, + 'conv-approval-sub', + testDir, + { + name: 'approval-sub-test', + nodes: [ + { + id: 'gather-context', + command: 'gather-context', + output_format: { + type: 'object', + properties: { + repo_name: { type: 'string' }, + app_code: { type: 'string' }, + frontend_port: { type: 'number' }, + }, + }, + }, + { + id: 'confirm', + depends_on: ['gather-context'], + approval: { + message: + 'Repo: $gather-context.output.repo_name | App: $gather-context.output.app_code | Port: $gather-context.output.frontend_port', + }, + }, + ], + }, + workflowRun, + 'claude', + undefined, + join(testDir, 'artifacts'), + join(testDir, 'logs'), + 'main', + 'docs/', + minimalConfig + ); + + // gather-context AI call ran once; approval node does NOT call AI + expect(mockSendQueryDag.mock.calls.length).toBe(1); + + // pauseWorkflowRun should receive the SUBSTITUTED message, not the literal placeholders + const pauseCalls = ( + store.pauseWorkflowRun as Mock<(id: string, ctx: Record) => Promise> + ).mock.calls; + expect(pauseCalls.length).toBe(1); + expect(pauseCalls[0][1]).toMatchObject({ + type: 'approval', + nodeId: 'confirm', + message: 'Repo: hcr-els | App: CCELS | Port: 3012', + }); + + // The fix touches FOUR emission sites (safeSendMessage / createWorkflowEvent / + // pauseWorkflowRun / event-emitter). Assert the other two reachable surfaces too — + // a future regression at any one of them would otherwise pass this test silently. + // (Per CodeRabbit review of PR coleam00/Archon#1426.) + + // (a) The chat-surface prompt emitted via platform.sendMessage must contain the + // substituted message and must NOT contain literal $gather-context.output refs. + const sentMessages = ( + platform.sendMessage as Mock<(...args: unknown[]) => Promise> + ).mock.calls.map((c: unknown[]) => c[1] as string); + expect(sentMessages.some(m => m.includes('Repo: hcr-els | App: CCELS | Port: 3012'))).toBe( + true + ); + expect(sentMessages.some(m => m.includes('$gather-context.output'))).toBe(false); + + // (b) The persisted approval_requested workflow event's data.message must be substituted. + const approvalRequestedEvents = ( + store.createWorkflowEvent as Mock<() => Promise> + ).mock.calls.filter( + (c: unknown[]) => (c[0] as { event_type: string }).event_type === 'approval_requested' + ); + expect(approvalRequestedEvents.length).toBe(1); + expect((approvalRequestedEvents[0][0] as { data: { message: string } }).data.message).toBe( + 'Repo: hcr-els | App: CCELS | Port: 3012' + ); + }); }); describe('executeDagWorkflow -- env var injection', () => { let testDir: string; diff --git a/packages/workflows/src/dag-executor.ts b/packages/workflows/src/dag-executor.ts index 419a9066f6..8fef337fea 100644 --- a/packages/workflows/src/dag-executor.ts +++ b/packages/workflows/src/dag-executor.ts @@ -2294,9 +2294,12 @@ async function executeApprovalNode( // Fall through to re-pause at the approval gate } - // Standard approval gate — send message and pause + // Standard approval gate — send message and pause. + // Resolve $nodeId.output[.field] references so the human sees concrete values + // (parity with prompt/bash/loop/cancel nodes, which all run the same substitution). + const renderedMessage = substituteNodeOutputRefs(node.approval.message, nodeOutputs); const approvalMsg = - `⏸ **Approval required**: ${node.approval.message}\n\n` + + `⏸ **Approval required**: ${renderedMessage}\n\n` + `Run ID: \`${workflowRun.id}\`\n` + `Approve: \`/workflow approve ${workflowRun.id}\` | Reject: \`/workflow reject ${workflowRun.id}\``; await safeSendMessage(platform, conversationId, approvalMsg, msgContext); @@ -2306,7 +2309,7 @@ async function executeApprovalNode( workflow_run_id: workflowRun.id, event_type: 'approval_requested', step_name: node.id, - data: { message: node.approval.message }, + data: { message: renderedMessage }, }) .catch((err: Error) => { getLog().error( @@ -2316,7 +2319,7 @@ async function executeApprovalNode( }); await deps.store.pauseWorkflowRun(workflowRun.id, { - message: node.approval.message, + message: renderedMessage, nodeId: node.id, type: 'approval', captureResponse: node.approval.capture_response, @@ -2328,7 +2331,7 @@ async function executeApprovalNode( type: 'approval_pending', runId: workflowRun.id, nodeId: node.id, - message: node.approval.message, + message: renderedMessage, }); // Return completed — the between-layer status check will see 'paused' and break.