Consolidates node runners to accept TaskNode with a new CreateNodeExecutionStep, replaces LoopProcessor with shared loop helpers, adds robust template variable validation, respects per-node isSimulated, enhances summarization, and renames TokenMetadata address to id across engine, protobufs, and tests. #429
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
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.
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.
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).
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]>
* 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]>
- 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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 17
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Defer simulation/real decision to per-node config (e.g., contractWrite.config.is_simulated) | ||
| result, err := n.RunNodeImmediately(nodeTypeStr, nodeConfig, inputVariables, user) | ||
| // Pass the isSimulated flag from node config to control simulation/real execution | ||
| result, err := n.RunNodeImmediately(nodeTypeStr, nodeConfig, inputVariables, user, useSimulation) |
There was a problem hiding this comment.
Bug
The useSimulation parameter extracted from nodeConfig["isSimulated"] is passed to RunNodeImmediately, but RunNodeImmediately signature uses variadic useSimulation ...interface{}. When passing a single bool value directly as RunNodeImmediately(nodeTypeStr, nodeConfig, inputVariables, user, useSimulation), Go wraps it in an interface slice. However, the extraction logic only checks len(useSimulation) > 0 and attempts type assertion useSimulation[0].(bool). If useSimulation is false, this boolean value is still successfully type-asserted and assigned, but the parameter handling appears designed for optional arguments. The real issue: when useSimulation is explicitly false, it should disable simulation, but the code path may not properly propagate this setting due to how the variadic parameter is being used versus how it's documented/intended.
chrisli30
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This is a comprehensive staging PR that includes multiple fixes and refactoring efforts focused on loop execution, template variable validation, and simulation mode handling. Key changes include:
- Protobuf field rename:
TokenMetadata.address→TokenMetadata.idfor consistency - Major refactoring: Removed LoopProcessor (1,748 lines of duplicate code) and extracted shared loop helpers
- Bug fixes: Template variable validation, isSimulated flag handling, and Tenderly simulation result clearing
- Documentation: Added comprehensive loop refactoring documentation
Reviewed Changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
protobuf/avs.proto |
Renamed TokenMetadata.address to id for consistency with other entities |
core/taskengine/loop_helpers.go |
New file with 322 lines of shared loop helper functions |
core/taskengine/vm.go |
Refactored to remove duplicate loop processing, updated runX functions to accept TaskNode |
core/taskengine/template_validation.go |
New comprehensive template variable validation with helpful error messages |
core/taskengine/node_utils.go |
Updated CreateNodeExecutionStep to respect isSimulated from node config |
core/taskengine/summarizer.go |
Enhanced email summaries with "What Executed Successfully" format |
operator/operator.go |
Improved error messages for ECDSA key decryption failures |
core/testutil/utils.go |
Lazy-loaded test config to avoid production binary overhead |
docs/refactoring-loop-processor.md |
Comprehensive documentation of loop processor removal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Note
Consolidates node runners to accept TaskNode with a new CreateNodeExecutionStep, replaces LoopProcessor with shared loop helpers, adds robust template variable validation, respects per-node isSimulated, enhances summarization, and renames TokenMetadata address to id across engine, protobufs, and tests.
createNodeExecutionStepwithCreateNodeExecutionStep; runners now takeTaskNode(REST, GraphQL, Branch, CustomCode, ContractRead/Write, Filter, ETHTransfer, Balance).RunNodeImmediatelyRPCpassesisSimulatedfrom node config; contract writes default to simulate unless overridden.LoopProcessor; add shared helpers incore/taskengine/loop_helpers.goand rework VMexecuteLoopWithQueueto use them.headers/pathParamsin vars.TokenMetadata.address→id; update engine, service, protobufs, and tests.Written by Cursor Bugbot for commit ee010a8. Configure here.