mirror of
https://github.com/coleam00/Archon
synced 2026-04-21 13:37:41 +00:00
fix(api): surface errors on unresolvable parent conv, add interactive_loop approve tests
- Return 500 (not silent success) when the parent conversation cannot be resolved during interactive_loop approval, per Fail Fast principle - Add .catch() to void dispatchToOrchestrator call to log dispatch errors - Add length guard to positional-index test assertion in orchestrator test - Add tests for the interactive_loop approve branch (status stays paused, loop_user_input stored, dispatch to parent conv, null-parent fallback, unresolvable conv returns 500) and assert updateWorkflowRun for standard approval path - Update docs: auto-resume on Web UI, stale "send a follow-up message" language removed; add Design Notes clarifying interactive loop path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
e09b5b2941
commit
2c0002fc96
5 changed files with 156 additions and 7 deletions
|
|
@ -1102,7 +1102,8 @@ describe('workflow dispatch routing — interactive flag', () => {
|
|||
|
||||
// Verify parentConversationId is passed so resume-after-approval works
|
||||
const callArgs = mockExecuteWorkflow.mock.calls[0] as unknown[];
|
||||
// param 11 (index 10) is parentConversationId — should be conversation.id
|
||||
// executeWorkflow is called with 11 positional args; index 10 is parentConversationId
|
||||
expect(callArgs).toHaveLength(11);
|
||||
expect(callArgs[10]).toBe('conv-1');
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -55,9 +55,10 @@ to the user on whatever platform they're using (CLI, Slack, GitHub, etc.). On th
|
|||
block the worktree path guard (no other workflow can start on the same path).
|
||||
4. **Approve**: The user approves, which writes a `node_completed` event for
|
||||
the approval node and transitions the run to resumable. Natural-language
|
||||
messages (recommended) and the CLI auto-resume immediately. The explicit
|
||||
`/workflow approve` command records the approval; send a follow-up message
|
||||
to resume.
|
||||
messages (recommended), the CLI, and the Web UI all auto-resume immediately.
|
||||
The explicit `/workflow approve` slash command records the approval and also
|
||||
auto-resumes on the Web UI; on other platforms it requires a follow-up
|
||||
message to trigger resume.
|
||||
5. **Reject**: The user rejects.
|
||||
- **Without `on_reject`**: The workflow is cancelled immediately.
|
||||
- **With `on_reject`**: The executor runs the `on_reject.prompt` via AI (with
|
||||
|
|
@ -227,3 +228,7 @@ PR #871). When approved, the run transitions through `failed` status briefly so
|
|||
that `findResumableRun` picks it up — this avoids duplicating resume logic. The
|
||||
`metadata.approval_response` field distinguishes approved-then-resumed from
|
||||
genuinely-failed runs.
|
||||
|
||||
Interactive loop gates follow a different path: the run stays `paused`, the
|
||||
approve endpoint auto-dispatches to the orchestrator, and the natural-language
|
||||
resume path (`getPausedWorkflowRun`) handles the transition.
|
||||
|
|
|
|||
|
|
@ -978,7 +978,7 @@ When the workflow reaches `review-gate`, it pauses and notifies you. Approve or
|
|||
|
||||
- **Natural language** (recommended): Just type your response in the conversation — the system detects the paused workflow and auto-resumes
|
||||
- **CLI**: `bun run cli workflow approve <run-id>` or `bun run cli workflow reject <run-id>`
|
||||
- **Explicit command**: `/workflow approve <run-id>` or `/workflow reject <run-id>` (records approval; send a follow-up message to resume)
|
||||
- **Explicit command**: `/workflow approve <run-id>` or `/workflow reject <run-id>` (auto-resumes on Web UI; on other platforms, send a follow-up message after approving)
|
||||
- **Web UI**: Click the Approve/Reject buttons on the dashboard card
|
||||
- **API**: `POST /api/workflows/runs/<run-id>/approve` or `/reject`
|
||||
|
||||
|
|
|
|||
|
|
@ -1902,9 +1902,22 @@ export function registerApiRoutes(
|
|||
// dispatches the resumed workflow.
|
||||
const parentConvDbId = run.parent_conversation_id ?? run.conversation_id;
|
||||
const parentConv = await conversationDb.getConversationById(parentConvDbId);
|
||||
if (parentConv?.platform_conversation_id) {
|
||||
void dispatchToOrchestrator(parentConv.platform_conversation_id, comment);
|
||||
if (!parentConv?.platform_conversation_id) {
|
||||
// Can't auto-dispatch — surface the failure so the user can resume manually.
|
||||
getLog().error(
|
||||
{ runId, parentConvDbId, workflowName: run.workflow_name },
|
||||
'api.workflow_run_approve_interactive_loop_no_parent_conv'
|
||||
);
|
||||
return apiError(
|
||||
c,
|
||||
500,
|
||||
'Workflow approved but could not auto-resume: parent conversation not found. ' +
|
||||
'Send a message to continue the workflow.'
|
||||
);
|
||||
}
|
||||
void dispatchToOrchestrator(parentConv.platform_conversation_id, comment).catch(err => {
|
||||
getLog().error({ err, runId }, 'api.workflow_run_approve_interactive_loop_dispatch_failed');
|
||||
});
|
||||
return c.json({
|
||||
success: true,
|
||||
message: `Workflow approved and resuming: ${run.workflow_name}.`,
|
||||
|
|
|
|||
|
|
@ -1251,6 +1251,136 @@ describe('POST /api/workflows/runs/:runId/approve', () => {
|
|||
data: { node_output: '', approval_decision: 'approved' },
|
||||
});
|
||||
});
|
||||
|
||||
test('transitions standard approval run to failed status with cleared rejection metadata', async () => {
|
||||
mockGetWorkflowRun.mockResolvedValueOnce(MOCK_PAUSED_RUN);
|
||||
const { app } = makeApp();
|
||||
await app.request('/api/workflows/runs/run-paused-1/approve', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ comment: 'LGTM' }),
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
});
|
||||
expect(mockUpdateWorkflowRun).toHaveBeenCalledWith('run-paused-1', {
|
||||
status: 'failed',
|
||||
metadata: { approval_response: 'approved', rejection_reason: '', rejection_count: 0 },
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests: POST /api/workflows/runs/:runId/approve — interactive_loop branch
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const MOCK_LOOP_RUN: MockWorkflowRun = {
|
||||
...MOCK_RUNNING_RUN,
|
||||
id: 'run-loop-1',
|
||||
status: 'paused',
|
||||
conversation_id: 'worker-conv-uuid',
|
||||
parent_conversation_id: 'parent-conv-uuid',
|
||||
metadata: {
|
||||
approval: {
|
||||
type: 'interactive_loop',
|
||||
nodeId: 'loop-gate',
|
||||
message: 'Please provide feedback',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
describe('POST /api/workflows/runs/:runId/approve — interactive_loop branch', () => {
|
||||
beforeEach(() => {
|
||||
mockGetWorkflowRun.mockReset();
|
||||
mockUpdateWorkflowRun.mockReset();
|
||||
mockCreateWorkflowEvent.mockReset();
|
||||
mockGetConversationById.mockReset();
|
||||
mockHandleMessage.mockReset();
|
||||
});
|
||||
|
||||
test('keeps run status paused and stores loop_user_input in metadata', async () => {
|
||||
mockGetWorkflowRun.mockResolvedValueOnce(MOCK_LOOP_RUN);
|
||||
mockGetConversationById.mockResolvedValueOnce({
|
||||
id: 'parent-conv-uuid',
|
||||
platform_conversation_id: 'web-parent-abc',
|
||||
});
|
||||
const { app } = makeApp();
|
||||
const response = await app.request('/api/workflows/runs/run-loop-1/approve', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ comment: 'Looks great, continue' }),
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
});
|
||||
expect(response.status).toBe(200);
|
||||
// Must NOT call node_completed — executor writes that on actual loop exit
|
||||
const nodeCompletedCall = mockCreateWorkflowEvent.mock.calls.find(
|
||||
(c: unknown[]) => (c[0] as Record<string, unknown>).event_type === 'node_completed'
|
||||
);
|
||||
expect(nodeCompletedCall).toBeUndefined();
|
||||
// Status must stay paused — not transition to 'failed'
|
||||
expect(mockUpdateWorkflowRun).toHaveBeenCalledWith('run-loop-1', {
|
||||
metadata: { loop_user_input: 'Looks great, continue' },
|
||||
});
|
||||
const callArg = mockUpdateWorkflowRun.mock.calls[0][1] as Record<string, unknown>;
|
||||
expect(callArg).not.toHaveProperty('status');
|
||||
// Message must indicate auto-resuming
|
||||
const body = (await response.json()) as { success: boolean; message: string };
|
||||
expect(body.message).toContain('resuming');
|
||||
expect(body.message).not.toContain('Send a message');
|
||||
});
|
||||
|
||||
test('dispatches to parent conversation when parent_conversation_id is set', async () => {
|
||||
mockGetWorkflowRun.mockResolvedValueOnce(MOCK_LOOP_RUN);
|
||||
mockGetConversationById.mockResolvedValueOnce({
|
||||
id: 'parent-conv-uuid',
|
||||
platform_conversation_id: 'web-parent-abc',
|
||||
});
|
||||
const { app } = makeApp();
|
||||
await app.request('/api/workflows/runs/run-loop-1/approve', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ comment: 'proceed' }),
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
});
|
||||
// Allow fire-and-forget microtask to flush
|
||||
await new Promise(resolve => setTimeout(resolve, 0));
|
||||
expect(mockHandleMessage).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
'web-parent-abc',
|
||||
'proceed',
|
||||
expect.anything()
|
||||
);
|
||||
});
|
||||
|
||||
test('falls back to conversation_id when parent_conversation_id is null', async () => {
|
||||
const runNullParent = {
|
||||
...MOCK_LOOP_RUN,
|
||||
parent_conversation_id: null,
|
||||
conversation_id: 'worker-conv-uuid',
|
||||
};
|
||||
mockGetWorkflowRun.mockResolvedValueOnce(runNullParent);
|
||||
mockGetConversationById.mockResolvedValueOnce({
|
||||
id: 'worker-conv-uuid',
|
||||
platform_conversation_id: 'web-worker-abc',
|
||||
});
|
||||
const { app } = makeApp();
|
||||
await app.request('/api/workflows/runs/run-loop-1/approve', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ comment: 'go' }),
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
});
|
||||
expect(mockGetConversationById).toHaveBeenCalledWith('worker-conv-uuid');
|
||||
});
|
||||
|
||||
test('returns 500 when parent conversation cannot be resolved', async () => {
|
||||
mockGetWorkflowRun.mockResolvedValueOnce(MOCK_LOOP_RUN);
|
||||
mockGetConversationById.mockResolvedValueOnce(null);
|
||||
const { app } = makeApp();
|
||||
const response = await app.request('/api/workflows/runs/run-loop-1/approve', {
|
||||
method: 'POST',
|
||||
body: JSON.stringify({ comment: 'proceed' }),
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
});
|
||||
expect(response.status).toBe(500);
|
||||
const body = (await response.json()) as { error: string };
|
||||
expect(body.error).toContain('could not auto-resume');
|
||||
expect(mockHandleMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in a new issue