Skip to content

fix(mothership): mothership-ran workflows show workflow validation errors#3634

Open
TheodoreSpeaks wants to merge 6 commits intostagingfrom
fix/mothership-wf-validation-error
Open

fix(mothership): mothership-ran workflows show workflow validation errors#3634
TheodoreSpeaks wants to merge 6 commits intostagingfrom
fix/mothership-wf-validation-error

Conversation

@TheodoreSpeaks
Copy link
Collaborator

Summary

Mothership ran workflows didn't show validation failure. Routed the error message so it also adds to console.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Other: ___________

Testing

  • Validated mothership ran workflows show workflow validation error in logs and pops up.

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)

Screenshots/Videos

image

@vercel
Copy link

vercel bot commented Mar 17, 2026

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

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Mar 18, 2026 0:35am

Request Review

@cursor
Copy link

cursor bot commented Mar 17, 2026

PR Summary

Medium Risk
Touches shared workflow execution streaming and console error/cancellation handling used by both UI and copilot/mothership paths; regressions could hide logs or misclassify failures, but changes are localized to client-side reporting.

Overview
Ensures mothership/copilot-triggered workflow runs surface pre-execution failures (validation and HTTP startup errors) in the terminal console, not just block-level errors.

Refactors execution-level console logging into shared helpers in workflow-execution-utils (timing, error, cancelled, and new HTTP-status-based entries) and updates use-workflow-execution to reuse those helpers.

Unifies SSE parsing by exporting processSSEStream from use-execution-stream and using it in executeWorkflowWithFullLogging, while also propagating httpStatus on fetch failures and honoring X-Execution-Id for correct execution tracking/log association.

Written by Cursor Bugbot for commit 5751bc2. This will update automatically on new commits. Configure here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes a bug where workflow validation errors from mothership-triggered runs were silently swallowed and never surfaced to the developer console. The change is isolated to workflow-execution-utils.ts and touches two distinct error paths.

Key changes:

  • Critical reference fix: accumulatedBlockLogs was previously passed as an inline [] literal to createBlockEventHandlers, meaning the outer scope always saw an empty array. Extracting it to a named const before passing it by reference ensures block logs accumulate correctly and the isPreExecutionError classification actually works.
  • HTTP-level error logging: When the initial fetch returns a non-200 response (the mothership execution path), an error ConsoleEntry is now added before re-throwing, so the failure is visible in the terminal.
  • SSE execution:error logging: The execution:error stream event now adds a console entry (with blockType: 'validation' for pre-execution/validation failures, 'error' for runtime errors) and calls cancelRunningEntries to tidy up any in-flight entries.
  • Timeout differentiation: A string-match heuristic (timed out) is used to assign a distinct blockId/blockName for timeout errors versus generic execution errors.

Confidence Score: 4/5

  • This PR is safe to merge; it adds missing console logging for error paths with no risk of breaking existing happy-path execution.
  • The change is well-scoped to error handling code paths that previously had no console output at all, so any regression is bounded to error display rather than execution correctness. The critical accumulatedBlockLogs reference fix is correct and necessary. Minor style concerns exist around the inconsistent blockType between the HTTP and SSE error paths, and the use of Number.MAX_SAFE_INTEGER as a sentinel executionOrder, but neither affects runtime correctness.
  • No files require special attention beyond the two inline suggestions left on workflow-execution-utils.ts.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-execution-utils.ts Adds console error entries for both HTTP-level failures (!response.ok) and SSE-streamed execution:error events; also fixes a critical reference bug where accumulatedBlockLogs was passed as an inline literal (losing shared reference), and adds cancelRunningEntries call on error.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[executeWorkflowWithFullLogging called] --> B[fetch /api/workflows/:id/execute]
    B --> C{response.ok?}

    C -- No --> D[Parse error JSON]
    D --> E[addConsole: blockId='execution-error', blockType='error']
    E --> F[throw new Error]

    C -- Yes --> G[Read SSE stream]
    G --> H{event.type}

    H -- execution:started --> I[setCurrentExecutionId]
    H -- block:started / completed / error --> J[blockHandlers\n accumulatedBlockLogs updated]
    H -- execution:completed --> K[Set executionResult success]
    H -- execution:error --> L[cancelRunningEntries]

    L --> M{accumulatedBlockLogs\n.length === 0?}
    M -- Yes isPreExecutionError --> N[addConsole\nblockId='validation'\nblockType='validation']
    M -- No --> O{any log has error?}
    O -- No hasBlockError=false --> P{error includes\n'timed out'?}
    P -- Yes --> Q[addConsole\nblockId='timeout-error'\nblockType='error']
    P -- No --> R[addConsole\nblockId='execution-error'\nblockType='error']
    O -- Yes --> S[Skip: block-level\nerror already logged]
Loading

Last reviewed commit: 6caa79b

error: errorMessage,
durationMs: event.data.duration || 0,
startedAt: now,
executionOrder: isPreExecutionError ? 0 : Number.MAX_SAFE_INTEGER,
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Number.MAX_SAFE_INTEGER as sentinel executionOrder

Using Number.MAX_SAFE_INTEGER (9,007,199,254,740,991) as the executionOrder for a non-pre-execution error entry is a rough sentinel that could cause unexpected sorting behavior if any downstream consumer adds to, compares, or serialises this value. A named constant or a derivation from the actual accumulated log count would be safer and clearer:

Suggested change
executionOrder: isPreExecutionError ? 0 : Number.MAX_SAFE_INTEGER,
executionOrder: accumulatedBlockLogs.length,

This naturally places the error entry after all completed block logs without relying on an extreme sentinel value.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

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