fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog#1329
Merged
fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog#1329
Conversation
📝 WalkthroughWalkthroughThreads 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
Sequence DiagramsequenceDiagram
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)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 | 🟠 MajorThread
issueContextthrough instead of hard-codingundefined.
handleWorkflowInvocationResultreceivesissueContext, andexecuteWorkflowhas a dedicatedissueContextparameter. Passingundefinedhere 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-resumeAlso pass
issueContextfromhandleWorkflowInvocationResult(...)intodispatchOrchestratorWorkflow(...), 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 fixesconversation.idpropagation 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
📒 Files selected for processing (11)
CHANGELOG.mdpackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator.test.tspackages/server/src/routes/api.tspackages/server/src/routes/api.workflow-runs.test.tspackages/web/src/components/chat/WorkflowProgressCard.tsxpackages/web/src/components/dashboard/ConfirmRunActionDialog.tsxpackages/web/src/components/dashboard/WorkflowRunCard.tsxpackages/web/src/components/dashboard/WorkflowRunGroup.tsxpackages/web/src/routes/DashboardPage.tsxpackages/workflows/src/dag-executor.ts
Collaborator
Author
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 FIXESCore logic is sound — Critical / Important
Suggestions
Strengths
Recommended merge path
Generated by /prp-review-agents. Each finding was verified against the actual diff/code before inclusion; agent false-positives were dropped. |
…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
force-pushed
the
fix/web-approval-gate-auto-resume
branch
from
3b16dd6 to
6704ead
Compare
Collaborator
Author
|
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)
Important (4/4)
Suggestions (5 accepted, 3 deferred)
`bun run validate` green locally; CI should go green on the rebase. |
There was a problem hiding this comment.
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 | 🟡 MinorOnly collect a rejection reason when
on_rejectexists.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'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 callingshouldContinueStreamingForStatusduring streaming and these tests would still pass. Please add a deterministic test wheregetWorkflowRunStatus()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
platformConvIdand resume message (/workflow run deploy Deploy feature X) lock in the contract nicely.Two optional tightenings you might consider:
- For the "parent deleted" case (lines 1438–1456), also assert
mockGetConversationByIdwas called with'deleted-conv-uuid'so regressions that skip the lookup entirely (e.g. accidentally short-circuiting onrun.parent_conversation_id) are caught.- Add a sibling case where
getConversationByIdreturns a conversation with an empty stringplatform_conversation_id— currently only thenullparent is exercised for theskippedNoPlatformConvpath.🤖 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 intryAutoResumeAfterGateand 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
📒 Files selected for processing (14)
CHANGELOG.mdpackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator.test.tspackages/docs-web/src/content/docs/guides/approval-nodes.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/server/src/routes/api.tspackages/server/src/routes/api.workflow-runs.test.tspackages/web/src/components/chat/WorkflowProgressCard.tsxpackages/web/src/components/dashboard/ConfirmRunActionDialog.tsxpackages/web/src/components/dashboard/WorkflowRunCard.tsxpackages/web/src/components/dashboard/WorkflowRunGroup.tsxpackages/web/src/routes/DashboardPage.tsxpackages/workflows/src/dag-executor.test.tspackages/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
Collaborator
Author
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)
Suggestions (11)
Strengths
Documentation
VerdictNEEDS 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 Recommended Actions
|
…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.
5 tasks
Collaborator
Author
|
Pushed `c002d4c8` addressing the multi-agent review. Closed
Deferred (tracked in #1350)Opened a dedicated follow-up issue covering the polish and ergonomics items that don't block this merge:
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
`bun run validate` green locally; 189 dag-executor tests, 98 orchestrator-agent tests, 68 api.workflow-runs tests. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/orchestrator/orchestrator-agent.test.ts (1)
1102-1138: LGTM — regression coverage aligns withexecuteWorkflowsignature.Positional index assertions (
callArgs[10],callArgs[3]) correctly map toparentConversationIdandcwdper theexecuteWorkflowsignature inpackages/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
executeWorkflowargs are reordered. Once the deferred options-bag refactor forexecuteWorkflowlands, 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
📒 Files selected for processing (4)
packages/core/src/orchestrator/orchestrator-agent.test.tspackages/server/src/routes/api.tspackages/web/src/components/dashboard/ConfirmRunActionDialog.tsxpackages/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
6 tasks
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)
parentConversationIdwas never set on web-dispatched runs.orchestrator-agent.tscallsexecuteWorkflowat 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./approveand/rejectdidn'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.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):
`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
Out of scope
Closes #1131.
Co-authored-by: Jonas Vanderhaegen 7755555+jonasvanderhaegen@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation