feat: consolidates node runners to accept TaskNode with a new CreateNodeExecutionStep, replaces LoopProcessor with shared loop helpers, respects per-node isSimulated, enhances summarization #430
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
* fix: respect isSimulated config for ContractWrite operations
Previously, all ContractWrite operations were hardcoded to use Tenderly
simulation (isSimulated=true) regardless of the node config. This change
fixes the logic to:
1. Default to simulation (true) for safety
2. Respect the is_simulated config flag when explicitly set
3. Allow real UserOp execution when isSimulated=false
This enables workflows to control whether each ContractWrite node should
be simulated (Tenderly) or executed for real (via bundler).
Fixes issue where approve_token1 node with isSimulated:false was still
being simulated.
* refactor: remove redundant nodeValue parameter from runX functions
- Simplified runBalance() to only take taskNode parameter
- Node value is now extracted inside the function using taskNode.GetBalance()
- Updated all call sites to remove the redundant third parameter
- Prevents mismatches between taskNode and nodeValue
- Makes taskNode the single source of truth for node configuration
This refactoring:
- Reduces parameter count and API surface
- Makes it impossible to pass mismatched taskNode/nodeValue pairs
- Improves code clarity by having one source of truth
- Maintains full backward compatibility with existing tests
All tests passing after refactoring.
* refactor: remove redundant stepID parameter from runX functions
All runX functions (runRestApi, runGraphQL, runContractRead, runContractWrite,
runCustomCode, runBranch, runLoop, runFilter, runEthTransfer, runBalance)
previously took both stepID and taskNode parameters, but stepID was always
equal to taskNode.Id.
Changes:
- Updated all runX function signatures to take only taskNode parameter
- Each function now extracts stepID from taskNode.Id internally
- Updated all call sites in executeNode, executeNodeDirect, and executeNodeWithIsolatedVars
- Updated all test files with the new function signatures
This eliminates redundancy and simplifies the API while maintaining identical functionality.
* fix: set TaskNode on BranchProcessor in email summary tests
The branch email summary tests were failing because they didn't set
the TaskNode on the processor before calling Execute(). After the
refactoring that changed createNodeExecutionStep to use GetTaskNode()
instead of an explicit nodeType parameter, the processor needs to have
the TaskNode set to get the correct node type.
This fix adds TaskNode wrappers in both test functions and sets them
on the processor using SetTaskNode() before execution.
* docs: document SetTaskNode requirement in processor Execute methods
Added documentation to GraphQLQueryProcessor and BranchProcessor Execute
methods to clarify that SetTaskNode() must be called before Execute() to
ensure proper node type identification in the execution step.
This addresses the potential nil pointer dereference concern raised by
Copilot. While CreateNodeExecutionStep handles nil gracefully (resulting
in NODE_TYPE_UNKNOWN instead of panic), proper initialization via
SetTaskNode() is required for correct execution step type identification.
In normal VM execution flow, SetTaskNode() is always called by the runX
methods before invoking Execute(). This documentation helps developers
who might call processor Execute() methods directly (e.g., in tests).
* fix: clarify runLoop uses queue-based execution, not LoopProcessor
Addressed Copilot's concern about runLoop not using the processor pattern.
The reason is architectural: runLoop uses executeLoopWithQueue (newer
implementation with worker pool execution queue) instead of LoopProcessor
(older implementation with goroutines). This is intentional for better
performance and resource management.
Added comments to clarify this is by design, not an oversight. The
executeLoopWithQueue method is the preferred approach and is consistently
used across executeNodeDirect and executeNodeWithIsolatedVars as well.
* refactor: remove LoopProcessor and extract shared loop helpers
- Remove vm_runner_loop.go (835 lines) and vm_runner_loop_test.go (505 lines)
- Create loop_helpers.go with 7 shared helper functions (322 lines)
- Enhance utils.go with type conversion utilities (+170 lines)
- Clean vm.go by removing duplicate methods (-160 lines)
- Net reduction: 1,748 lines of code
Loop execution is now integrated directly in VM via executeLoopWithQueue(),
eliminating the need for a separate vm_runner file. All functionality preserved
with zero regressions.
Key improvements:
- Single source of truth for loop execution
- No code duplication
- Better architecture (worker pools, no unnecessary processor wrapper)
- All unit tests passing
- Comprehensive documentation in docs/refactoring-loop-processor.md
* fix: use execution-time template processing for loop iterations
The previous refactoring introduced a bug where template variables with dot
notation (e.g. {{value.spender}}) weren't being resolved in loop iterations.
The issue was that template substitution was being done at node creation time
using a simple string replacement that didn't support dot notation. The old
LoopProcessor passed iteration variables to RunNodeWithInputs, which used the
VM's preprocessTextWithVariableMapping that supports dot notation.
This fix removes the premature template substitution from createNestedNodeFromLoop
and lets the execution-time template processing handle it. The iteration variables
are passed to executeNodeDirectWithVars, which makes them available to the
processors' normal template processing that supports dot notation.
Fixes: TestLoopNode_ContractWrite_Approve_PerIterationData
* Update core/taskengine/loop_helpers.go
Co-authored-by: Copilot <[email protected]>
---------
Co-authored-by: Chris Li <[email protected]>
Co-authored-by: Copilot <[email protected]>
Previously, all ContractWrite operations were hardcoded to use Tenderly simulation (isSimulated=true) regardless of the node config. This change fixes the logic to: 1. Default to simulation (true) for safety 2. Respect the is_simulated config flag when explicitly set 3. Allow real UserOp execution when isSimulated=false This enables workflows to control whether each ContractWrite node should be simulated (Tenderly) or executed for real (via bundler). Fixes issue where approve_token1 node with isSimulated:false was still being simulated.
- Simplified runBalance() to only take taskNode parameter - Node value is now extracted inside the function using taskNode.GetBalance() - Updated all call sites to remove the redundant third parameter - Prevents mismatches between taskNode and nodeValue - Makes taskNode the single source of truth for node configuration This refactoring: - Reduces parameter count and API surface - Makes it impossible to pass mismatched taskNode/nodeValue pairs - Improves code clarity by having one source of truth - Maintains full backward compatibility with existing tests All tests passing after refactoring.
All runX functions (runRestApi, runGraphQL, runContractRead, runContractWrite, runCustomCode, runBranch, runLoop, runFilter, runEthTransfer, runBalance) previously took both stepID and taskNode parameters, but stepID was always equal to taskNode.Id. Changes: - Updated all runX function signatures to take only taskNode parameter - Each function now extracts stepID from taskNode.Id internally - Updated all call sites in executeNode, executeNodeDirect, and executeNodeWithIsolatedVars - Updated all test files with the new function signatures This eliminates redundancy and simplifies the API while maintaining identical functionality.
Addressed Copilot's concern about runLoop not using the processor pattern. The reason is architectural: runLoop uses executeLoopWithQueue (newer implementation with worker pool execution queue) instead of LoopProcessor (older implementation with goroutines). This is intentional for better performance and resource management. Added comments to clarify this is by design, not an oversight. The executeLoopWithQueue method is the preferred approach and is consistently used across executeNodeDirect and executeNodeWithIsolatedVars as well.
- Remove vm_runner_loop.go (835 lines) and vm_runner_loop_test.go (505 lines) - Create loop_helpers.go with 7 shared helper functions (322 lines) - Enhance utils.go with type conversion utilities (+170 lines) - Clean vm.go by removing duplicate methods (-160 lines) - Net reduction: 1,748 lines of code Loop execution is now integrated directly in VM via executeLoopWithQueue(), eliminating the need for a separate vm_runner file. All functionality preserved with zero regressions. Key improvements: - Single source of truth for loop execution - No code duplication - Better architecture (worker pools, no unnecessary processor wrapper) - All unit tests passing - Comprehensive documentation in docs/refactoring-loop-processor.md
The previous refactoring introduced a bug where template variables with dot
notation (e.g. {{value.spender}}) weren't being resolved in loop iterations.
The issue was that template substitution was being done at node creation time
using a simple string replacement that didn't support dot notation. The old
LoopProcessor passed iteration variables to RunNodeWithInputs, which used the
VM's preprocessTextWithVariableMapping that supports dot notation.
This fix removes the premature template substitution from createNestedNodeFromLoop
and lets the execution-time template processing handle it. The iteration variables
are passed to executeNodeDirectWithVars, which makes them available to the
processors' normal template processing that supports dot notation.
Fixes: TestLoopNode_ContractWrite_Approve_PerIterationData
Co-authored-by: Copilot <[email protected]>
- Move test config loading from init() to lazy loading to prevent loading aggregator-sepolia.yaml during production runs - Add better error messages for ECDSA key decryption failures - Configure different ports for multi-chain aggregators (sepolia:2206, base-sepolia:2207, ethereum:3206, base:3207)
The runNodeWithInputs RPC was incorrectly showing isSimulated:true in the executionContext even when the request specified isSimulated:false. Root causes: 1. RunNodeImmediatelyRPC didn't extract and pass the isSimulated flag from nodeConfig to RunNodeImmediately(), causing it to default to simulation mode (true) 2. ExtractNodeConfiguration() didn't include the isSimulated field when extracting ContractWrite node configuration Changes: - Extract isSimulated from node config and pass to RunNodeImmediately() - Add isSimulated field extraction to ExtractNodeConfiguration() - Add comprehensive unit test to verify the fix The simulation mode now correctly depends solely on the node input as documented in the task engine architecture.
- Remove verbose execution log from error field - Error now contains only the first sentence summary - Detailed logs remain in the log field where they belong
- Create common validation utility (template_validation.go) for reuse - Add ValidateTemplateVariableResolution() for individual values - Add ValidateResolvedParams() for parameter arrays - Add extractFailedVariables() helper function Apply validation to all node types with template variables: - ContractWrite: methodParams (before and after JSON array expansion) - ContractRead: methodParams - ETHTransfer: destination, amount - GraphQL: url, query, variables - CustomCode: source - Balance: address, chain, tokenAddresses - REST API: url, body, headers Improve error messages: - Remove redundant 'parameter N' index - Remove '=undefined' suffix - Simplify help text - Example: 'could not resolve variable get_quote.data.amountOut in method exactInputSingle. Make sure to run the get_quote node first' Add comprehensive tests: - Unit tests for validation utilities - Integration test for missing node dependencies This ensures users must provide all required node outputs in inputVariables when using runNodeImmediately, maintaining data flow integrity in both simulation and real execution modes.
When a Tenderly simulation reverts, the EVM still computes intermediate return values that exist in the return data buffer. Previously, these garbage values were being decoded and included in the response even when success=false. Now when simulation.status or transaction.status is false, we clear result.ReturnData to ensure no misleading data is returned to the user.
…ull due to protobuf limitation
…athParams and fix ETH transfer success flag handling
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 17. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
…d improve summary generation
- Update test templates to use direct variable access ({{user.id}}) instead of trigger.* syntax for RunNodeImmediately calls, since inputVariables are added directly to VM
- Skip template variable validation for immediate executions (RunNodeImmediately) to allow undefined values gracefully
- Include workflow name and step names in summary body fallback when no contract writes are recorded
- Only skip validation when VM has TaskNodes but no task (indicating RunNodeImmediately) - Test VMs have no task and no TaskNodes, so validation still runs correctly - Fixes TestValidateTemplateVariableResolution and TestValidateResolvedParams failures
chrisli30
changed the title
chrisli30
changed the title
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.
This pull request introduces significant improvements to the loop execution and node processing logic in the task engine, with a focus on template variable handling, node configuration, and debugging. The changes include a new helper module for loop operations, enhancements to trigger variable accessibility, improved simulation flag handling for contract write nodes, and expanded debug logging for troubleshooting. Additionally, there are minor adjustments to token metadata and contract read execution for consistency.
Loop execution and template variable handling:
loop_helpers.gowith helper functions for loop execution, including template variable substitution, input variable resolution, and nested node creation, ensuring robust and flexible handling of loop iterations and template variables in node configurations.headersandpathParamsdirectly in manual trigger data, enabling template access such asManualTrigger.pathParams.endpoint.Node execution and simulation logic:
CreateNodeExecutionStep) to use the actualTaskNode, allowing dynamic determination of node type and simulation flags (including support for explicitis_simulatedconfiguration in contract write nodes). [1] [2]Debugging and logging enhancements:
Other improvements:
Idinstead ofAddressfor better consistency inGetTokenMetadata.TaskNodefor consistency with real node execution.Test enhancements:
TaskNodeand set them in the processor, ensuring tests reflect the latest execution logic. [1] [2]