Skip to content

fix(mothership): run workflow tools (run from block, run until block)#3595

Merged
Sg312 merged 2 commits intostagingfrom
fix/mothership-2
Mar 15, 2026
Merged

fix(mothership): run workflow tools (run from block, run until block)#3595
Sg312 merged 2 commits intostagingfrom
fix/mothership-2

Conversation

@Sg312
Copy link
Collaborator

@Sg312 Sg312 commented Mar 15, 2026

Summary

Fixes run workflow tools
Fixes stream reconnect bug

Type of Change

  • Bug fix

Testing

Manual

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 15, 2026 1:54am

Request Review

@Sg312 Sg312 merged commit aad620c into staging Mar 15, 2026
11 checks passed
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 15, 2026

Greptile Summary

This PR fixes two related issues in the Mothership/Copilot feature: broken workflow execution tools (run_workflow, run_from_block, run_workflow_until_block, run_block) and a stream reconnect bug when returning to an in-progress chat.

Key changes:

  • handlers.ts: Removes the CLIENT_WORKFLOW_TOOLS special-case in the interactive === false path that was calling waitForToolCompletion. These tools now fall through to fireToolExecution() like any other server-side tool. The concern here is whether the browser's use-chat.ts still fires executeRunToolOnClient for the same event (see inline comment).
  • use-chat.ts: Overhauled stream reconnect — instead of a separate useEffect watching activeStreamId that blindly re-fetched from from=0, the reconnect now happens inside the chatHistory effect using a snapshot of buffered events piped into a combined ReadableStream, falling back to a live SSE tail for incomplete streams. processSSEStreamRef and finalizeRef refs are synced each render via useLayoutEffect to avoid stale closures. A new isReconnecting state gates a skeleton overlay during the reconnect phase.
  • workflow-execution-utils.ts: Separates JSON.parse from the event switch so a malformed line no longer silently drops the entire batch; converts execution:error from throw to a returned failure result, which is compatible with the existing caller in run-tool-execution.ts.
  • route.ts / tasks.ts: The GET /api/copilot/chat response now includes a streamSnapshot (buffered events + status) when a conversationId is present, eliminating an extra round-trip during reconnect.

Confidence Score: 3/5

  • Merge with caution — the core fixes look directionally correct but the workflow tool execution path needs verification to rule out duplicate server+client execution.
  • The stream reconnect rework is well-structured and the execution:error fix is clean. The main uncertainty is whether removing CLIENT_WORKFLOW_TOOLS from the interactive === false handler causes both fireToolExecution() (server-side) and executeRunToolOnClient (browser-side) to fire for the same tool call, which would trigger two concurrent workflow executions. Additionally, the isReconnecting skeleton condition change in home.tsx drops the !hasMessages guard, potentially flashing a skeleton over already-loaded messages.
  • Pay close attention to apps/sim/lib/copilot/orchestrator/sse/handlers/handlers.ts (dual-execution risk) and apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts (reconnect state management).

Important Files Changed

