fix(twenty-server): honor per-step continueOnFailure outside of iterators - #24111
fix(twenty-server): honor per-step continueOnFailure outside of iterators#24111suparikoli wants to merge 1 commit into
Conversation
|
👋 Thanks for contributing to Twenty! We're excited to have you on board. Your PR has been set to draft while you work on it. Once you're done, mark it as Ready for review and our automated checks will run. By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement. |
Greptile SummaryThis PR makes the workflow executor honor each step's existing
Confidence Score: 5/5The PR appears safe to merge, with the new behavior confined to failed steps that explicitly opt into continuation. The executor still follows its existing failure path by default, while explicitly opted-in failures are converted to the already-supported failed-safely state and allowed to continue. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Execute workflow step] --> B{Action returned an error?}
B -->|No| C[Persist successful result]
B -->|Yes| D{Enclosing iterator continues failures?}
D -->|Yes| F[Set shouldFailSafely]
D -->|No| E{Step continueOnFailure enabled?}
E -->|Yes| F
E -->|No| G[Persist failed step and fail workflow]
F --> H[Persist FAILED_SAFELY]
H --> I[Continue to eligible next steps]
Reviews (1): Last reviewed commit: "fix(twenty-server): honor per-step conti..." | Re-trigger Greptile |
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts">
<violation number="1" location="packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts:155">
P2: The new per-step `continueOnFailure` check also applies to child steps inside an iterator loop, whereas the PR scopes it to non-iterator steps. A failing child step inside an iterator now sets `shouldFailSafely` even when the enclosing iterator has `shouldContinueOnIterationFailure: false`, so the step resumes its own next step and the run records FAILED_SAFE instead of FAILED. Confirm this in-loop behavior is intended; if not, gate `stepHasContinueOnFailure` to steps not inside a loop, and add a test covering the iterator interaction.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (isDefined(enclosingIterator)) { | ||
| if ( | ||
| isDefined(enclosingIterator) || | ||
| stepHasContinueOnFailure(stepToExecute) |
There was a problem hiding this comment.
P2: The new per-step continueOnFailure check also applies to child steps inside an iterator loop, whereas the PR scopes it to non-iterator steps. A failing child step inside an iterator now sets shouldFailSafely even when the enclosing iterator has shouldContinueOnIterationFailure: false, so the step resumes its own next step and the run records FAILED_SAFE instead of FAILED. Confirm this in-loop behavior is intended; if not, gate stepHasContinueOnFailure to steps not inside a loop, and add a test covering the iterator interaction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts, line 155:
<comment>The new per-step `continueOnFailure` check also applies to child steps inside an iterator loop, whereas the PR scopes it to non-iterator steps. A failing child step inside an iterator now sets `shouldFailSafely` even when the enclosing iterator has `shouldContinueOnIterationFailure: false`, so the step resumes its own next step and the run records FAILED_SAFE instead of FAILED. Confirm this in-loop behavior is intended; if not, gate `stepHasContinueOnFailure` to steps not inside a loop, and add a test covering the iterator interaction.</comment>
<file context>
@@ -149,7 +150,10 @@ export class WorkflowExecutorWorkspaceService {
- if (isDefined(enclosingIterator)) {
+ if (
+ isDefined(enclosingIterator) ||
+ stepHasContinueOnFailure(stepToExecute)
+ ) {
actionOutput.shouldFailSafely = true;
</file context>
…tors Every workflow step has been initialized with errorHandlingOptions.continueOnFailure.value since that field was added, but the executor never actually read it. The only failure-continuation path implemented was iterator-specific: findEnclosingIteratorWithContinueOnFailure checks a *different* flag (shouldContinueOnIterationFailure) that only rescues a failed step when it's inside an iterator loop. Outside of an iterator, a step's own continueOnFailure was silently ignored - any error (including a pre-execution/input-validation error, not just the action's own logic) always failed the whole run. Verified nothing else in the executable path reads this field: grep -rn "continueOnFailure" packages/twenty-server/src --include="*.ts" only turns up the type definition and default-value initialization (new/prefilled steps, mock fixtures) - never a read in the executor. Change: extracted the check into stepHasContinueOnFailure() (mirrors the existing pattern of small, independently-tested predicates like shouldFailSafely/shouldSkipStepExecution in this same directory), and OR it into the existing condition that already sets actionOutput.shouldFailSafely for the iterator case. This is strictly additive/opt-in: steps default to continueOnFailure: false, so nothing changes for any workflow that doesn't explicitly set it. Verified: - Added step-has-continue-on-failure.util.spec.ts (3 cases: default false, explicitly true, and defensively handles a step missing errorHandlingOptions entirely for older/malformed data) - all pass. - npx nx typecheck twenty-server: clean. - npx oxlint --type-aware and npx oxfmt --check on all three changed files: clean (see follow-up in this PR's checks). Testing gap, disclosed rather than overclaimed: I did not add a test that exercises the full executeFromStep orchestration in WorkflowExecutorWorkspaceService itself. That method has 9 constructor dependencies (billing, message queue, workspace cache, etc.) and no existing spec file in this codebase already covers it directly either - the same pattern I'm following (small tested utility + a one-line, directly-parallel wiring change at the call site) is how the existing iterator continuation check is structured. The one-line change here is mechanically identical in shape to that already-shipped iterator check, which is what gives me confidence in it beyond the unit test. This PR only addresses the backend/executor half of the issue. It does not add a "Continue on failure" toggle to the step settings UI, which the issue also asks for - that's a frontend feature addition I have not attempted or verified. Addresses the backend half of twentyhq#24044
aa95f83 to
9218768
Compare
🔍 Automated Pre-Review✅ No issues detected - This PR is ready for human review. 🧭 External PR Triage Review✅ Looks good — a maintainer can pick this up. Checks
Detailed findings (duplicate candidates, standards notes, summary) are in the workflow run logs. Automated pre-review — human approval still required. |
1 participant
Summary
Addresses the backend half of #24044.
Every workflow step is initialized with
errorHandlingOptions.continueOnFailure.value(seeworkflow-version-step-operations.workspace-service.ts), but the executor never actually reads it. I confirmed this with:which only turns up the type definition and places that set a default value (new/prefilled steps, mock fixtures) — never a place that reads it to make a decision.
The only failure-continuation path that's actually wired up is iterator-specific:
findEnclosingIteratorWithContinueOnFailurechecks a different flag (shouldContinueOnIterationFailure) that only rescues a failed step when it's inside an iterator loop. Outside of an iterator, a step's owncontinueOnFailuresetting is silently ignored — any failure (including a pre-execution/input-validation error, not just the action's own runtime error) always fails the whole workflow run, regardless of what the step's error-handling settings say.Change
Extracted the check into
stepHasContinueOnFailure(), matching this directory's existing pattern of small, independently-tested predicates (shouldFailSafely,shouldSkipStepExecution), and OR'd it into the existing condition inworkflow-executor.workspace-service.tsthat already setsactionOutput.shouldFailSafelyfor the iterator case:This is strictly additive/opt-in: steps default to
continueOnFailure: false, so nothing changes for any workflow that doesn't explicitly set it totrue.Test plan
step-has-continue-on-failure.util.spec.ts: defaultfalse, explicitlytrue, and a step missingerrorHandlingOptionsentirely (defensive case for older/malformed step data) — all 3 pass.should-fail-safely.util.spec.ts,find-enclosing-iterator-with-continue-on-failure.util.spec.ts) to check for regressions — 16/16 still pass, unchanged.npx nx typecheck twenty-server: clean.npx oxlint --type-aware -c .oxlintrc.jsonandnpx oxfmt --checkon all three changed files: clean.Disclosed gap, not overclaimed: I did not write a test that exercises the full
executeFromSteporchestration insideWorkflowExecutorWorkspaceServiceitself. That method has 9 constructor-injected dependencies (billing, message queue, workspace cache, exception handler, etc.) and — as far as I could find — has no existing spec file covering it directly in this codebase either. I followed the same pattern the codebase already uses for the iterator case: a small, well-tested pure predicate, wired in with a one-line change that's mechanically identical in shape to the already-shipped iterator check. I'm confident in the wiring by inspection and by that direct parallel, but it is not covered by an automated integration test.Scope: this PR is backend/executor only. The issue also asks for a "Continue on failure" toggle in the step settings UI (
errorHandlingOptions.continueOnFailurecurrently has no UI control at all, so it can only be set via the API/MCP tools today) — I have not attempted that; it's a frontend feature addition outside what I verified here.🤖 Generated with the assistance of Claude (Opus 5), verified by running the real test suite plus typecheck/lint before opening this PR.