Skip to content

fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog#1329

Merged
Wirasm merged 3 commits intodevfrom
fix/web-approval-gate-auto-resume
Apr 22, 2026
Merged

fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog#1329
Wirasm merged 3 commits intodevfrom
fix/web-approval-gate-auto-resume

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Apr 21, 2026

Fresh re-do of #1147 (closed — 87 commits stale, reject UX had moved on). Full credit to @jonasvanderhaegen via `Co-authored-by:`.

Problem

Approving or rejecting a paused workflow from the Web UI recorded the decision but never continued the workflow. The user had to send a follow-up chat message (or use the CLI) to make anything happen. Closes #1131.

Root causes (three, intertwined)

  1. parentConversationId was never set on web-dispatched runs. orchestrator-agent.ts calls executeWorkflow at three sites for web/chat dispatches (`foreground_resume_detected`, `workflow.interactive`, and the single-workflow else branch) and omitted the 11th positional arg. Without that field on the `workflow_runs` row, `findResumableRunByParentConversation` couldn't find the paused run from the same conversation on a follow-up, and the approve/reject API handlers had no conversation to dispatch back to.

  2. /approve and /reject didn't dispatch resume. Both endpoints recorded the decision and returned `"Send a message to continue the workflow."` — a UX dead-end. The CLI equivalents (`workflowApproveCommand` / `workflowRejectCommand`) already auto-resume via `workflowRunCommand({ resume: true })`; the web endpoints needed the same.

  3. DAG executor aborted concurrent streams on `paused`. The during-streaming cancel check read `streamStatus !== 'running'` and aborted any in-flight AI node whenever the run transitioned out of running — including the legitimate `paused` transition that an approval node performs. In a layer where an approval node runs alongside an AI node, the AI node got torn down mid-stream.

Fix

  • `packages/core/src/orchestrator/orchestrator-agent.ts` — pass `conversation.id` as `parentConversationId` to all three web-side `executeWorkflow` call sites.

  • `packages/server/src/routes/api.ts` — new `tryAutoResumeAfterGate(run, logPrefix)` helper. After approve/reject marks the run, if `run.parent_conversation_id` is set, look up the parent conversation, build `/workflow run `, and `dispatchToOrchestrator` it. Failures are non-fatal — the user can still manually send any message to resume. Response message changes to `"Resuming workflow."` / `"Running on-reject prompt."` on success and falls back to the old `"Send a message to continue."` when the run has no parent (e.g. CLI-dispatched runs).

  • `packages/workflows/src/dag-executor.ts` — tolerate `paused` in the during-streaming status check. Only null / unknown / truly-terminal states abort the in-flight stream now. Inline comment explains the layer-concurrency reasoning.

UX changes

  • `ConfirmRunActionDialog` gains an optional `reasonInput` prop (`{ label, placeholder }`). When supplied, a textarea renders below the description and the trimmed value is passed to `onConfirm(reason?: string)`. Empty-after-trim becomes `undefined` so callers can distinguish "no reason" from "empty string". The dialog resets the textarea on close via `onOpenChange` so a previous reason doesn't bleed into the next reject.

  • `WorkflowRunCard.tsx` (dashboard) — Reject button now uses `reasonInput`. Description clarifies what happens to the reason: propagates to `on_reject` prompt as `$REJECTION_REASON`, or the run is cancelled if no `on_reject` is configured.

  • `WorkflowProgressCard.tsx` (chat) — Reject button upgraded from `window.confirm` to the same `ConfirmRunActionDialog` as the dashboard, with matching `reasonInput`. Two improvements in one: UX consistency + reason capture.

  • Signature propagation: `onReject?: (runId: string, reason?: string) => void` across `WorkflowRunCard`, `WorkflowRunGroup`, and `DashboardPage.handleReject`.

Tests

`packages/server/src/routes/api.workflow-runs.test.ts` (5 new):

  • approve with `parent_conversation_id` set → dispatches `/workflow run deploy ` through `handleMessage` with the correct platform conversation id, returns `"Resuming workflow"`
  • approve with `parent_conversation_id = null` (CLI-dispatched run) → no dispatch, returns `"Send a message to continue"`
  • approve where parent conversation has been deleted → `getConversationById` returns null, no dispatch, falls back to manual-resume message
  • reject with `on_reject` configured and parent set → dispatches resume, returns `"Running on-reject prompt"`
  • reject that cancels (no `on_reject`) → `cancelWorkflowRun` called, no dispatch (nothing to resume to)

`packages/core/src/orchestrator/orchestrator.test.ts` — updated the two `synthesizedPrompt`-dispatch tests for the new `executeWorkflow` arity (added `undefined, undefined, expect.anything()` for issueContext / isolationContext / parentConversationId).

Validation

  • `bun run validate` green locally — type-check + lint + format:check + check:bundled + full test suite across all 10 packages

Out of scope

  • Approve button doesn't gain a comment textarea — the current `comment` query-param on the endpoint already works for CLI; adding a UI for it is a separate enhancement if anyone asks.
  • GitHub adapter approval flow — not web, uses a different path; unchanged here.
  • Approval node schema / DB schema — unchanged.

Closes #1131.

Co-authored-by: Jonas Vanderhaegen 7755555+jonasvanderhaegen@users.noreply.github.com

Summary by CodeRabbit

  • New Features

    • Paused workflows auto-resume after Approve/Reject from the Web UI when the run originated from the Web UI.
    • Reject flow accepts an optional free-text reason; trimmed reason is passed into the workflow as rejection context.
  • Bug Fixes / UX

    • Streaming from concurrent nodes continues even if a run is paused by an approval gate.
    • Approve/Reject responses now surface “Resuming workflow” vs “Send a message to continue” appropriately.
  • Documentation

    • Guides updated to reflect auto-resume and the “Reject with reason” UX.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 21, 2026

📝 Walkthrough

Walkthrough

Threads parent conversation IDs through orchestrator dispatch, adds API auto-resume logic for Web UI approve/reject actions, tolerates paused status during streaming, and updates the Web UI reject dialog to accept and forward an optional rejection reason to workflows.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Added entry noting Web UI Approve/Reject now auto-resume paused workflows.
Orchestrator Dispatch
packages/core/src/orchestrator/orchestrator-agent.ts, packages/core/src/orchestrator/orchestrator.test.ts, packages/core/src/orchestrator/orchestrator-agent.test.ts
Pass conversation.id as parentConversationId to executeWorkflow for resumable/web/interactive dispatch paths; tests updated to expect added positional args.
API Approval/Rejection Routes & Tests
packages/server/src/routes/api.ts, packages/server/src/routes/api.workflow-runs.test.ts
Added tryAutoResumeAfterGate helper; /approve and /reject conditionally dispatch /workflow run <name> <user_message> to orchestrator when a web parent conversation exists; tests cover positive/negative cases.
Web UI: Reject Dialog & Flow
packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx, packages/web/src/components/chat/WorkflowProgressCard.tsx, packages/web/src/components/dashboard/WorkflowRunCard.tsx, packages/web/src/components/dashboard/WorkflowRunGroup.tsx, packages/web/src/routes/DashboardPage.tsx
Replaced confirm prompt with ConfirmRunActionDialog that optionally collects a trimmed reason; propagated reason?: string through UI handlers and server mutation to populate $REJECTION_REASON for on_reject prompts.
DAG Executor Streaming Behavior & Tests
packages/workflows/src/dag-executor.ts, packages/workflows/src/dag-executor.test.ts
Exported shouldContinueStreamingForStatus and updated streaming/loop gating to treat 'paused' as non-terminal so streaming can continue for concurrent nodes; added tests for status behavior.
Docs
packages/docs-web/src/content/docs/guides/approval-nodes.md, packages/docs-web/src/content/docs/guides/authoring-workflows.md
Documented Web UI auto-resume behavior, web-only caveat, and "Reject with reason" propagation to $REJECTION_REASON.

Sequence Diagram

sequenceDiagram
    participant User as User (Web UI)
    participant WebUI as Web UI
    participant API as API Server
    participant DB as Database
    participant Orchestrator as Orchestrator
    participant Executor as Workflow Executor

    User->>WebUI: Click Approve/Reject (optional reason)
    WebUI->>API: POST /api/workflows/runs/:id/approve or /reject (reason)
    API->>DB: update run approval/rejection metadata
    API->>DB: read run.parent_conversation_id
    alt parent_conversation_id present
        API->>DB: fetch parent conversation (platform_type, platform_conversation_id)
        alt platform_type == "web"
            API->>Orchestrator: dispatchToOrchestrator("/workflow run <name> <user_message>")
            Orchestrator->>Executor: executeWorkflow(..., parentConversationId)
            Executor-->>Orchestrator: resume run from paused node
        else
            API-->>WebUI: "Send a message to continue"
        end
    else
        API-->>WebUI: "Send a message to continue"
    end
    API-->>WebUI: response (auto-resumed or manual required)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐇 I hopped through code where pauses lay,

A button clicked — no more delay.
I trimmed your reason, stored it neat,
Sent the run on nimble feet.
Hooray — the workflow wakes and plays!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and specifically summarizes the main changes: web approval gates auto-resume and reject-with-reason dialog. It directly addresses the primary bug fix and UX improvements.
Description check ✅ Passed Description covers problem statement, root causes, fixes across all affected modules, UX changes, tests, validation, and scope boundaries. Follows template structure (problem, root causes, fix, UX changes, tests, validation, out of scope).
Linked Issues check ✅ Passed All requirements from #1131 are met: parentConversationId is threaded through web-side executeWorkflow call sites [orchestrator-agent.ts], API handlers auto-resume via tryAutoResumeAfterGate [api.ts], and DAG executor tolerates paused status [dag-executor.ts].
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving #1131: orchestrator auto-resume wiring, server API handlers, DAG executor streaming tolerance, UI dialog enhancement, and related tests/docs. No extraneous changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/web-approval-gate-auto-resume

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/core/src/orchestrator/orchestrator-agent.ts (1)

288-300: ⚠️ Potential issue | 🟠 Major

Thread issueContext through instead of hard-coding undefined.

handleWorkflowInvocationResult receives issueContext, and executeWorkflow has a dedicated issueContext parameter. Passing undefined here drops issue/PR context for foreground and resumed workflow nodes.

