refactor(core,editor): split logic based on payload type / manual execution type #22219
+402
−227
Conversation
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
… nodes without trigger, upgrading to full executions if the destination node's branch does not have enough run data
…root node has runData
❌ 19 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
n8n-assistant
bot
added
core
|
Found 31 test failures on Blacksmith runners: Failures
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 9 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="packages/core/src/execution-engine/partial-execution-utils/directed-graph.ts">
<violation number="1" location="packages/core/src/execution-engine/partial-execution-utils/directed-graph.ts:522">
Rule violated: **Prefer Typeguards over Type casting**
`outputType` is still cast with `as NodeConnectionType`, bypassing any verification of the connection type. Please replace the cast with a proper narrowing strategy (e.g., add a type guard that checks the key against `NodeConnectionTypes` before using it) so the compiler enforces valid connection types instead of relying on `as`.</violation>
</file>
<file name="packages/cli/src/workflows/workflow-execution.service.ts">
<violation number="1" location="packages/cli/src/workflows/workflow-execution.service.ts:150">
Rule violated: **Prefer Typeguards over Type casting**
The payload is cast to `any` just to delete `runData`, bypassing the manual execution typings. Use the existing type guard (or refine the union type) so the deletion happens on a properly narrowed type instead of an `as any` cast, per the Prefer Typeguards over Type casting rule.</violation>
<violation number="2" location="packages/cli/src/workflows/workflow-execution.service.ts:153">
Remove the raw payload console log (or replace it with a redacted logger message) to avoid leaking manual execution data and to respect the project’s logging conventions.</violation>
<violation number="3" location="packages/cli/src/workflows/workflow-execution.service.ts:295">
Await the manual execution run so the method returns the real execution ID instead of a pending Promise.</violation>
</file>
<file name="packages/core/src/execution-engine/workflow-execute.ts">
<violation number="1" location="packages/core/src/execution-engine/workflow-execute.ts:211">
Removing the standalone-node partial execution path now causes manual runs of nodes without parents to throw an assertion instead of executing, regressing a supported scenario.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| from, | ||
| to, | ||
| // TODO: parse outputType instead of casting it | ||
| type: outputType as NodeConnectionType, |
Contributor
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
outputType is still cast with as NodeConnectionType, bypassing any verification of the connection type. Please replace the cast with a proper narrowing strategy (e.g., add a type guard that checks the key against NodeConnectionTypes before using it) so the compiler enforces valid connection types instead of relying on as.
Prompt for AI agents
Address the following comment on packages/core/src/execution-engine/partial-execution-utils/directed-graph.ts at line 522:
<comment>`outputType` is still cast with `as NodeConnectionType`, bypassing any verification of the connection type. Please replace the cast with a proper narrowing strategy (e.g., add a type guard that checks the key against `NodeConnectionTypes` before using it) so the compiler enforces valid connection types instead of relying on `as`.</comment>
<file context>
@@ -493,6 +493,44 @@ export class DirectedGraph {
+ from,
+ to,
+ // TODO: parse outputType instead of casting it
+ type: outputType as NodeConnectionType,
+ outputIndex,
+ inputIndex,
</file context>
| // TODO: fix this on the FE | ||
| if ('triggerToStartFrom' in payload) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access | ||
| delete (payload as any).runData; |
Contributor
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
The payload is cast to any just to delete runData, bypassing the manual execution typings. Use the existing type guard (or refine the union type) so the deletion happens on a properly narrowed type instead of an as any cast, per the Prefer Typeguards over Type casting rule.
Prompt for AI agents
Address the following comment on packages/cli/src/workflows/workflow-execution.service.ts at line 150:
<comment>The payload is cast to `any` just to delete `runData`, bypassing the manual execution typings. Use the existing type guard (or refine the union type) so the deletion happens on a properly narrowed type instead of an `as any` cast, per the Prefer Typeguards over Type casting rule.</comment>
<file context>
@@ -91,149 +93,225 @@ export class WorkflowExecutionService {
+ // TODO: fix this on the FE
+ if ('triggerToStartFrom' in payload) {
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
+ delete (payload as any).runData;
+ }
+
</file context>
| }); | ||
|
|
||
| return this.processRunExecutionData(graph.toWorkflow({ ...workflow })); | ||
| assert.fail('this code should be dead'); |
Contributor
There was a problem hiding this comment.
Removing the standalone-node partial execution path now causes manual runs of nodes without parents to throw an assertion instead of executing, regressing a supported scenario.
Prompt for AI agents
Address the following comment on packages/core/src/execution-engine/workflow-execute.ts at line 211:
<comment>Removing the standalone-node partial execution path now causes manual runs of nodes without parents to throw an assertion instead of executing, regressing a supported scenario.</comment>
<file context>
@@ -205,37 +205,39 @@ export class WorkflowExecute {
- });
-
- return this.processRunExecutionData(graph.toWorkflow({ ...workflow }));
+ assert.fail('this code should be dead');
+ // // short cut here, only create a subgraph and the stacks
+ // graph = findSubgraph({
</file context>
| delete (payload as any).runData; | ||
| } | ||
|
|
||
| console.log('payload', payload); |
Contributor
There was a problem hiding this comment.
Remove the raw payload console log (or replace it with a redacted logger message) to avoid leaking manual execution data and to respect the project’s logging conventions.
Prompt for AI agents
Address the following comment on packages/cli/src/workflows/workflow-execution.service.ts at line 153:
<comment>Remove the raw payload console log (or replace it with a redacted logger message) to avoid leaking manual execution data and to respect the project’s logging conventions.</comment>
<file context>
@@ -91,149 +93,225 @@ export class WorkflowExecutionService {
+ delete (payload as any).runData;
+ }
+
+ console.log('payload', payload);
+
+ if (isPartialManualExecutionToDestination(payload)) {
</file context>
| console.log('pinnedTrigger', pinnedTrigger); | ||
| } | ||
|
|
||
| const executionId = this.workflowRunner.run({ |
Contributor
There was a problem hiding this comment.
Await the manual execution run so the method returns the real execution ID instead of a pending Promise.
Prompt for AI agents
Address the following comment on packages/cli/src/workflows/workflow-execution.service.ts at line 295:
<comment>Await the manual execution run so the method returns the real execution ID instead of a pending Promise.</comment>
<file context>
@@ -91,149 +93,225 @@ export class WorkflowExecutionService {
+ console.log('pinnedTrigger', pinnedTrigger);
+ }
+
+ const executionId = this.workflowRunner.run({
+ executionMode: 'manual',
+ pinData: payload.workflowData.pinData,
</file context>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
WIP
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)