Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions packages/workflows/src/dag-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>) => Promise<void>>
).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<void>>
).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<void>>
).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;
Expand Down
13 changes: 8 additions & 5 deletions packages/workflows/src/dag-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Consider: loop.gate_message has the same bug pattern and is not addressed here.

The PR description states approval was "the only missing case," but executeLoopNode still emits loop.gate_message raw at three sites (lines 2128, 2135, 2144) without running it through substituteNodeOutputRefs. If a workflow author writes a gate_message referencing $nodeId.output.field, the placeholder will leak to the user — same UX defect this PR fixes for approval gates.

Out of scope for this PR, but worth a follow-up so the two HITL gates behave consistently. Happy to open a tracking issue if useful.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/workflows/src/dag-executor.ts` at line 2300, The executeLoopNode
implementation emits loop.gate_message raw in three places without substituting
node output refs; update executeLoopNode to pass loop.gate_message (from the
loop object) through substituteNodeOutputRefs using the same nodeOutputs context
before emitting/returning it so placeholders like $nodeId.output.field are
resolved; ensure you reference the loop variable's gate_message at each emission
site and reuse substituteNodeOutputRefs(node.gate_message, nodeOutputs)
consistently to mirror the fix applied for approval.message.

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);
Expand All @@ -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(
Expand All @@ -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,
Expand All @@ -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.
Expand Down
Loading