🛠️ Suggested fix direction
 async function dispatchOrchestratorWorkflow(
   platform: IPlatformAdapter,
   conversationId: string,
   conversation: Conversation,
   codebase: Codebase,
   workflow: WorkflowDefinition,
   userMessage: string,
-  isolationHints?: HandleMessageContext['isolationHints']
+  isolationHints?: HandleMessageContext['isolationHints'],
+  issueContext?: string
 ): Promise<void> {
         userMessage,
         conversation.id,
         codebase.id,
-        undefined, // issueContext
+        issueContext,
         undefined, // isolationContext
         conversation.id // parentConversationId — enables approve/reject auto-resume

Also pass issueContext from handleWorkflowInvocationResult(...) into dispatchOrchestratorWorkflow(...), and include it in the background dispatch context if that path supports it.

Also applies to: 303-315, 332-344

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

In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 288 - 300,
The call to executeWorkflow in orchestrator-agent.ts is passing undefined for
issueContext, which drops PR/issue context; thread the issueContext received by
handleWorkflowInvocationResult through into executeWorkflow and into
dispatchOrchestratorWorkflow so the same context is used for foreground,
resumed, and background runs. Update the signature/usage in
handleWorkflowInvocationResult -> dispatchOrchestratorWorkflow ->
executeWorkflow to accept an issueContext parameter and forward it (including
where the background dispatch context is built) so background dispatches also
include the issueContext instead of undefined.
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator.test.ts (1)

1076-1088: Assert the exact parent conversation ID in these regression tests.

expect.anything() would still pass if the wrong non-null ID were forwarded. Since this PR fixes conversation.id propagation for auto-resume, the tests should pin that exact value.

🧪 Suggested assertion tightening
       expect(mockExecuteWorkflow).toHaveBeenCalledWith(
         expect.anything(), // deps
         expect.anything(), // platform
         expect.anything(), // conversationId
         expect.anything(), // cwd
         expect.anything(), // workflow
         synthesized, // synthesizedPrompt, not original message
-        expect.anything(), // conversation.id
-        expect.anything(), // codebase.id
+        mockConversation.id, // conversation.id
+        mockCodebase.id, // codebase.id
         undefined, // issueContext
         undefined, // isolationContext
-        expect.anything() // parentConversationId — web approval auto-resume
+        mockConversation.id // parentConversationId — web approval auto-resume
       );

Apply the same tightening in the fallback-message assertion.

Also applies to: 1104-1116

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

In `@packages/core/src/orchestrator/orchestrator.test.ts` around lines 1076 -
1088, The test currently uses expect.anything() for the parentConversationId
when asserting mockExecuteWorkflow calls, which can hide regressions; update the
assertions in orchestrator.test.ts (the mockExecuteWorkflow expectations that
include synthesized) to assert the exact conversation.id value used for
auto-resume (replace the final expect.anything() with the specific
conversation.id constant/variable referenced in the test), and apply the same
tightening to the fallback-message assertion block around the other
mockExecuteWorkflow call (the block at the 1104-1116 range) so both assertions
explicitly verify the exact parentConversationId instead of using
expect.anything().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/server/src/routes/api.ts`:
- Around line 1038-1073: The tryAutoResumeAfterGate function currently fires
dispatchToOrchestrator with a leading void which allows rejected promises to
escape try/catch; change this to await the Promise returned by
dispatchToOrchestrator (in tryAutoResumeAfterGate) and handle its result (check
the returned {accepted, status} and log non-accepted responses) so any rejection
is caught by the existing try/catch and the function's success/false return
remains consistent with actual dispatch outcome.

---

Outside diff comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 288-300: The call to executeWorkflow in orchestrator-agent.ts is
passing undefined for issueContext, which drops PR/issue context; thread the
issueContext received by handleWorkflowInvocationResult through into
executeWorkflow and into dispatchOrchestratorWorkflow so the same context is
used for foreground, resumed, and background runs. Update the signature/usage in
handleWorkflowInvocationResult -> dispatchOrchestratorWorkflow ->
executeWorkflow to accept an issueContext parameter and forward it (including
where the background dispatch context is built) so background dispatches also
include the issueContext instead of undefined.

---

Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator.test.ts`:
- Around line 1076-1088: The test currently uses expect.anything() for the
parentConversationId when asserting mockExecuteWorkflow calls, which can hide
regressions; update the assertions in orchestrator.test.ts (the
mockExecuteWorkflow expectations that include synthesized) to assert the exact
conversation.id value used for auto-resume (replace the final expect.anything()
with the specific conversation.id constant/variable referenced in the test), and
apply the same tightening to the fallback-message assertion block around the
other mockExecuteWorkflow call (the block at the 1104-1116 range) so both
assertions explicitly verify the exact parentConversationId instead of using
expect.anything().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a50eec34-d7d6-4d47-ba63-0ea1764bff18

📥 Commits

Reviewing files that changed from the base of the PR and between ba4b9b4 and 3b16dd6.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • packages/core/src/orchestrator/orchestrator-agent.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/server/src/routes/api.ts
  • packages/server/src/routes/api.workflow-runs.test.ts
  • packages/web/src/components/chat/WorkflowProgressCard.tsx
  • packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx
  • packages/web/src/components/dashboard/WorkflowRunCard.tsx
  • packages/web/src/components/dashboard/WorkflowRunGroup.tsx
  • packages/web/src/routes/DashboardPage.tsx
  • packages/workflows/src/dag-executor.ts

Comment thread packages/server/src/routes/api.ts
@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 21, 2026

PR Review Summary (multi-agent)

Ran code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, and code-simplifier in parallel. Findings below are aggregated and de-duplicated; each has been cross-checked against the actual diff/code before inclusion.

Verdict: NEEDS FIXES

Core logic is sound — paused tolerance in dag-executor, ConfirmRunActionDialog state reset, and onReject signature propagation are all well-designed. But two correctness issues should land before merge, plus a handful of smaller items.


Critical / Important

# Agent Issue Location
C1 code-reviewer (82) + verified parentConversationId passed on the non-web else branch. The third executeWorkflow call site is the else of if (platform.getPlatformType() === 'web'), not an inner web branch. Slack/Telegram/GitHub/Discord runs will now carry parent_conversation_id; if a user ever approves such a run via the Web API, tryAutoResumeAfterGate will look up the parent, get a Slack thread_ts / Telegram chat_id, and call dispatchToOrchestrator(…) (wired to the web adapter + lock manager) — misfiring through the wrong adapter. Fix: drop the new 11th arg from that call site (packages/core/src/orchestrator/orchestrator-agent.ts:326-335), or guard in tryAutoResumeAfterGate on parentConv.platform === 'web'. orchestrator-agent.ts:326-335
C2 silent-failure-hunter (HIGH) + verified void dispatchToOrchestrator(…) drops the promise. The .dispatched log and the HTTP response "Resuming workflow." fire before async work completes. dispatchToOrchestrator does log/surface errors internally via SSE — but the response claims resume succeeded even when dispatch will ultimately throw, and the outer try/catch in the helper can never see those errors. Fix: replace void with await; the existing catch then correctly covers both sync and async failure. api.ts new tryAutoResumeAfterGate (void dispatchToOrchestrator(platformConvId, resumeMessage))
I1 code-reviewer (80) + simplifier (medium) + silent-failure-hunter (medium) logPrefix template yields 3-segment event names (api.workflow_approve_auto_resume.dispatched) that violate the {domain}.{action}_{state} convention used everywhere else. Also hurts greppability — tooling can't search for a literal event name. Fix: drop the logPrefix param, use inline literal event names (api.workflow_approve_auto_resume_dispatched, etc.) and add { action: 'approve' | 'reject' } as a structured field if needed. api.ts tryAutoResumeAfterGate
I2 comment-analyzer (CRITICAL) + verified The approve-handler inline comment claims parentConversationId is passed "for every web-dispatched foreground/interactive workflow." Background web dispatches also set parent_conversation_id (see orchestrator.ts:353dispatchBackgroundWorkflow stores parent_conversation_id: ctx.conversationDbId on the pre-created row). A future reader will incorrectly conclude background workflows won't auto-resume. Fix: drop the qualifier or write "every web-dispatched workflow (foreground, interactive, and background)." api.ts:1935-1939
I3 pr-test-analyzer (8/10) The core DAG-executor fix — tolerating streamStatus === 'paused' so a concurrent AI node isn't torn down when a sibling approval node pauses the run — has no behavioral test. The existing cancel tests use getWorkflowRunStatus returning 'cancelled'; an analogous test returning 'paused' and asserting the stream continues is missing. Without it a future refactor could revert the tolerance silently. dag-executor.test.ts (missing)
I4 docs-impact (must-fix) Two docs pages still say "send a follow-up message to resume" for Web UI approve/reject — directly contradicted by this PR. packages/docs-web/src/content/docs/guides/approval-nodes.md:58-60 step 4 and guides/authoring-workflows.md:1025, 1029 need updates. approval-nodes.md Web UI section (~lines 140-143) also needs a note about the new reject-with-reason dialog and $REJECTION_REASON propagation. docs-web/src/content/docs/guides/{approval-nodes,authoring-workflows}.md

Suggestions

# Agent Suggestion Location
S1 simplifier (medium) The negative-of-negative if (streamStatus === null || (streamStatus !== 'running' && streamStatus !== 'paused')) reads against the comment. Express the safe list positively: const shouldContinue = streamStatus === 'running' || streamStatus === 'paused'; if (!shouldContinue) { … }. dag-executor.ts:627
S2 simplifier (medium) handleReject now wraps rejectWorkflowRun in a closure to squeeze it through runAction's (runId) => … signature. Inlining the reject path directly (3 extra lines) is clearer than the closure workaround; keeps runAction usefully narrow for approve. DashboardPage.tsx:272-297
S3 silent-failure-hunter (HIGH, pre-existing) cancelWorkflowRun (db/workflows.ts:572-586) does not check rowCount after UPDATE, violating the project rule followed by the neighbouring updateWorkflowRun. Pre-existing defect, but this PR adds a new call site in the reject max-attempts branch, making it newly reachable from a web action. packages/core/src/db/workflows.ts:572-586
S4 pr-test-analyzer (5/10) expect.anything() for the new parentConversationId positional in orchestrator.test.ts matches both a real UUID and undefined. If a future edit drops conversation.id at a call site, the test still passes. Tighten to expect.stringMatching(/^[0-9a-f-]{36}$/) or a specific value. orchestrator.test.ts:1084, 1109
S5 pr-test-analyzer (6/10) No assertion on the fallback response body "Send a message to continue." — existing tests hit that branch but never check the string. A one-line expect(body.message).toContain(…) prevents silent copy drift. api.workflow-runs.test.ts
S6 comment-analyzer (delete) The onConfirm JSDoc in ConfirmRunActionDialog names runAction in DashboardPage.tsx as "all callsites." After this PR WorkflowProgressCard.tsx is a new callsite that bypasses runAction — the comment was already stale at merge time. Delete it; the signature is self-descriptive. ConfirmRunActionDialog.tsx onConfirm prop JSDoc
S7 comment-analyzer tryAutoResumeAfterGate docstring's "the user can resume manually by sending any message" is misleading — routing doesn't reliably resume on arbitrary messages. Change to "by re-running the workflow command." api.ts:1038-1049
S8 type-design (future) executeWorkflow now has 12 positional params with 5 optional trailing; undefined, undefined, conversation.id call sites rely on inline comments to convey intent. Out of scope here, but flag for a standalone ExecuteWorkflowOptions bag refactor. workflows/src/executor.ts:230-247

Strengths

  • paused tolerance inline comment in dag-executor.ts:622-631 is an exemplar — captures a non-obvious concurrency invariant (approval node pauses run ↔ sibling AI node mid-stream in same topological layer).
  • ConfirmRunActionDialog state reset via onOpenChange → setReason('') correctly prevents reason-bleed across open/close cycles on the same card.
  • Trim-to-undefined encoded in one place (the dialog) rather than scattered across callers; well-designed encapsulation.
  • onReject signature widening mechanically clean across 4 layers (WorkflowRunGroup as pure passthrough; handleReject closure captures correctly).
  • CHANGELOG entry placed correctly under Unreleased > Fixed with accurate root-cause summary.
  • New server-route tests cover all five branches the PR description claims: parent set → dispatch, parent null → fallback, parent deleted → fallback, reject+on_reject → dispatch, reject cancel → no dispatch. (pr-test-analyzer initially reported these missing — analyzer was looking at a stale view; verified present in the diff.)

Recommended merge path

  1. Drop the 11th arg from the non-web else at orchestrator-agent.ts:326-335 (C1) — smallest correctness fix, unblocks most cross-platform surprises.
  2. await dispatchToOrchestrator(…) inside tryAutoResumeAfterGate (C2) — aligns response text with reality.
  3. Collapse the 3-segment log events and drop logPrefix (I1).
  4. Add the paused tolerance test (I3) — one-liner scenario in dag-executor.test.ts.
  5. Fix the approve inline comment qualifier (I2) + the two docs pages (I4).
  6. Suggestions S1-S7 are non-blocking but cheap; S8 is a standalone follow-up.

Generated by /prp-review-agents. Each finding was verified against the actual diff/code before inclusion; agent false-positives were dropped.

Wirasm and others added 2 commits April 22, 2026 11:50
…th-reason dialog

Fixes three tightly-coupled bugs that made web approval gates unusable:

1. orchestrator-agent did not pass parentConversationId to executeWorkflow
   for any web-dispatched foreground / interactive / resumable run. Without
   that field, findResumableRunByParentConversation (the machinery the CLI
   relies on for resume) couldn't find the paused run from the same
   conversation on a follow-up message, and the approve/reject API handlers
   had no conversation to dispatch back to.

2. POST /api/workflows/runs/:runId/{approve,reject} recorded the decision
   and returned "Send a message to continue the workflow." — the workflow
   never actually resumed. Added tryAutoResumeAfterGate() that mirrors what
   workflowApproveCommand / workflowRejectCommand already do on the CLI:
   look up the parent conversation, dispatch `/workflow run <name>
   <userMessage>` back through dispatchToOrchestrator. Failures are
   non-fatal — the user can still send a manual message as a fallback.

3. The during-streaming cancel-check in dag-executor aborted any streaming
   node whenever the run status left 'running', including the legitimate
   transition to 'paused' that an approval node performs. A concurrent AI
   node in the same DAG layer now tolerates 'paused' and finishes its own
   stream; only truly terminal / unknown states (null, cancelled, failed,
   completed) abort the in-flight stream.

Web UI: ConfirmRunActionDialog gains an optional reasonInput prop (label +
placeholder) that renders a textarea and passes the trimmed value to
onConfirm. WorkflowRunCard (dashboard) and WorkflowProgressCard (chat)
both use it for Reject now — the chat card was still on window.confirm,
which was both inconsistent with the dashboard and couldn't collect a
reason. The trimmed reason threads through to $REJECTION_REASON in the
workflow's on_reject prompt.

Supersedes #1147. @jonasvanderhaegen surfaced the root cause and shape of
the fix; that PR was 87 commits stale and pre-dated the reject-UX upgrade
(#1261 area), so this is a fresh re-do on current dev.

Tests:
- packages/server/src/routes/api.workflow-runs.test.ts — 5 new cases:
  approve with parent dispatches; approve without parent returns "Send a
  message"; approve with deleted parent conversation skips safely; reject
  dispatches on-reject flows; reject that cancels (no on_reject) does NOT
  dispatch.
- packages/core/src/orchestrator/orchestrator.test.ts — updated the two
  synthesizedPrompt-dispatch tests for the new executeWorkflow arity.

Closes #1131.

Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>
C1 (critical) — cross-adapter misrouting guard
  tryAutoResumeAfterGate now checks parentConv.platform_type === 'web'
  before dispatching. Non-web parents (Slack/Telegram/GitHub/Discord)
  being approved from the dashboard skip auto-resume rather than
  dispatching a Slack thread_ts or Telegram chat_id through the web
  adapter's lock manager.

C2 (critical) — fire-and-forget dispatch replaced with await
  void dispatchToOrchestrator() meant the "Resuming workflow." response
  fired before async work completed, and the outer try/catch couldn't
  observe dispatch failures. Changed to await; response now accurately
  reflects dispatch outcome.

I1 — replaced logPrefix string-template (which produced 3-segment
  api.workflow_*.dispatched event names violating {domain}.{action}_{state})
  with literal event names per action, branched inside the helper.
  Accepts action: 'approve' | 'reject' instead.

I2 — corrected misleading "foreground/interactive" qualifier in the
  approve-endpoint comment; background web dispatches also set
  parent_conversation_id via the pre-created run, so they auto-resume too.

I3 — extracted shouldContinueStreamingForStatus() as a small exported
  policy and added 7 unit tests covering running/paused/null/cancelled/
  failed/completed/unknown. Full-integration coverage of the paused-
  tolerance invariant would require manipulating the 10s
  CANCEL_CHECK_INTERVAL_MS, which is flaky-prone; unit test of the
  policy function captures the same invariant deterministically.

I4 — updated approval-nodes.md and authoring-workflows.md to reflect
  that Web UI approve/reject now auto-resumes (no "send a follow-up
  message" copy), documented the reject-with-reason dialog and
  $REJECTION_REASON flow, and called out the cross-platform caveat.

S1 — rewrote streaming status check as positive shouldContinue safe-list
  via the extracted policy function, matching the inline comment.

S2 — inlined handleReject on the dashboard rather than squeezing
  rejectWorkflowRun through runAction with a closure; keeps runAction
  narrow for the single-arg lifecycle actions.

S5 — new regression test covering the non-web-parent skip path
  (slack-platform parent → dispatch skipped → response falls back to
  "Send a message to continue").

S6 — removed stale reference to runAction in ConfirmRunActionDialog's
  onConfirm JSDoc (no longer accurate now that WorkflowProgressCard
  calls the dialog without runAction).

S7 — fixed misleading "user can resume manually by sending any message"
  docstring (resume is triggered by re-running the workflow command,
  not by an arbitrary message).

Skipped as out-of-scope:
  S3 — cancelWorkflowRun rowCount check (pre-existing defect; separate PR)
  S4 — tightening expect.anything() to UUID regex (deferred)
  S8 — 12-positional-arg executeWorkflow → options-bag refactor
    (tracked follow-up)

bun run validate green locally; 68 tests in api.workflow-runs.test.ts
(up from 67), 173 in dag-executor.test.ts (up from 166).
@Wirasm Wirasm force-pushed the fix/web-approval-gate-auto-resume branch from 3b16dd6 to 6704ead Compare April 22, 2026 08:52
@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 22, 2026

Pushed `6704ead9` addressing the multi-agent review. Also rebased on current `dev` to resolve the CHANGELOG conflict (MCP filter from #1327 and this PR's auto-resume entry both land under Fixed).

Critical (2/2)

  • C1 — cross-adapter misrouting guard
    `tryAutoResumeAfterGate` now checks `parentConv.platform_type === 'web'` before dispatching. Non-web parents (Slack/Telegram/GitHub/Discord) being approved from the dashboard skip auto-resume gracefully — prevents routing a Slack `thread_ts` or Telegram `chat_id` through the web adapter's lock manager. New test case locks this in.

  • C2 — fire-and-forget dispatch replaced with await
    `void dispatchToOrchestrator` meant the "Resuming workflow." response could fire before async work completed, and the outer try/catch couldn't observe dispatch failures. Changed to `await` so the response text reflects reality.

Important (4/4)

  • I1 — inline literal event names
    Dropped the `logPrefix` string-template. Helper now takes `action: 'approve' | 'reject'` and branches into literal event names like `api.workflow_approve_auto_resume_dispatched` (single-dot, matches the `{domain}.{action_state}` convention).

  • I2 — corrected comment qualifier
    "every web-dispatched foreground/interactive workflow" → "any web-dispatched workflow — foreground, interactive, and background". Background dispatches do get `parent_conversation_id` via the pre-created run; the old comment would have misled future readers.

  • I3 — paused-tolerance coverage
    Extracted `shouldContinueStreamingForStatus()` as a small exported policy function; 7 unit tests cover every state (running/paused/null/cancelled/failed/completed/unknown). Integration-level coverage of the paused-tolerance invariant would require manipulating the 10s `CANCEL_CHECK_INTERVAL_MS`, which is flaky-prone; pure-function tests capture the same invariant deterministically.

  • I4 — doc updates
    `approval-nodes.md` now reflects that Web UI approve/reject auto-resumes, documents the reject-with-reason dialog and `$REJECTION_REASON` flow, and calls out the cross-platform caveat. `authoring-workflows.md` resume-paths list updated to match.

Suggestions (5 accepted, 3 deferred)

ID Status
S1 ✅ rewrote streaming check as positive `shouldContinue` safe-list via the extracted policy
S2 ✅ inlined `handleReject` on the dashboard rather than squeezing through `runAction`
S3 ⚪ deferred — `cancelWorkflowRun` rowCount is a pre-existing defect; separate focused PR
S4 ⚪ deferred — UUID regex vs `expect.anything()` — small hygiene, not worth churn
S5 ✅ new non-web-parent test also asserts the "Send a message to continue" fallback copy
S6 ✅ removed stale `runAction` mention from `ConfirmRunActionDialog` JSDoc
S7 ✅ fixed misleading "send any message" docstring on `tryAutoResumeAfterGate`
S8 ⚪ deferred — 12-positional-arg `executeWorkflow` → options-bag refactor, tracked as follow-up

`bun run validate` green locally; CI should go green on the rebase.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/web/src/components/dashboard/WorkflowRunCard.tsx (1)

321-345: ⚠️ Potential issue | 🟡 Minor

Only collect a rejection reason when on_reject exists.

Line 338 always shows the reason textarea, but the reason is only consumed when the approval metadata has an onRejectPrompt. Without that prompt, the run is cancelled and the typed reason is effectively discarded.

Suggested conditional rendering
 export function WorkflowRunCard({
   run,
   isDocker,
   onCancel,
@@
   const displayMessage = run.user_message
     ? messageExpanded || !longMessage
       ? run.user_message
       : run.user_message.slice(0, 80) + '…'
     : null;
+  const approvalMetadata =
+    run.metadata?.approval != null && typeof run.metadata.approval === 'object'
+      ? (run.metadata.approval as Record<string, unknown>)
+      : undefined;
+  const hasOnRejectPrompt =
+    typeof approvalMetadata?.onRejectPrompt === 'string' &&
+    approvalMetadata.onRejectPrompt.trim() !== '';
 
   return (
@@
               title="Reject workflow?"
               description={
-                <>
-                  Reject the paused workflow <strong>{run.workflow_name}</strong>. If the approval
-                  node defines an <code>on_reject</code> prompt, it runs with your reason as{' '}
-                  <code>$REJECTION_REASON</code>; otherwise the run is cancelled.
-                </>
+                hasOnRejectPrompt ? (
+                  <>
+                    Reject the paused workflow <strong>{run.workflow_name}</strong>. The approval
+                    node&apos;s <code>on_reject</code> prompt runs with your reason as{' '}
+                    <code>$REJECTION_REASON</code>.
+                  </>
+                ) : (
+                  <>
+                    Reject the paused workflow <strong>{run.workflow_name}</strong>. This approval
+                    node has no <code>on_reject</code> prompt, so the run will be cancelled.
+                  </>
+                )
               }
-              confirmLabel="Reject"
-              reasonInput={{
-                label: 'Reason (optional)',
-                placeholder: 'Why are you rejecting? Visible to the on_reject prompt.',
-              }}
+              confirmLabel={hasOnRejectPrompt ? 'Reject' : 'Reject and cancel'}
+              reasonInput={
+                hasOnRejectPrompt
+                  ? {
+                      label: 'Reason (optional)',
+                      placeholder: 'Why are you rejecting? Visible to the on_reject prompt.',
+                    }
+                  : undefined
+              }
               onConfirm={(reason): void => {
                 onReject(run.id, reason);
               }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/components/dashboard/WorkflowRunCard.tsx` around lines 321 -
345, The Reject dialog always renders a reason textarea even when the approval
node has no on_reject handler, so update the WorkflowRunCard
ConfirmRunActionDialog usage to conditionally include the reasonInput prop:
detect the approval metadata on the run (e.g., check run.approval?.on_reject or
run.approval?.onRejectPrompt) and only pass the reasonInput object when that key
exists; keep the ConfirmRunActionDialog, the onConfirm call to onReject(run.id,
reason), and the reject trigger unchanged.
🧹 Nitpick comments (3)
packages/workflows/src/dag-executor.test.ts (1)

6041-6081: Add one executor-level regression test for the paused-stream path.

These tests cover the exported predicate, but not the actual cancel-check loop in executeDagWorkflow. A future change could stop calling shouldContinueStreamingForStatus during streaming and these tests would still pass. Please add a deterministic test where getWorkflowRunStatus() returns 'paused' while a streaming AI node is mid-response, then assert the node is allowed to complete rather than being aborted.

As per coding guidelines, **/*.test.{ts,tsx} tests should be deterministic — no flaky timing or network dependence.

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

In `@packages/workflows/src/dag-executor.test.ts` around lines 6041 - 6081, Add a
deterministic executor-level test in dag-executor.test.ts that exercises
executeDagWorkflow's cancel-check loop: stub/mock getWorkflowRunStatus so it
returns 'running' initially but yields 'paused' while a streaming AI node is
mid-response, then run executeDagWorkflow and assert the streaming node is
allowed to finish (not aborted). Specifically, locate executeDagWorkflow and the
streaming node handler, replace/mock getWorkflowRunStatus to produce the
sequence ['running', ...'paused'] during the streaming Promise (use a
controllable Promise or test double rather than timers), and assert final node
output/state shows completion; keep the test synchronous/deterministic by
resolving the streaming Promise only after the mocked status flips to 'paused'.
packages/server/src/routes/api.workflow-runs.test.ts (1)

1373-1548: LGTM on the auto-resume test coverage.

The five new cases neatly cover the approve/reject matrix: parent set (web), parent null (CLI-dispatched), parent deleted, non-web parent (Slack/Telegram cross-adapter guard), and reject-cancel. Assertions on the dispatched platformConvId and resume message (/workflow run deploy Deploy feature X) lock in the contract nicely.

Two optional tightenings you might consider:

  1. For the "parent deleted" case (lines 1438–1456), also assert mockGetConversationById was called with 'deleted-conv-uuid' so regressions that skip the lookup entirely (e.g. accidentally short-circuiting on run.parent_conversation_id) are caught.
  2. Add a sibling case where getConversationById returns a conversation with an empty string platform_conversation_id — currently only the null parent is exercised for the skippedNoPlatformConv path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/server/src/routes/api.workflow-runs.test.ts` around lines 1373 -
1548, Add an assertion that mockGetConversationById was invoked with the deleted
conversation id in the "approve: skips dispatch when parent conversation no
longer exists" test by checking mockGetConversationById.mock.calls or
toHaveBeenCalledWith('deleted-conv-uuid') so we ensure the lookup occurs; also
add a new sibling test (e.g., "approve: skips dispatch when parent conversation
has empty platform_conversation_id") that sets mockGetConversationById to
resolve to { id: 'some-id', platform_conversation_id: '', platform_type: 'web' }
and asserts the same fallback behavior (200 response, message contains 'Send a
message to continue', and mockHandleMessage not called) to cover the
empty-string platform_conv edge-case.
packages/docs-web/src/content/docs/guides/approval-nodes.md (1)

58-60: LGTM — approval-nodes guide is in sync with the new web UX.

The Approve/Reject auto-resume description, the reject-with-reason dialog behavior (trimmed value mapped to $REJECTION_REASON, empty-after-trim omitted), and the cross-platform caveat for Slack/Telegram/GitHub-dispatched runs all track the server implementation in tryAutoResumeAfterGate and the UI dialog changes.

One tiny readability nit (optional): consider collapsing the repeated "Web-UI-dispatched" wording across lines 151–155 into a single sentence — e.g. "Auto-resume only fires for runs originally dispatched from the Web UI; otherwise re-run the workflow from the originating platform." No behavior change.

Also applies to: 143-155

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

In `@packages/docs-web/src/content/docs/guides/approval-nodes.md` around lines 58
- 60, In the approval-nodes guide (section describing Approve/Reject auto-resume
behavior), collapse the repeated "Web-UI-dispatched" wording across the
paragraph that explains auto-resume into one clear sentence — e.g. replace the
multi-line repetition around the "Web-UI-dispatched" phrase (lines mentioning
auto-resume only firing for runs dispatched from the Web UI) with a single
sentence like "Auto-resume only fires for runs originally dispatched from the
Web UI; otherwise re-run the workflow from the originating platform." Keep the
described behavior (trimmed $REJECTION_REASON, omission when empty) unchanged
and preserve the cross-platform caveat for Slack/Telegram/GitHub-dispatched
runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/web/src/components/dashboard/WorkflowRunCard.tsx`:
- Around line 321-345: The Reject dialog always renders a reason textarea even
when the approval node has no on_reject handler, so update the WorkflowRunCard
ConfirmRunActionDialog usage to conditionally include the reasonInput prop:
detect the approval metadata on the run (e.g., check run.approval?.on_reject or
run.approval?.onRejectPrompt) and only pass the reasonInput object when that key
exists; keep the ConfirmRunActionDialog, the onConfirm call to onReject(run.id,
reason), and the reject trigger unchanged.

---

Nitpick comments:
In `@packages/docs-web/src/content/docs/guides/approval-nodes.md`:
- Around line 58-60: In the approval-nodes guide (section describing
Approve/Reject auto-resume behavior), collapse the repeated "Web-UI-dispatched"
wording across the paragraph that explains auto-resume into one clear sentence —
e.g. replace the multi-line repetition around the "Web-UI-dispatched" phrase
(lines mentioning auto-resume only firing for runs dispatched from the Web UI)
with a single sentence like "Auto-resume only fires for runs originally
dispatched from the Web UI; otherwise re-run the workflow from the originating
platform." Keep the described behavior (trimmed $REJECTION_REASON, omission when
empty) unchanged and preserve the cross-platform caveat for
Slack/Telegram/GitHub-dispatched runs.

In `@packages/server/src/routes/api.workflow-runs.test.ts`:
- Around line 1373-1548: Add an assertion that mockGetConversationById was
invoked with the deleted conversation id in the "approve: skips dispatch when
parent conversation no longer exists" test by checking
mockGetConversationById.mock.calls or toHaveBeenCalledWith('deleted-conv-uuid')
so we ensure the lookup occurs; also add a new sibling test (e.g., "approve:
skips dispatch when parent conversation has empty platform_conversation_id")
that sets mockGetConversationById to resolve to { id: 'some-id',
platform_conversation_id: '', platform_type: 'web' } and asserts the same
fallback behavior (200 response, message contains 'Send a message to continue',
and mockHandleMessage not called) to cover the empty-string platform_conv
edge-case.

In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 6041-6081: Add a deterministic executor-level test in
dag-executor.test.ts that exercises executeDagWorkflow's cancel-check loop:
stub/mock getWorkflowRunStatus so it returns 'running' initially but yields
'paused' while a streaming AI node is mid-response, then run executeDagWorkflow
and assert the streaming node is allowed to finish (not aborted). Specifically,
locate executeDagWorkflow and the streaming node handler, replace/mock
getWorkflowRunStatus to produce the sequence ['running', ...'paused'] during the
streaming Promise (use a controllable Promise or test double rather than
timers), and assert final node output/state shows completion; keep the test
synchronous/deterministic by resolving the streaming Promise only after the
mocked status flips to 'paused'.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e7c6e42f-15a9-4150-aba5-47d4d7f2f768

📥 Commits

Reviewing files that changed from the base of the PR and between 3b16dd6 and 6704ead.

📒 Files selected for processing (14)
  • CHANGELOG.md
  • packages/core/src/orchestrator/orchestrator-agent.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/docs-web/src/content/docs/guides/approval-nodes.md
  • packages/docs-web/src/content/docs/guides/authoring-workflows.md
  • packages/server/src/routes/api.ts
  • packages/server/src/routes/api.workflow-runs.test.ts
  • packages/web/src/components/chat/WorkflowProgressCard.tsx
  • packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx
  • packages/web/src/components/dashboard/WorkflowRunCard.tsx
  • packages/web/src/components/dashboard/WorkflowRunGroup.tsx
  • packages/web/src/routes/DashboardPage.tsx
  • packages/workflows/src/dag-executor.test.ts
  • packages/workflows/src/dag-executor.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • packages/core/src/orchestrator/orchestrator-agent.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/web/src/components/chat/WorkflowProgressCard.tsx
  • packages/web/src/routes/DashboardPage.tsx
  • packages/web/src/components/dashboard/WorkflowRunGroup.tsx
  • packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx
  • packages/workflows/src/dag-executor.ts

@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 22, 2026

PR Review Summary (multi-agent)

7 specialized agents reviewed the diff: code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, code-simplifier. No critical blockers found. Two correctness gaps worth addressing before merge.

Critical Issues (0)

None.

Important Issues (7)

ID Agent Issue Location
I1 silent-failure-hunter loop_node inter-iteration check still uses strict runStatus !== 'running' — will abort a loop node mid-iteration when a sibling approval node pauses the run, inconsistent with the fix applied to the streaming check. Same bug class this PR fixes, missed path. packages/workflows/src/dag-executor.ts:1690
I2 silent-failure-hunter skipIfStatusChanged helper has the same strict status !== 'running' guard and will unregister the run's event emitter when a sibling approval gate pauses the run. packages/workflows/src/dag-executor.ts:2860-2867
I3 pr-test-analyzer foreground_resume_detected branch was modified (added 11th positional arg) but no test exercises itmockFindResumableRunByParentConversation is never configured to return a non-null value. Positional mistake would silently break the auto-resume loop. Rating 8/10. packages/core/src/orchestrator/orchestrator-agent.ts:288-297
I4 silent-failure-hunter getConversationById returning null (deleted parent — a data-integrity signal) and a real DB exception collapse to the same user-facing message. The null path also logs at debug where info would be appropriate. packages/server/src/routes/api.ts (tryAutoResumeAfterGate, null-branch)
I5 silent-failure-hunter "Resuming workflow." response is returned as soon as dispatchToOrchestrator resolves — but that only confirms enqueue, not delivery. If SSE is disconnected or orchestrator errors downstream, user sees success text while the run never resumes. Suggest: "Resuming workflow — check the workflow dashboard for progress." packages/server/src/routes/api.ts
I6 code-reviewer Hardcoded id="confirm-run-action-reason" in ConfirmRunActionDialog produces duplicate DOM IDs when multiple dialog instances exist. Mitigated in practice by Radix portals but fragile — use React.useId(). Rating 82/100. packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx
I7 pr-test-analyzer Non-fatal tryAutoResumeAfterGate failure path (dispatch throws) is untested — a regression that propagates the exception would turn approve/reject into a 500 even though the gate decision was already recorded. Rating 6/10. packages/server/src/routes/api.workflow-runs.test.ts

Suggestions (11)

ID Agent Suggestion Location
S1 code-reviewer / type-design executeWorkflow now takes 12 positional args with two adjacent string | undefined params (positions 9 and 11) — silent transposition risk. Collapse trailing optionals into an ExecuteWorkflowOptions object (follow-up PR). packages/workflows/src/executor.ts:230
S2 code-simplifier events ternary-object in tryAutoResumeAfterGate is over-engineered for 4 one-shot log calls. Collapse to a prefix string or split into two concrete functions. packages/server/src/routes/api.ts:1063-1081
S3 code-simplifier Inline platformConvId (used twice, named intermediate adds no clarity; also skips type narrowing of parentConv). packages/server/src/routes/api.ts:1082-1095
S4 code-simplifier Duplicate reasonInput string literal across dashboard + chat reject dialogs (2 of 3 — not yet Rule of Three, but pure strings). Consider exporting REJECTION_REASON_INPUT constant from ConfirmRunActionDialog.tsx. WorkflowRunCard.tsx, WorkflowProgressCard.tsx
S5 code-simplifier onChange handler is a block-bodied arrow with explicit : void — simplify to onChange={e => setReason(e.target.value)}. ConfirmRunActionDialog.tsx:97-99
S6 code-simplifier Call-site comment above streaming-check fully duplicates the JSDoc on shouldContinueStreamingForStatus. Trim to // Policy: see shouldContinueStreamingForStatus(). dag-executor.ts:709-717
S7 silent-failure-hunter Non-web else branch at orchestrator-agent.ts:326-336 correctly omits parentConversationId but has no comment explaining why. A future reader may add it and re-enable cross-adapter misrouting. packages/core/src/orchestrator/orchestrator-agent.ts
S8 type-design ConfirmRunActionDialog: onConfirm: (reason?: string) => void is identical whether reasonInput is present or absent — a discriminated union would prevent future "why is my reason always undefined" confusion. LOW priority. ConfirmRunActionDialog.tsx
S9 comment-analyzer JSDoc of tryAutoResumeAfterGate names specific CLI callers (workflowApproveCommand / workflowRunCommand({ resume: true })) and references a "3-segment shape that broke" — comment rot risk per CLAUDE.md "don't reference callers or current fix". packages/server/src/routes/api.ts
S10 comment-analyzer Three // parentConversationId — enables approve/reject auto-resume suffix comments at executeWorkflow call sites restate the parameter name + reference the fix task. Keep the positional label (// parentConversationId), drop the suffix. orchestrator-agent.ts (3 sites), orchestrator.test.ts
S11 pr-test-analyzer Extend the interactive-workflow dispatch test to assert mockExecuteWorkflow.mock.calls[0][10] === conversation.id — currently only arity is checked. Rating 5/10. packages/core/src/orchestrator/orchestrator-agent.test.ts:1092

Strengths

  • Non-fatal design in tryAutoResumeAfterGate is correctly implemented — caught errors log at warn with run context and return false rather than propagating. Matches the PR's stated contract.
  • Cross-adapter guard (platform_type !== 'web') is well-tested — explicitly covered for Slack-sourced runs being approved from the dashboard, which is the safety-critical misrouting case.
  • shouldContinueStreamingForStatus extraction is a good move: named, unit-tested, covers null/terminal/unknown. JSDoc correctly identifies the CANCEL_CHECK_INTERVAL_MS timing constraint as the reason integration tests would be flaky.
  • ConfirmRunActionDialog reset-on-close is a valid WHY comment (React lifecycle state-bleed constraint).
  • DashboardPage.handleReject surfaces errors via setActionError rather than swallowing — correct delegation.
  • 5 new approve/reject auto-resume tests cover all non-exception branches: happy path, null parent, deleted parent, non-web parent, cancel-without-dispatch.

Documentation

  • packages/docs-web/src/content/docs/guides/approval-nodes.md — PR updates are accurate; the "How It Works" step 4 and Web UI section now correctly describe auto-resume + reason textarea + cross-platform caveat. OK on merge.
  • packages/docs-web/src/content/docs/guides/authoring-workflows.md — PR's Human-in-the-Loop pattern update is accurate. OK on merge.
  • CHANGELOG.md — entry placed correctly under ### Fixed. "87 commits stale" detail in the credit line is stylistic noise but not inaccurate.
  • CLAUDE.md:691optional minor: $REJECTION_REASON description mentions the CLI command but could add "or via the Web UI reject dialog" now that that's the primary UX path. Low priority.

Verdict

NEEDS FIXES — I1 and I2 are real correctness gaps of the same class the PR fixes (a sibling approval node pausing the run will still incorrectly abort a loop node or unregister the run's event emitter). They're small, local changes using the already-extracted shouldContinueStreamingForStatus helper. I3 (untested foreground_resume_detected branch) is the one test gap worth closing since the mock infrastructure is already in place.

Recommended Actions

  1. Before merge: apply shouldContinueStreamingForStatus at dag-executor.ts:1690 (loop inter-iteration) and dag-executor.ts:2860 (skipIfStatusChanged). Guard the user-visible "stopped at iteration" message on effectiveStatus !== 'paused' to match the between-layers pattern. (I1, I2)
  2. Before merge: add one test configuring mockFindResumableRunByParentConversation to return a resumable run, asserting parentConversationId is passed. (I3)
  3. Before merge: either soften the response text ("Resuming workflow — check the dashboard") or distinguish the deleted-parent case in logs/response. (I4, I5)
  4. Nice-to-have: useId() in ConfirmRunActionDialog (I6), dispatch-throws test (I7), comment cleanup (S9, S10).
  5. Follow-up PR: ExecuteWorkflowOptions refactor to eliminate the 12-positional-argument signature (S1).

…sume test, useId

I1 (loop inter-iteration check) — dag-executor.ts:1715
  Used `!== 'running'` in the loop node's between-iteration status check.
  A sibling approval node pausing the run in the same topological layer
  would abort the loop mid-iteration with "Loop node '<id>' stopped at
  iteration N (paused)". Switched to the shared shouldContinueStreamingForStatus
  helper so paused is tolerated — same semantics the streaming check got.
  Extended inline comment explains the sibling-layer concurrency reason.

I2 (skipIfStatusChanged emitter unregister) — dag-executor.ts:2886
  At DAG-finalization writes the helper correctly skipped writing on any
  non-running state (paused included — don't mark a paused run complete),
  but it *also* called getWorkflowEventEmitter().unregisterRun() which
  broke SSE observability for a run that's still live (waiting for user
  approval). Split the two responsibilities: skip the write for all
  non-running states, but only unregister the emitter for terminal states
  (cancelled / deleted / completed / failed). `paused` keeps the emitter
  registered so resume stays visible on the dashboard.

I3 (foreground_resume_detected branch untested) — orchestrator-agent.test.ts
  That branch was modified as part of the original fix (added
  parentConversationId as 11th positional arg) but no existing test
  configured mockFindResumableRunByParentConversation to return non-null.
  A positional mistake (e.g. accidentally swapping issueContext and
  parentConversationId) would silently break auto-resume with no failing
  test. New regression test configures the mock, asserts both the cwd
  comes from the resumable run's working_path AND parentConversationId
  is passed correctly at position 10.

I4 (null-parent log level) — api.ts tryAutoResumeAfterGate
  `getConversationById` returning null is a data-integrity signal (the
  parent conversation was deleted while the run was paused) — worth
  surfacing at info level so operators notice, not hiding at debug.
  Missing platform_conversation_id on an existing row would be an unusual
  DB state and stays at debug. Added `parentDeleted: boolean` to the log
  context so the two cases are distinguishable in observability.

I6 (hardcoded DOM id) — ConfirmRunActionDialog.tsx
  `id="confirm-run-action-reason"` collided when multiple dialog instances
  share the same page (Radix portals mitigate in practice but the code
  was fragile). Switched to React.useId() so each instance gets a unique
  id — htmlFor/id wiring preserved.

S11 (arity-only assertion) — orchestrator-agent.test.ts:1092 area
  The interactive-workflow-on-web test asserted mockExecuteWorkflow was
  called, but nothing about the args. Added a specific assertion that
  position 10 (parentConversationId) equals 'conv-1' (the caller
  conversation id) — pins the wiring that I1/I2 depend on being correct.

Deferred (from review S1-S10, I5, I7):
  - S1 (ExecuteWorkflowOptions bag) — tracked as standalone follow-up;
    12 positional args with 2 adjacent optionals is a real maintenance
    hazard but the refactor deserves its own PR.
  - S7 (WHY comment on non-web else branch) — review text says the branch
    "correctly omits" parentConversationId but the code passes it; the
    combination with the web-parent guard in tryAutoResumeAfterGate is
    intentional. Not adding a justify-what-we-don't-do comment.
  - S2/S3/S4/S5/S8/S9/S10 — pure polish (event-map ternary, platformConvId
    inlining, shared constant for REJECTION_REASON_INPUT, onChange arrow
    shorthand, discriminated union, docblock trim, suffix comment drop)
  - I5 (soften "Resuming workflow." to "— check the dashboard for progress")
    — users clicking from the dashboard are already on the dashboard; the
    current text is accurate (enqueue completed) and concise.
  - I7 (test dispatch-throws path) — covered implicitly by the try/catch
    branch of tryAutoResumeAfterGate returning false; a direct test would
    require mocking handleMessage to throw and would couple to
    dispatchToOrchestrator internals.

bun run validate green; 189 dag-executor tests, 98 orchestrator-agent
tests, 68 api.workflow-runs tests — all the new cases pass.
@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 22, 2026

Pushed `c002d4c8` addressing the multi-agent review.

Closed

ID Fix
I1 — loop inter-iteration check had same bug class Switched `dag-executor.ts:1715` to `shouldContinueStreamingForStatus`. Loop nodes no longer abort mid-iteration when a sibling approval node pauses the run. Inline comment explains the sibling-layer concurrency reason.
I2 — `skipIfStatusChanged` broke SSE observability on pause Split the helper's responsibilities: final-write skip stays strict `!== 'running'` (correct — don't mark a paused run complete), but emitter unregister only fires for terminal states (cancelled/deleted/completed/failed). `paused` keeps the emitter registered so resume stays visible on the dashboard.
I3 — `foreground_resume_detected` branch untested New regression test in `orchestrator-agent.test.ts` configures `mockFindResumableRunByParentConversation` to return a non-null run, asserts cwd comes from `working_path` (pos 3) and `parentConversationId` is `conversation.id` (pos 10).
I4 — null-parent vs DB-exception log collapse `parentConv === null` (data-integrity signal — parent deleted) now logs at `info` level with `{ parentDeleted: true }`; missing `platform_conversation_id` on an existing row stays at `debug`.
I6 — hardcoded DOM id Replaced `id="confirm-run-action-reason"` with `React.useId()`. Multiple dialog instances on the same page no longer share an id.
S11 — arity-only assertion The interactive-workflow-on-web test now also asserts `mockExecuteWorkflow.mock.calls[0][10] === 'conv-1'` — pins the wiring that I1/I2 depend on.

Deferred (tracked in #1350)

Opened a dedicated follow-up issue covering the polish and ergonomics items that don't block this merge:

  • S1 — `ExecuteWorkflowOptions` bag refactor to replace the 12-positional-arg signature (primary motivator for the issue)
  • S2/S3/S9 — `tryAutoResumeAfterGate` micro-polish (events-object shape, `platformConvId` inlining, JSDoc trim)
  • S4/S8 — `ConfirmRunActionDialog` shared `REJECTION_REASON_INPUT` constant + potential discriminated union
  • S5/S6/S10 — minor comment / style cleanups
  • I7 — test for `tryAutoResumeAfterGate`'s dispatch-throws path

Also called out in the issue why I5 (soften response text) and S7 (comment on non-web else) were intentionally not acted on.

Deliberately skipped

  • I5 — `"Resuming workflow."` response is served into the dashboard users are already viewing; adding "check the dashboard for progress" is circular. Current text is accurate (enqueue completed, SSE will show progress).
  • S7 — review claimed the non-web `else` "correctly omits" `parentConversationId`; the code does pass it, guarded downstream by the web-only check in `tryAutoResumeAfterGate`. Intentional, no comment needed.

`bun run validate` green locally; 189 dag-executor tests, 98 orchestrator-agent tests, 68 api.workflow-runs tests.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator-agent.test.ts (1)

1102-1138: LGTM — regression coverage aligns with executeWorkflow signature.

Positional index assertions (callArgs[10], callArgs[3]) correctly map to parentConversationId and cwd per the executeWorkflow signature in packages/workflows/src/executor.ts. The inline comments documenting the expected positions are helpful given the long positional arg list.

One optional note: these index-based assertions will silently drift if executeWorkflow args are reordered. Once the deferred options-bag refactor for executeWorkflow lands, consider switching to property-based assertions (e.g., expect(callArgs[0]).toMatchObject({ parentConversationId: 'conv-1' })) to make intent explicit and resilient.

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

In `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 1102
- 1138, Tests currently assert positional args from
mockExecuteWorkflow.mock.calls (e.g., expect(callArgs[10]) and
expect(callArgs[3])), which is fragile; when executeWorkflow is refactored to
accept an options-bag, update these tests to grab the options object from
mockExecuteWorkflow.mock.calls[0] and assert properties directly (e.g.,
expect(options).toMatchObject({ parentConversationId: 'conv-1', cwd:
'/repos/test-repo/worktrees/feature' })), referencing executeWorkflow and
mockExecuteWorkflow to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator-agent.test.ts`:
- Around line 1102-1138: Tests currently assert positional args from
mockExecuteWorkflow.mock.calls (e.g., expect(callArgs[10]) and
expect(callArgs[3])), which is fragile; when executeWorkflow is refactored to
accept an options-bag, update these tests to grab the options object from
mockExecuteWorkflow.mock.calls[0] and assert properties directly (e.g.,
expect(options).toMatchObject({ parentConversationId: 'conv-1', cwd:
'/repos/test-repo/worktrees/feature' })), referencing executeWorkflow and
mockExecuteWorkflow to locate the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b596e653-0cd1-4c1c-85d3-4568759d692b

📥 Commits

Reviewing files that changed from the base of the PR and between 6704ead and c002d4c.

📒 Files selected for processing (4)
  • packages/core/src/orchestrator/orchestrator-agent.test.ts
  • packages/server/src/routes/api.ts
  • packages/web/src/components/dashboard/ConfirmRunActionDialog.tsx
  • packages/workflows/src/dag-executor.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/server/src/routes/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/workflows/src/dag-executor.ts

@Wirasm Wirasm merged commit d5c1cd9 into dev Apr 22, 2026
4 checks passed
prospapledge88 added a commit to prospapledge88/Archon that referenced this pull request May 5, 2026
* fix(workflows): fail loudly on SDK isError results (coleam00#1208) (coleam00#1291)

Previously, `dag-executor` only failed nodes/iterations when the SDK
returned an `error_max_budget_usd` result. Every other `isError: true`
subtype — including `error_during_execution` — was silently `break`ed
out of the stream with whatever partial output had accumulated, letting
failed runs masquerade as successful ones with empty output.

This is the most likely explanation for the "5-second crash" symptom in
coleam00#1208: iterations finish instantly with empty text, the loop keeps
going, and only the `claude.result_is_error` log tips the user off.

Changes:
- Capture the SDK's `errors: string[]` detail on result messages
  (previously discarded) and surface it through `MessageChunk.errors`.
- Log `errors`, `stopReason` alongside `errorSubtype` in
  `claude.result_is_error` so users can see what actually failed.
- Throw from both the general node path and the loop iteration path
  on any `isError: true` result, including the subtype and SDK errors
  detail in the thrown message.

Note: this does not implement auto-retry. See PR comments on coleam00#1121 and
the analysis on coleam00#1208 — a retry-with-fresh-session approach for loop
iterations is not obviously correct until we see what
`error_during_execution` actually carries in the reporter's env.
This change is the observability + fail-loud step that has to come
first so that signal is no longer silent.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit 4c6ddd9)

* fix(db): throw on corrupt commands JSON instead of silent empty fallback (coleam00#1033)

* fix(db): throw on corrupt commands JSON instead of silent empty fallback (coleam00#967)

getCodebaseCommands() silently returned {} when the commands column
contained corrupt JSON. Callers had no way to distinguish 'no commands'
from 'unreadable data', violating fail-fast principles.

Now throws a descriptive error with the codebase ID and a recovery hint.
The error is still logged for observability before throwing.

Adds two test cases: corrupt JSON throws, valid JSON string parses.

* fix: include parse error in log for better diagnostics

(cherry picked from commit 39a05b7)

* fix(isolation): raise worktree git-operation timeout to 5m (coleam00#1306)

All 15 worktree git-subprocess timeouts in WorktreeProvider were hardcoded
at 30000ms. Repos with heavy post-checkout hooks (lint, dependency install,
submodule init) routinely exceed that budget and fail worktree creation.

Consolidate them onto a single GIT_OPERATION_TIMEOUT_MS constant at 5 min.
Generous enough to cover reported cases while still catching genuine hangs
(credential prompts in non-TTY, stalled fetches).

Chosen over the config-key approach in coleam00#1029 to avoid adding permanent
.archon/config.yaml surface for a problem a raised default solves cleanly.
If 5 min turns out to also be too tight for real-world use, we'll revisit.

Closes coleam00#1119
Supersedes coleam00#1029

Co-authored-by: Shay Elmualem <12733941+norbinsh@users.noreply.github.com>
(cherry picked from commit cc78071)

* fix(web,server): show real platform connection status in Settings (coleam00#1061)

The Settings page's Platform Connections section hardcoded every platform
except Web to 'Not configured', so users couldn't tell whether their Slack/
Telegram/Discord/GitHub/Gitea/GitLab adapters had actually started.

- Server: /api/health now returns an activePlatforms array populated live
  as each adapter's start() resolves. Passed into registerApiRoutes so the
  reference stays mutable — Telegram starts after the HTTP listener is
  already accepting requests, so a snapshot would miss it.
- Web: SettingsPage.PlatformConnectionsSection now reads activePlatforms
  from /api/health and looks each platform up in a Set. Also adds Gitea
  and GitLab to the list (they already ship as adapters).

Closes coleam00#1031

Co-authored-by: Lior Franko <liorfr@dreamgroup.com>
(cherry picked from commit 08de8ee)

* fix: initialize options.hooks before merging YAML node hooks (coleam00#1177)

When a workflow node defines hooks (PreToolUse/PostToolUse) in YAML but
no hooks exist yet on the options object, applyNodeConfig crashes with
"undefined is not an object" because it tries to assign properties on
the undefined options.hooks.

Initialize options.hooks to {} before the merge loop.

Reproduces with: archon workflow run archon-architect (which uses
per-node hooks extensively).

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
(cherry picked from commit 7ea3214)

* fix: detect completion signal in any XML tag, not just <promise> (coleam00#1126) (coleam00#1184)

* fix: detect completion signal in any XML tag, not just <promise> (coleam00#1126)

Loop nodes with `until:` reported max_iterations_reached when the AI wrapped
the completion signal in XML tags other than `<promise>` (e.g.,
`<COMPLETE>ALL_CLEAN</COMPLETE>`). The three existing regex patterns all missed
this format, causing the loop to exhaust iterations and fail.

Changes:
- Add generic XML-wrapped signal pattern to `detectCompletionSignal`
- Extend `stripCompletionTags` to strip matched XML-wrapped signals from output
- Pass `loop.until` to `stripCompletionTags` call site in dag-executor
- Add unit tests for detection and stripping of XML-wrapped signals
- Add integration test for loop completing on final iteration with XML tags

Fixes coleam00#1126

* fix: address review findings for completion signal detection

- Update detectCompletionSignal JSDoc to document all three detection formats
- Update stripCompletionTags JSDoc to mention the `until` parameter
- Remove superfluous `m` flag from xmlWrappedPattern (no anchors, no effect)
- Document that XML tag names are matched independently (intentional permissiveness)
- Add test: detects signal in mismatched XML tags (permissive behavior)
- Add test: strips both <promise> and XML-tagged signal in same chunk
- Add assertion in DAG integration test that raw XML tags don't appear in sent messages

* simplify: reduce complexity in changed files

* fix: require matching XML tag names in completion-signal detection

Follow-up to the initial broadening in this PR. The first version of the
regex accepted mismatched open/close tags (e.g. `<COMPLETE>X</done>`)
which was a small false-positive surface when the AI interleaves tags
in prose. Tightens both detectCompletionSignal and stripCompletionTags
to capture the tag name and enforce it on the close via \1
backreference. Case-insensitivity on the tag name is preserved.

Test updates:
- Flip the "permissive mismatch" case to assert strict rejection with a
  comment explaining the guard.
- Add a case-insensitive matching case to lock that behavior in.

No behavior change for workflows that use matching tags (the
overwhelming common case) or for <promise>...</promise>. Behavior change
is limited to the narrow "open tag and close tag disagree" case, which
only happens when the AI is confused — in which case we'd rather report
max_iterations_reached and let the author inspect than silently call
the loop complete.

(cherry picked from commit bc25dee)

* fix(web): allow deleting nodes from Workflow Builder (coleam00#971) (coleam00#1113)

* fix(web): allow deleting nodes from Workflow Builder (coleam00#971)

Three independent gaps prevented users from deleting nodes added to the
Workflow Builder canvas: dropped nodes were never auto-selected so
keyboard shortcuts silently no-oped, no right-click context menu
existed, and the Delete Node button was buried in the Advanced tab
(hidden below the viewport for Prompt/Command, completely absent for
Bash since bash nodes have no Advanced tab).

Fixes coleam00#971.

* fix(web): push undo snapshot before adding nodes on canvas

Call onPushSnapshot() before setNodes() in both onDrop and quick-add
handlers so that node additions are captured by undo/redo history.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR coleam00#1113 review feedback

- Hold nodes/edges in refs so handleNodeDeleteById and onPushSnapshot
  can't capture stale pre-drop state (fixes undo-stack correctness).
- Clamp context-menu x/y to viewport so right-click near edges stays
  fully on-screen.
- Drop non-conformant role=menu/menuitem from the single-item context
  menu; rely on the native button for accessibility.
- Extend isInputTarget() to cover ARIA combobox/textbox/searchbox so
  Backspace in Radix/shadcn widgets never nukes a node.
- Extract handleBuilderKeydown as a pure function and add tests
  covering the Delete/Backspace + isInputTarget invariant.
- Remove issue-number references from code comments per CLAUDE.md.
- Document the new delete affordances in the Workflow Builder docs.
- Inline context-menu dismissal, rename pointer handler, drop unused
  deps in keyboardActions useMemo.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
(cherry picked from commit d7f36b2)

* fix(workflows): make archon-adversarial-dev sed replacement macOS-safe (coleam00#1155)

* fix(workflows): make adversarial init sed portable on macOS

* chore: regenerate bundled-defaults after adversarial-dev sed fix

Sync generated bundle with the new temp-file sed pattern in
archon-adversarial-dev.yaml so check:bundled passes and binary
distributions ship the macOS-safe version.

---------

Co-authored-by: laplace young <yangqk12@whu.edu.cn>
Co-authored-by: Rasmus Widing <rasmus.widing@gmail.com>
(cherry picked from commit 817186d)

* fix(deps): override transitive axios to ^1.15.0 for CVE-2025-62718 (coleam00#1330)

axios <1.15.0 can be coerced to bypass NO_PROXY rules via hostname
normalization, enabling SSRF in the right network shape. Archon pulls
axios transitively through @slack/bolt (^1.12.0) and @slack/web-api
(^1.13.5); before this change bun.lock resolved axios@1.13.6 — within
the vulnerable range.

Adding "axios": "^1.15.0" to the root package.json overrides bumps the
transitive resolution to axios@1.15.1 (latest compatible 1.x). Both
Slack range specs accept it without API surface changes — no downstream
code touches axios directly.

Supersedes coleam00#1153. Credits @stefans71 for identifying and reporting the
vulnerability; their PR was stale on the lockfile (0.3.5 → 0.3.6 drift
on dev), so this is a fresh one-line re-do on current dev.

Closes coleam00#1053.

Co-authored-by: Stefans71 <stefans71@users.noreply.github.com>
(cherry picked from commit ae2d936)

* fix(cli): surface stale-workspace registration error instead of fake "not a git repo" (coleam00#1332)

* fix(cli): surface stale-workspace registration error instead of fake "not a git repo"

When workflowRunCommand auto-registers an unregistered repo, a stale
~/.archon/workspaces/<owner>/<repo>/source symlink (pointing to an old
checkout) causes createProjectSourceSymlink() in @archon/paths to throw:

  Source symlink at <linkPath> already points to <existing>, expected <target>

The CLI caught that in a try/catch, logged it at warn level, continued
with `codebase = null`, and then the isolation / resume branches hit
their "codebase missing" fallback and threw the generic:

  Cannot create worktree: not in a git repository.

That message is false — the repo is valid; the Archon workspace entry
is stale. It sends users down the wrong diagnostic path (checking git
config, permissions, etc.) instead of pointing at the workspace dir.

Fix: preserve the registration error on a new `codebaseRegistrationError`
local, and at both fallback sites (resume + worktree-creation) check it
before the generic "not a git repo" branch. When set, throw a truthful:

  Cannot {create worktree,resume}: repository registration failed.
  Error: <original message>
  Hint: Remove the stale workspace entry at <dir> and retry, or
        use --no-worktree to skip isolation.

The hint's exact path comes from a small parser that extracts the
workspace directory from the known "Source symlink at …" format; when
the message shape doesn't match (future error text changes), the parser
returns null and we fall back to a generic "check registration under
<archon-home>/workspaces" hint — safe degradation.

Regression test in workflow.test.ts asserts the new error message and
negatively asserts the old "not in a git repository" string is gone.

Supersedes coleam00#1157 — that PR was draft + CONFLICTING against current dev,
and also mentioned Windows test-compat changes that weren't in the diff
(pruned scope). This is a fresh re-do focused strictly on coleam00#1146.

Closes coleam00#1146.

Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>

* review: add resume-path test, null-fallback test, update troubleshooting docs

Addresses multi-agent review feedback on this PR:

- Add regression test for the --resume fallback site (the worktree-create
  site was already covered; the resume site had identical wiring but zero
  test coverage).
- Add test for the unrecognized-error-shape branch of
  buildRegistrationFailureError so the generic workspace hint is pinned
  (prevents accidental inversion of the stale-entry vs generic-hint
  ternary).
- Update the troubleshooting page to key on the new
  "Cannot create worktree: repository registration failed." message.
  Users hitting the new error won't find the page under the old heading,
  and the "In the future..." note is obsolete now that the error itself
  contains the cleanup path.
- Trim both new docblocks: keep the load-bearing cross-package error
  string contract in extractStaleWorkspaceEntry, drop narration of what
  the code already shows. Drop the "Before this helper existed..."
  paragraph from buildRegistrationFailureError — that's CHANGELOG
  material. Drop PR-reference suffix from the test section divider.

* review: guard getArchonHome in hint + export parser for direct tests

Two follow-up fixes to the multi-agent review commit (f32f002):

CodeRabbit finding — unguarded getArchonHome() in the fallback hint.
If getArchonHome() ever throws (misconfigured env vars, permission issues
on the resolution path), the registration-failure Error would never get
constructed: we'd throw a secondary home-resolution error that masks the
root cause. Wrap the fallback branch in try/catch — prefer losing the
exact path in the hint over replacing the actionable registration error.
A safe generic hint ("Check your Archon workspace registration and retry")
takes over when getArchonHome() throws. The original error.message is
always embedded verbatim in the re-thrown Error.

S2 — export extractStaleWorkspaceEntry for direct table tests. The parser
is where the cross-package string contract with @archon/paths actually
lives; direct tests against it are cheaper than end-to-end CLI tests and
pin the edge cases:

- POSIX path with forward slashes (typical unix user)
- Windows path with backslashes (verifies Math.max(lastIndexOf / , lastIndexOf \))
- Unrelated error message (no prefix) → null
- Prefix matches but delimiter missing → null
- Source path without any separator → null (guards against returning
  empty string, which would produce a nonsense "Remove the stale
  workspace entry at " hint)
- Empty string → null

Six new cases in the test file. The claim of Windows support in the
PR description is now actually verified.

* fix(test): make generic-hint assertion path-separator agnostic

Windows test runner (CI) hit:
  Expected to contain: "Check your Archon workspace registration under /home/test/.archon/workspaces"
  Received: "... under \home\test\.archon\workspaces and retry, ..."

path.join normalizes to `\` on Windows and `/` on POSIX. The test hardcoded
forward slashes in the expected substring. Split into two separator-agnostic
asserts: the prefix up to "under", then `/workspaces\b/` regex for the final
path segment. Behavior doesn't change — the hint still gets the full
path.join'd workspaces dir on either platform.

---------

Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
(cherry picked from commit 056707d)

* fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog (coleam00#1329)

* fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog

Fixes three tightly-coupled bugs that made web approval gates unusable:

1. orchestrator-agent did not pass parentConversationId to executeWorkflow
   for any web-dispatched foreground / interactive / resumable run. Without
   that field, findResumableRunByParentConversation (the machinery the CLI
   relies on for resume) couldn't find the paused run from the same
   conversation on a follow-up message, and the approve/reject API handlers
   had no conversation to dispatch back to.

2. POST /api/workflows/runs/:runId/{approve,reject} recorded the decision
   and returned "Send a message to continue the workflow." — the workflow
   never actually resumed. Added tryAutoResumeAfterGate() that mirrors what
   workflowApproveCommand / workflowRejectCommand already do on the CLI:
   look up the parent conversation, dispatch `/workflow run <name>
   <userMessage>` back through dispatchToOrchestrator. Failures are
   non-fatal — the user can still send a manual message as a fallback.

3. The during-streaming cancel-check in dag-executor aborted any streaming
   node whenever the run status left 'running', including the legitimate
   transition to 'paused' that an approval node performs. A concurrent AI
   node in the same DAG layer now tolerates 'paused' and finishes its own
   stream; only truly terminal / unknown states (null, cancelled, failed,
   completed) abort the in-flight stream.

Web UI: ConfirmRunActionDialog gains an optional reasonInput prop (label +
placeholder) that renders a textarea and passes the trimmed value to
onConfirm. WorkflowRunCard (dashboard) and WorkflowProgressCard (chat)
both use it for Reject now — the chat card was still on window.confirm,
which was both inconsistent with the dashboard and couldn't collect a
reason. The trimmed reason threads through to $REJECTION_REASON in the
workflow's on_reject prompt.

Supersedes coleam00#1147. @jonasvanderhaegen surfaced the root cause and shape of
the fix; that PR was 87 commits stale and pre-dated the reject-UX upgrade
(coleam00#1261 area), so this is a fresh re-do on current dev.

Tests:
- packages/server/src/routes/api.workflow-runs.test.ts — 5 new cases:
  approve with parent dispatches; approve without parent returns "Send a
  message"; approve with deleted parent conversation skips safely; reject
  dispatches on-reject flows; reject that cancels (no on_reject) does NOT
  dispatch.
- packages/core/src/orchestrator/orchestrator.test.ts — updated the two
  synthesizedPrompt-dispatch tests for the new executeWorkflow arity.

Closes coleam00#1131.

Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>

* fix: address multi-agent review findings for web approval auto-resume

C1 (critical) — cross-adapter misrouting guard
  tryAutoResumeAfterGate now checks parentConv.platform_type === 'web'
  before dispatching. Non-web parents (Slack/Telegram/GitHub/Discord)
  being approved from the dashboard skip auto-resume rather than
  dispatching a Slack thread_ts or Telegram chat_id through the web
  adapter's lock manager.

C2 (critical) — fire-and-forget dispatch replaced with await
  void dispatchToOrchestrator() meant the "Resuming workflow." response
  fired before async work completed, and the outer try/catch couldn't
  observe dispatch failures. Changed to await; response now accurately
  reflects dispatch outcome.

I1 — replaced logPrefix string-template (which produced 3-segment
  api.workflow_*.dispatched event names violating {domain}.{action}_{state})
  with literal event names per action, branched inside the helper.
  Accepts action: 'approve' | 'reject' instead.

I2 — corrected misleading "foreground/interactive" qualifier in the
  approve-endpoint comment; background web dispatches also set
  parent_conversation_id via the pre-created run, so they auto-resume too.

I3 — extracted shouldContinueStreamingForStatus() as a small exported
  policy and added 7 unit tests covering running/paused/null/cancelled/
  failed/completed/unknown. Full-integration coverage of the paused-
  tolerance invariant would require manipulating the 10s
  CANCEL_CHECK_INTERVAL_MS, which is flaky-prone; unit test of the
  policy function captures the same invariant deterministically.

I4 — updated approval-nodes.md and authoring-workflows.md to reflect
  that Web UI approve/reject now auto-resumes (no "send a follow-up
  message" copy), documented the reject-with-reason dialog and
  $REJECTION_REASON flow, and called out the cross-platform caveat.

S1 — rewrote streaming status check as positive shouldContinue safe-list
  via the extracted policy function, matching the inline comment.

S2 — inlined handleReject on the dashboard rather than squeezing
  rejectWorkflowRun through runAction with a closure; keeps runAction
  narrow for the single-arg lifecycle actions.

S5 — new regression test covering the non-web-parent skip path
  (slack-platform parent → dispatch skipped → response falls back to
  "Send a message to continue").

S6 — removed stale reference to runAction in ConfirmRunActionDialog's
  onConfirm JSDoc (no longer accurate now that WorkflowProgressCard
  calls the dialog without runAction).

S7 — fixed misleading "user can resume manually by sending any message"
  docstring (resume is triggered by re-running the workflow command,
  not by an arbitrary message).

Skipped as out-of-scope:
  S3 — cancelWorkflowRun rowCount check (pre-existing defect; separate PR)
  S4 — tightening expect.anything() to UUID regex (deferred)
  S8 — 12-positional-arg executeWorkflow → options-bag refactor
    (tracked follow-up)

bun run validate green locally; 68 tests in api.workflow-runs.test.ts
(up from 67), 173 in dag-executor.test.ts (up from 166).

* review: close I1/I2/I3/I4/I6 — paused tolerance in loop + emitter, resume test, useId

I1 (loop inter-iteration check) — dag-executor.ts:1715
  Used `!== 'running'` in the loop node's between-iteration status check.
  A sibling approval node pausing the run in the same topological layer
  would abort the loop mid-iteration with "Loop node '<id>' stopped at
  iteration N (paused)". Switched to the shared shouldContinueStreamingForStatus
  helper so paused is tolerated — same semantics the streaming check got.
  Extended inline comment explains the sibling-layer concurrency reason.

I2 (skipIfStatusChanged emitter unregister) — dag-executor.ts:2886
  At DAG-finalization writes the helper correctly skipped writing on any
  non-running state (paused included — don't mark a paused run complete),
  but it *also* called getWorkflowEventEmitter().unregisterRun() which
  broke SSE observability for a run that's still live (waiting for user
  approval). Split the two responsibilities: skip the write for all
  non-running states, but only unregister the emitter for terminal states
  (cancelled / deleted / completed / failed). `paused` keeps the emitter
  registered so resume stays visible on the dashboard.

I3 (foreground_resume_detected branch untested) — orchestrator-agent.test.ts
  That branch was modified as part of the original fix (added
  parentConversationId as 11th positional arg) but no existing test
  configured mockFindResumableRunByParentConversation to return non-null.
  A positional mistake (e.g. accidentally swapping issueContext and
  parentConversationId) would silently break auto-resume with no failing
  test. New regression test configures the mock, asserts both the cwd
  comes from the resumable run's working_path AND parentConversationId
  is passed correctly at position 10.

I4 (null-parent log level) — api.ts tryAutoResumeAfterGate
  `getConversationById` returning null is a data-integrity signal (the
  parent conversation was deleted while the run was paused) — worth
  surfacing at info level so operators notice, not hiding at debug.
  Missing platform_conversation_id on an existing row would be an unusual
  DB state and stays at debug. Added `parentDeleted: boolean` to the log
  context so the two cases are distinguishable in observability.

I6 (hardcoded DOM id) — ConfirmRunActionDialog.tsx
  `id="confirm-run-action-reason"` collided when multiple dialog instances
  share the same page (Radix portals mitigate in practice but the code
  was fragile). Switched to React.useId() so each instance gets a unique
  id — htmlFor/id wiring preserved.

S11 (arity-only assertion) — orchestrator-agent.test.ts:1092 area
  The interactive-workflow-on-web test asserted mockExecuteWorkflow was
  called, but nothing about the args. Added a specific assertion that
  position 10 (parentConversationId) equals 'conv-1' (the caller
  conversation id) — pins the wiring that I1/I2 depend on being correct.

Deferred (from review S1-S10, I5, I7):
  - S1 (ExecuteWorkflowOptions bag) — tracked as standalone follow-up;
    12 positional args with 2 adjacent optionals is a real maintenance
    hazard but the refactor deserves its own PR.
  - S7 (WHY comment on non-web else branch) — review text says the branch
    "correctly omits" parentConversationId but the code passes it; the
    combination with the web-parent guard in tryAutoResumeAfterGate is
    intentional. Not adding a justify-what-we-don't-do comment.
  - S2/S3/S4/S5/S8/S9/S10 — pure polish (event-map ternary, platformConvId
    inlining, shared constant for REJECTION_REASON_INPUT, onChange arrow
    shorthand, discriminated union, docblock trim, suffix comment drop)
  - I5 (soften "Resuming workflow." to "— check the dashboard for progress")
    — users clicking from the dashboard are already on the dashboard; the
    current text is accurate (enqueue completed) and concise.
  - I7 (test dispatch-throws path) — covered implicitly by the try/catch
    branch of tryAutoResumeAfterGate returning false; a direct test would
    require mocking handleMessage to throw and would couple to
    dispatchToOrchestrator internals.

bun run validate green; 189 dag-executor tests, 98 orchestrator-agent
tests, 68 api.workflow-runs tests — all the new cases pass.

---------

Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>
(cherry picked from commit d5c1cd9)

* feat(providers): autodetect canonical binary install paths for Claude and Codex (coleam00#1361)

Both binary resolvers previously stopped at env-var + explicit config and
threw a "not found" error when neither was set. Users who followed the
upstream-recommended install flow (Anthropic's `curl install.sh` for
Claude, `npm install -g @openai/codex`) still had to manually set either
`CLAUDE_BIN_PATH` / `CODEX_BIN_PATH` or the corresponding config field
before any workflow could run.

Add a tier-N autodetect step between the explicit config tier and the
install-instructions throw. Purely additive: env and config still win
when set (precedence covered by new tests). On autodetect miss, the same
install-instructions error fires as before.

Claude probe list (verified against docs.claude.com "Uninstall Claude
Code → Native installation" section):
  - $HOME/.local/bin/claude            (mac/linux native installer)
  - $USERPROFILE\.local\bin\claude.exe (Windows native installer)

Codex probe list (verified against openai/codex README; npm global-
install puts the binary at `{npm_prefix}/bin/<name>` on POSIX,
`{npm_prefix}\<name>.cmd` on Windows):
  - $HOME/.npm-global/bin/codex   (user-set `npm config set prefix`)
  - /opt/homebrew/bin/codex       (mac arm64 with homebrew-node)
  - /usr/local/bin/codex          (mac intel / linux system node)
  - %APPDATA%\npm\codex.cmd       (Windows npm global default)
  - $HOME\.npm-global\codex.cmd   (Windows user-set prefix)

Not probed (explicit override still required):
  - Custom npm prefixes — `npm root -g` would need a subprocess per
    resolve, too much surface for a probe helper
  - `brew install --cask codex` — cask layout isn't a PATH binary
  - Manual GitHub Releases extracts — placement is user-determined
  - `~/.bun/bin/codex` — not documented in openai/codex README

Pi provider intentionally has no equivalent change: the Pi SDK is
bundled into the archon binary (no subprocess), so there's no "binary"
to resolve. Pi auth lives at `~/.pi/agent/auth.json` which the SDK
already finds by default, and the PR A shim (`PI_PACKAGE_DIR`) handles
the package-dir case via Pi's own documented escape hatch.

E2E verified: removed both config entries from ~/.archon/config.yaml,
rebuilt compiled binary, ran `archon workflow run archon-assist` and a
Codex workflow. Logs showed `source: 'autodetect'` for both, responses
returned cleanly.

(cherry picked from commit b99cee4)

* fix(providers/test): use os.homedir() instead of $HOME in claude binary autodetect test

The native-installer autodetect test computed its expected path from
process.env.HOME, but the implementation uses node:os homedir(). On
Windows, HOME is typically unset (Windows uses USERPROFILE), so the
test fell back to '/Users/test' while the resolver returned the real
home dir — making the spy's path-equality check fail and breaking CI
on windows-latest.

Mirror the implementation by importing homedir() from node:os and
joining with node:path so the expected path matches the actual
platform-resolved home and separator.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit f9f8775)

* fix(server): contain Discord login failure so it doesn't kill the server (coleam00#1365)

Reported in coleam00#1365: a user running `archon serve` with DISCORD_BOT_TOKEN
set but the "Message Content Intent" toggle disabled in the Discord
Developer Portal saw the entire server crash with `Used disallowed
intents`. Discord rejects the gateway connection (close code 4014) when
a privileged intent is requested without being enabled, and the
unguarded `await discord.start()` propagated the error all the way up,
taking the web UI down with it.

Wrap discord.start() in try/catch — log the failure with an actionable
hint (special-cased for the disallowed-intent error) and continue
running. Other adapters and the web UI come up regardless. The shutdown
handler already uses optional chaining (`discord?.stop()`) so nulling
discord after a failed start is safe.

Other adapters (Telegram, Slack, GitHub, Gitea, GitLab) have the same
unguarded-start pattern but are out of scope for this fix — addressing
them is tracked separately.

Also expanded the Discord setup docs with a caution callout that names
the exact error string and the new log event so users can grep for
both.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit 5957c6e)

---------

Co-authored-by: Cole Medin <cole@dynamous.ai>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Kagura <kagura.chen28@gmail.com>
Co-authored-by: Rasmus Widing <152263317+Wirasm@users.noreply.github.com>
Co-authored-by: Shay Elmualem <12733941+norbinsh@users.noreply.github.com>
Co-authored-by: Lior Franko <lior.franko@ironsrc.com>
Co-authored-by: Lior Franko <liorfr@dreamgroup.com>
Co-authored-by: Alex Siri <alexsiri7@gmail.com>
Co-authored-by: Ahmed <44034059+medevs@users.noreply.github.com>
Co-authored-by: CauchYoung <2024302072042@whu.edu.cn>
Co-authored-by: laplace young <yangqk12@whu.edu.cn>
Co-authored-by: Rasmus Widing <rasmus.widing@gmail.com>
Co-authored-by: Stefans71 <stefans71@users.noreply.github.com>
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interactive workflows cannot resume after approval gate (parent_conversation_id is NULL)

1 participant