Skip to content

Conversation

@chrisli30
Copy link
Member

  • fix: respect isSimulated config for ContractWrite operations
  • refactor: remove redundant nodeValue parameter from runX functions

Chris Li added 2 commits November 5, 2025 10:11
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.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the node execution step creation process to improve type safety and consistency. The main change involves updating createNodeExecutionStep to CreateNodeExecutionStep which now accepts a *avsproto.TaskNode parameter instead of just the NodeType, allowing the function to extract node-specific configuration like is_simulated flags.

Key changes:

  • Renamed createNodeExecutionStep to CreateNodeExecutionStep (exported) and changed signature to accept *avsproto.TaskNode
  • Added taskNode field to CommonProcessor with getter/setter methods
  • Updated all node runner methods (runRestApi, runBalance, runContractRead, etc.) to accept a taskNode parameter
  • Modified all processor constructors to properly initialize CommonProcessor with named field vm: vm
  • Updated all test cases to create and pass TaskNode wrappers when calling runner methods

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
core/taskengine/node_utils.go Refactored createNodeExecutionStep to CreateNodeExecutionStep with new signature accepting TaskNode, improved logic to extract is_simulated from node config
core/taskengine/vm.go Added taskNode field to CommonProcessor, updated all run* methods to accept taskNode parameter, modified executeNode to pass taskNode to runners
core/taskengine/vm_runner_*.go Updated all Execute methods to use CreateNodeExecutionStep with taskNode, fixed CommonProcessor initialization in GraphQL processor
core/taskengine/*_test.go Updated test calls to create TaskNode wrappers and pass to runner methods
core/taskengine/run_node_immediately.go Updated simulation method call to create TaskNode wrapper
core/taskengine/trigger_data_flattening_test.go Added missing Type field to TaskNode creation
Comments suppressed due to low confidence (2)

core/taskengine/vm_runner_rest.go:418

  • [nitpick] The CommonProcessor is initialized with only the vm field in NewRestProcessor, but not the taskNode field. This is inconsistent with how SetTaskNode is called later in the execution flow. Consider documenting that the taskNode will be set via SetTaskNode() before execution, or initializing it to nil explicitly for clarity.
		CommonProcessor: &CommonProcessor{
			vm: vm,
		},

core/taskengine/vm_runner_rest.go:431

  • [nitpick] The CommonProcessor is initialized with only the vm field in NewRestProcessorWithExecutor, but not the taskNode field. This is inconsistent with how SetTaskNode is called later in the execution flow. Consider documenting that the taskNode will be set via SetTaskNode() before execution, or initializing it to nil explicitly for clarity.
		CommonProcessor: &CommonProcessor{
			vm: vm,
		},

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.
@chrisli30 chrisli30 merged commit 18e81e6 into staging Nov 6, 2025
17 checks passed
chrisli30 added a commit that referenced this pull request Nov 18, 2025
* 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]>
chrisli30 added a commit that referenced this pull request Nov 19, 2025
* 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]>
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.

2 participants