fix(cli): send workflow dispatch/result messages for Web UI cards#1052
fix(cli): send workflow dispatch/result messages for Web UI cards#1052
Conversation
) CLI-launched workflows were visible in the Web UI chat but displayed as plain text only — no WorkflowProgressCard or WorkflowResultCard. The CLI adapter already handled both metadata fields; the sendMessage calls were simply missing from workflowRunCommand. Changes: - Send workflowDispatch message before executeWorkflow (mirrors orchestrator.ts) - Send workflowResult message after successful completion with summary - Wrap result message in try/catch with warn log (same pattern as orchestrator) Fixes #1017 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSends CLI-emitted workflow metadata messages: a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Adapter as CLI Adapter
participant DB as Messages DB
participant Executor as Workflow Executor
participant UI as Web UI
CLI->>Adapter: sendMessage(dispatch_status, {workflowDispatch})
Adapter->>DB: persist message (category: workflow_dispatch_status)
DB-->>Adapter: success / failure
Adapter-->>CLI: resolved (errors logged as warnings)
CLI->>Executor: executeWorkflow(...)
Executor-->>CLI: result { summary?, workflowRunId, status }
alt result.summary exists and not paused
CLI->>Adapter: sendMessage(result.summary, {workflowResult})
Adapter->>DB: persist message (category: workflow_result)
DB-->>Adapter: success / failure
Adapter-->>CLI: resolved (errors logged as warnings)
DB->>UI: new message -> render WorkflowResultCard
else no summary or paused
CLI->>CLI: print completion / paused message (no result card sent)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
🔍 Comprehensive PR ReviewPR: #1052 (fixes issue #1017) SummaryMinimal, well-scoped fix (+22 lines, 1 file) that adds two Verdict:
🟡 Medium Issues (Needs Decision)M1: Unguarded Dispatch
|
| Issue | Location | Agent | Note |
|---|---|---|---|
| Dispatch not wrapped in try/catch (informational) | workflow.ts:595 |
code-review | Consistent with orchestrator; deliberate fail-fast. No action needed. |
surfaceError as Error cast |
workflow.ts:630 |
error-handling | Pervasive codebase pattern, not introduced by this PR. Leave as-is. |
| Paused-check guard equivalence untested | workflow.ts:619-623 |
test-coverage | Low risk — else if structure is obvious. Optional test only. |
| Result-card comment could note paused exclusion | workflow.ts:622 |
comment-quality | Very minor. Optional. |
✅ What's Good
- Correct TypeScript union narrowing:
'summary' in result && result.summaryproperly handles thesummary?: stringoptional field where direct access produces TS2339 in the union context. - Result card error handling is correct: try/catch correctly treats UI surfacing as non-critical; workflow success still reported to console even if DB write fails.
- Logger pattern correct:
getLog().warn(...)matches the module's lazy-initialized logger pattern. workerConversationIdsemantics correct: Same ID is correct since CLI has no separate worker conversation.- Minimal diff: Exactly two
sendMessagecalls — no new abstractions, interfaces, or config. Pure YAGNI. - CLAUDE.md compliance: No
anytypes, fail-fast for dispatch, warn-level logging for non-critical result failure. - CLIAdapter unit tests:
cli-adapter.test.ts:122-155already verifies both metadata shapes. - Consistent event naming:
workflow_output_surface_failedmatches the orchestrator's identical log event name.
📋 Suggested Follow-up Issue
"Add workflowRunCommand integration tests for dispatch/result sendMessage calls" (P2)
Covers: dispatch fires before executeWorkflow, result card with summary, result card without summary, result card DB error suppression.
Next Steps
- ⚡ Easy wins before merge: M1 (4-line error wrap) + M4+M5 (2-line comment updates each)
- 📝 Decide on M2+M3: Fix tests now, or create a follow-up issue
- ✅ No action needed for LOW findings
Reviewed by Archon comprehensive-pr-review workflow (4 specialized agents)
Artifacts: ~/.archon/workspaces/coleam00/Archon/artifacts/runs/477c30179cdbaf6d4cfc4dd2df6b8399/review/
…for PR #1052 - Wrap dispatch sendMessage in try/catch (matches result card pattern) to prevent UI notification failures from blocking workflow execution - Update dispatch comment to accurately describe structural similarity to orchestrator while noting synchronous CLI semantics and that workerConversationId === conversationId in the CLI path - Add note to result card comment about paused-path exclusion - Add 4 integration tests for workflowRunCommand: dispatch ordering and metadata shape, result card with summary, no result card without summary, and non-throwing DB failure on result persist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (6 total)
View all fixes
Tests Added
Skipped (3)
Validation✅ Type check | ✅ Lint | ✅ Tests (77 @archon/cli + all other packages passed) Self-fix by Archon · aggressive mode · fixes pushed to |
Fix Report: PR #1052 Review FindingsAll 5 MEDIUM findings from the consolidated review have been addressed in commit Fixes Applied
Skipped (LOW, no action needed)
Validation |
Template literals automatically coerce numbers to strings; wrapping with String() is redundant. Removed from formatAge, formatDuration, and all console.log calls in workflow.ts. Also compacted a two-line object spread in workflowStatusCommand to a single line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deduplicate JSON branch in workflowStatusCommand by computing the output array once with a single console.log call, removing the duplicated verbose/non-verbose conditional branches. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comprehensive PR ReviewPR: #1052 — fix(cli): send workflow dispatch/result messages for Web UI cards SummaryClean, well-scoped PR that adds two Verdict:
LOW IssuesOnly 1 of the 4 findings is actionable — the rest explicitly recommend no change:
View details for Finding 1 (log naming fix)The two new log events use flat names instead of the // Current
'workflow_dispatch_surface_failed'
'workflow_output_surface_failed'
// Should be
'cli.workflow_dispatch_surface_failed'
'cli.workflow_result_surface_failed'What's Good
Next Steps
Reviewed by Archon comprehensive-pr-review workflow |
Apply review finding: rename flat log event names to use the
cli.{action}_{state} convention matching the rest of the file.
- workflow_dispatch_surface_failed → cli.workflow_dispatch_surface_failed
- workflow_output_surface_failed → cli.workflow_result_surface_failed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Auto-Fix ReportStatus: COMPLETE SummaryNo CRITICAL or HIGH issues found by the review. Applied 1 actionable LOW fix.
What Was Fixed
LOW Issues (No Change Needed)These were reviewed and explicitly accepted as-is:
Validation
Auto-fixed by Archon comprehensive-pr-review workflow |
![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
🤖 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/cli/src/commands/workflow.test.ts`:
- Around line 998-1008: The test sets persistent mock implementations on
messagesDb.addMessage and executeWorkflow using mockImplementation(), causing
subsequent tests to inherit behavior; change both to use
mockImplementationOnce() so the custom behaviors apply only to the next call
(i.e., replace the mockImplementation(...) on messagesDb.addMessage and the one
on executeWorkflow with mockImplementationOnce(...) to avoid cross-test
pollution).
🪄 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: bd76f045-b5a9-4163-92a1-8ff9a4bb7511
📒 Files selected for processing (2)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
| (messagesDb.addMessage as ReturnType<typeof mock>).mockImplementation( | ||
| async (_dbId: unknown, role: unknown, content: unknown) => { | ||
| if (role === 'assistant') { | ||
| callOrder.push(`addMessage:${String(content)}`); | ||
| } | ||
| } | ||
| ); | ||
| (executeWorkflow as ReturnType<typeof mock>).mockImplementation(async () => { | ||
| callOrder.push('executeWorkflow'); | ||
| return { success: true, workflowRunId: 'run-1' }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file around the specified lines
cd packages/cli/src/commands
wc -l workflow.test.tsRepository: coleam00/Archon
Length of output: 80
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '980,1020p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 1988
🏁 Script executed:
# Look for how these mocks are initially set up - search for messagesDb and executeWorkflow mocking
rg -n "mock.module.*messagesDb|spyOn.*messagesDb" packages/cli/src/commands/workflow.test.ts | head -20Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Check for executeWorkflow mock setup
rg -n "mock.module.*executeWorkflow|spyOn.*executeWorkflow" packages/cli/src/commands/workflow.test.ts | head -20Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Look at the broader context of the test to understand test structure and reset patterns
sed -n '950,1050p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 4585
🏁 Script executed:
# Look at the top of the test file to see how mocking is set up
head -100 packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 3235
🏁 Script executed:
# Search for mock.module() calls in the file
rg -n "mock\.module\(" packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 746
🏁 Script executed:
# Look for beforeEach/afterEach patterns
rg -n "beforeEach|afterEach" packages/cli/src/commands/workflow.test.ts | head -10Repository: coleam00/Archon
Length of output: 371
🏁 Script executed:
# Look at the beforeEach/afterEach patterns
sed -n '148,160p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 503
🏁 Script executed:
# Also check other beforeEach/afterEach blocks
sed -n '344,355p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 441
🏁 Script executed:
# Look at the full mock.module for messages to understand the initial state
sed -n '124,128p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 208
🏁 Script executed:
# Check more context around lines 980-1050 to see all tests in this block
sed -n '920,1120p' packages/cli/src/commands/workflow.test.ts | head -150Repository: coleam00/Archon
Length of output: 6924
🏁 Script executed:
# Check if there's any reset/clearing of these mocks between tests
sed -n '1060,1110p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 2506
🏁 Script executed:
# Look for any patterns where tests depend on mock state
rg -n "mockClear|mockRestore" packages/cli/src/commands/workflow.test.ts | grep -A 2 -B 2 "messagesDb\|executeWorkflow"Repository: coleam00/Archon
Length of output: 354
🏁 Script executed:
# Look at the full test structure around line 998 to see the complete test case
sed -n '987,1045p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 2780
🏁 Script executed:
# Check if the next test properly resets or has its own implementations
sed -n '1045,1120p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 3524
🏁 Script executed:
# Verify exactly what mockClear does vs what's needed - check if there's any reset of executeWorkflow in test 2
sed -n '1049,1075p' packages/cli/src/commands/workflow.test.tsRepository: coleam00/Archon
Length of output: 1279
Use mockImplementationOnce() to prevent test isolation violations.
The mockImplementation() calls at lines 998–1008 persist beyond this test case. Since mockClear() in subsequent tests only clears call history but preserves the implementation, later tests inherit these persistent behaviors and become order-dependent. Replace both with mockImplementationOnce() to ensure each mock applies only to the next call.
Suggested change
- (messagesDb.addMessage as ReturnType<typeof mock>).mockImplementation(
+ (messagesDb.addMessage as ReturnType<typeof mock>).mockImplementationOnce(
async (_dbId: unknown, role: unknown, content: unknown) => {
if (role === 'assistant') {
callOrder.push(`addMessage:${String(content)}`);
}
}
);
- (executeWorkflow as ReturnType<typeof mock>).mockImplementation(async () => {
+ (executeWorkflow as ReturnType<typeof mock>).mockImplementationOnce(async () => {
callOrder.push('executeWorkflow');
return { success: true, workflowRunId: 'run-1' };
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (messagesDb.addMessage as ReturnType<typeof mock>).mockImplementation( | |
| async (_dbId: unknown, role: unknown, content: unknown) => { | |
| if (role === 'assistant') { | |
| callOrder.push(`addMessage:${String(content)}`); | |
| } | |
| } | |
| ); | |
| (executeWorkflow as ReturnType<typeof mock>).mockImplementation(async () => { | |
| callOrder.push('executeWorkflow'); | |
| return { success: true, workflowRunId: 'run-1' }; | |
| }); | |
| (messagesDb.addMessage as ReturnType<typeof mock>).mockImplementationOnce( | |
| async (_dbId: unknown, role: unknown, content: unknown) => { | |
| if (role === 'assistant') { | |
| callOrder.push(`addMessage:${String(content)}`); | |
| } | |
| } | |
| ); | |
| (executeWorkflow as ReturnType<typeof mock>).mockImplementationOnce(async () => { | |
| callOrder.push('executeWorkflow'); | |
| return { success: true, workflowRunId: 'run-1' }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/workflow.test.ts` around lines 998 - 1008, The test
sets persistent mock implementations on messagesDb.addMessage and
executeWorkflow using mockImplementation(), causing subsequent tests to inherit
behavior; change both to use mockImplementationOnce() so the custom behaviors
apply only to the next call (i.e., replace the mockImplementation(...) on
messagesDb.addMessage and the one on executeWorkflow with
mockImplementationOnce(...) to avoid cross-test pollution).
Prevents "Cannot read properties of undefined (reading 'status')" crash when navigating between chat and workflow execution views during race conditions where run data may be transiently undefined. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…1052 - Fix half-applied optional chaining in WorkflowProgressCard refetchInterval (query.state.data?.run.status → ?.run?.status) preventing TypeError in polling - Add dispatch-failure test verifying executeWorkflow still runs when dispatch sendMessage fails - Add paused-workflow test proving paused guard fires before summary check - Strengthen dispatch metadata assertion to verify workerConversationId format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…for PR coleam00#1052 - Wrap dispatch sendMessage in try/catch (matches result card pattern) to prevent UI notification failures from blocking workflow execution - Update dispatch comment to accurately describe structural similarity to orchestrator while noting synchronous CLI semantics and that workerConversationId === conversationId in the CLI path - Add note to result card comment about paused-path exclusion - Add 4 integration tests for workflowRunCommand: dispatch ordering and metadata shape, result card with summary, no result card without summary, and non-throwing DB failure on result persist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oleam00#1052 - Fix half-applied optional chaining in WorkflowProgressCard refetchInterval (query.state.data?.run.status → ?.run?.status) preventing TypeError in polling - Add dispatch-failure test verifying executeWorkflow still runs when dispatch sendMessage fails - Add paused-workflow test proving paused guard fires before summary check - Strengthen dispatch metadata assertion to verify workerConversationId format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…for PR coleam00#1052 - Wrap dispatch sendMessage in try/catch (matches result card pattern) to prevent UI notification failures from blocking workflow execution - Update dispatch comment to accurately describe structural similarity to orchestrator while noting synchronous CLI semantics and that workerConversationId === conversationId in the CLI path - Add note to result card comment about paused-path exclusion - Add 4 integration tests for workflowRunCommand: dispatch ordering and metadata shape, result card with summary, no result card without summary, and non-throwing DB failure on result persist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oleam00#1052 - Fix half-applied optional chaining in WorkflowProgressCard refetchInterval (query.state.data?.run.status → ?.run?.status) preventing TypeError in polling - Add dispatch-failure test verifying executeWorkflow still runs when dispatch sendMessage fails - Add paused-workflow test proving paused guard fires before summary check - Strengthen dispatch metadata assertion to verify workerConversationId format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-issue-1775831868291 fix(cli): send workflow dispatch/result messages for Web UI cards
1 participant
Summary
WorkflowProgressCardduring execution, noWorkflowResultCardafter completion, and no "View full logs" link.sendMessage()calls inpackages/cli/src/commands/workflow.ts— one beforeexecuteWorkflow(dispatch) and one after (result) — mirroring the pattern inorchestrator.ts:dispatchBackgroundWorkflow.cli-adapter.ts,MessageList.tsx, the DAG executor, and all other packages are untouched. The CLI adapter already handled both metadata fields; it just needed to be called.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
workflow.tscli-adapter.sendMessage()cli-adapter.tsmessagestableworkflowDispatch/workflowResultMessageList.tsxWorkflowProgressCardMessageList.tsxWorkflowResultCardLabel Snapshot
risk: lowsize: XSclicli:workflow-commandChange Metadata
bugcliLinked Issue
Validation Evidence (required)
'summary' in result && result.summarybecause TypeScript could not narrow the union type in theelse if (result.success)branch with a direct property access.getLog().warn(...)instead of barelogto match the lazy-initialized logger pattern used byworkflowRunCommand.Security Impact (required)
Compatibility / Migration
Human Verification (required)
else if (result.success && !paused)branch); missingresult.summaryis guarded before callingsendMessagebun run cli workflow runwith the Web UI open (automated validation only)Side Effects / Blast Radius (required)
cli-adapter.sendMessage()already handles both metadata fields and persists to the same DB table used by the web pathsendMessagewould surface as a logged error; the dispatch/result messages are non-critical (workflow execution continues regardless)Rollback Plan (required)
sendMessage()calls inpackages/cli/src/commands/workflow.ts; no DB migration neededRisks and Mitigations
sendMessagethrows after workflow completes, masking successwarnlog (mirroringorchestrator.ts:380-389); CLIconsole.log('completed')still runsSummary by CodeRabbit
New Features
Tests
Improvements