Filename Overview
apps/sim/lib/copilot/orchestrator/sse/handlers/handlers.ts Removes the CLIENT_WORKFLOW_TOOLS special-case for interactive === false, eliminating the waitForToolCompletion blocking path; workflow tools now execute server-side via fireToolExecution(). This is the core fix, but may trigger duplicate execution if the browser still receives the forwarded SSE event with ui.clientExecutable = true.
apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Significant rework of stream reconnect logic: moved from a standalone useEffect watching activeStreamId to inline reconnect inside the chatHistory effect, using processSSEStreamRef/finalizeRef to avoid stale closures. Adds isReconnecting state and lazy assistant-message creation in flush(). The skeleton condition change in home.tsx (consuming isReconnecting) may flash over existing messages.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-execution-utils.ts Refactors SSE parsing: separates JSON.parse into its own try/catch so parse errors skip only the current event (not the entire event handler). Converts execution:error from throwing an exception to returning a { success: false } result object; this is compatible with the existing callers in run-tool-execution.ts.
apps/sim/app/api/copilot/chat/route.ts Adds stream snapshot (events + status) to the GET /api/copilot/chat response when chat.conversationId is set. Errors are caught and logged without blocking the main response, and the snapshot is only included if it successfully loaded.
apps/sim/hooks/queries/tasks.ts Adds StreamSnapshot interface and optional streamSnapshot field to TaskChatHistory; maps chat.streamSnapshot from the API response in fetchChatHistory. Clean, additive change.
apps/sim/app/workspace/[workspaceId]/home/home.tsx Exposes isReconnecting from useChat and updates the loading skeleton condition to also show during reconnection. The removal of the !hasMessages guard may cause the skeleton to briefly replace already-rendered messages during a reconnect.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant NextAPI as Next.js API
    participant Orchestrator as Orchestrator (handlers.ts)
    participant Go as Go Backend

    Note over Browser,Go: Normal send-message flow
    Browser->>NextAPI: POST /api/copilot/chat
    NextAPI->>Go: Forward message + open SSE stream
    Go-->>Orchestrator: SSE: tool_call (run_workflow, clientExecutable=true)
    
    alt interactive === false (NEW behavior)
        Orchestrator->>Orchestrator: fireToolExecution() → SIM_WORKFLOW_TOOL_HANDLERS
        Orchestrator->>NextAPI: Execute workflow server-side
        Orchestrator-->>Go: markToolComplete (result)
    end
    
    Go-->>NextAPI: SSE forwarded to browser stream
    NextAPI-->>Browser: SSE: tool_call (ui.clientExecutable=true)
    Browser->>Browser: executeRunToolOnClient() ← still triggered!
    Browser->>NextAPI: POST /api/workflows/{id}/execute (client-side)
    Browser->>NextAPI: POST /api/copilot/confirm (reportCompletion → Redis)
    Note over NextAPI: Nobody reads Redis result in new code

    Note over Browser,Go: Stream reconnect flow (NEW)
    Browser->>NextAPI: GET /api/copilot/chat?chatId=X
    NextAPI->>NextAPI: getStreamMeta + readStreamEvents (parallel)
    NextAPI-->>Browser: chat + streamSnapshot{events[], status}
    
    alt snapshot exists & not expired
        Browser->>Browser: Pipe batch events into ReadableStream
        Browser->>NextAPI: GET /api/copilot/chat/stream?from=lastEventId
        NextAPI-->>Browser: Live SSE tail
        Browser->>Browser: processSSEStream(combinedStream)
    else snapshot expired (status=unknown, 0 events)
        Browser->>NextAPI: POST /api/mothership/chat/stop (cleanup)
    end
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/copilot/orchestrator/sse/handlers/handlers.ts, line 316-321 (link)

    Potential double execution of workflow tools

    By removing the CLIENT_WORKFLOW_TOOLS guard in the interactive === false path, the server now calls fireToolExecution()executeToolAndReportexecuteToolServerSideSIM_WORKFLOW_TOOL_HANDLERS.run_workflow (etc.) directly. However, the same SSE tool_call event is forwarded to the browser, where use-chat.ts still checks ui?.clientExecutable and calls executeRunToolOnClient for run_workflow, run_from_block, etc. This means both paths can execute the workflow concurrently — one server-side API call and one client-side API call — resulting in two separate execution runs against the same workflow.

    If ui.clientExecutable is guaranteed to be false when options.interactive === false, this isn't an issue; but if the Go backend sends ui.clientExecutable: true in both modes, the duplicate execution should be explicitly guarded against. It's worth confirming that the Go backend does not set clientExecutable: true on tool events in the non-interactive path, or that isToolAvailableOnSimSide should return false for these tools in that context.

Last reviewed commit: 34283b5

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.

1 participant