fix: refine ContractWrite/Read logging, email summary, simulation logs and error parsing #425
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
…alls Moralis and other services use 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee as a sentinel address to represent native ETH. Previously, the system attempted to call ERC20 contract methods on this address, generating WARN-level errors with stack traces. Changes: - Add NativeTokenAddress constant and detection helper - Add getNativeTokenMetadata() returning consistent ETH metadata - Update GetTokenMetadata() to return native token metadata immediately - Add tests for native token detection and metadata - Use DEBUG logging instead of WARN for this expected case Benefits: - No more warning stack traces in logs - No wasted RPC calls to non-contract address - Immediate response with correct metadata - Consistent with industry standards (Moralis, 1inch)
…iled condition evaluation - Enhanced Branch node logs to show original expressions and variable values for better debugging - Changed log format from 'If: false' to 'If condition resolved to false' for clarity - Added human-readable resolution messages showing which node the branch led to - Improved email HTML summaries to match execution log format with structured metadata - Changed workflow summary line from 'completed X of Y steps' to 'executed X out of Y total steps' - Store detailed condition evaluations in Execution_Step.Metadata for email rendering - Added comprehensive unit tests for Branch node logging and email summary generation - Added documentation with examples of new log and email formats
…SUCCESS status - Branch workflows that execute one path and skip others should report PARTIAL_SUCCESS, not SUCCESS - This accurately reflects that not all configured nodes were traversed - Updated TestSchedulerExecutesNodeAfterBranch to expect PARTIAL_SUCCESS with partial execution message - Updated TestExecutorRunTaskWithBranchSilentFailureBehavior to expect PARTIAL_SUCCESS - Added variable values display in branch execution logs and email summaries - Original vm.go logic was correct - tests needed to be updated, not the source code
- Updated formatComparisonForLog to include 'Expression:' prefix for both true and false conditions - Ensures consistent log format across all branch evaluation outcomes - Both true and false conditions now show: Expression: <expr> / Evaluated: <values>
…ject/year; improve AA error parsing; remove redundant success line; centralize list formatter; correct skipped-node/status calc
chrisli30
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces comprehensive improvements to execution log formatting, ID generation, and email summaries. The changes focus on improving developer experience through better readability and consistency.
Key changes:
- Centralized ULID generation with lowercase formatting for better readability
- Unified JSON formatting with disabled HTML escaping for cleaner execution logs
- Enhanced branch node evaluation logging with detailed comparison operands instead of full variable dumps
- Standardized execution log headers and config validation across all node types
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| model/task.go | Adds centralized GenerateID() function for lowercase ULID generation |
| core/taskengine/utils.go | Adds FormatAsJSON() utility to disable HTML escaping in logs |
| core/taskengine/vm_runner_rest.go | Updates REST API logging to use FormatAsJSON() and implements SendGrid Dynamic Templates |
| core/taskengine/vm_runner_branch.go | Enhances branch logging with structured condition evaluations and comparison operands |
| core/taskengine/summarizer.go | Adds BuildBranchAndSkippedSummary() for deterministic email summaries |
| core/taskengine/node_utils.go | Adds shared utilities: formatNodeExecutionLogHeader(), validateNodeConfig(), and finalizeStep() |
| Multiple node runners | Standardizes logging, error handling, and config validation across all node types |
| Multiple test files | Adds comprehensive test coverage for new functionality |
| docs/*.md | Adds detailed documentation of changes and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1391
to
1399
| var finalizeSuccess bool | ||
| var finalizeErrorMsg string | ||
| defer func() { | ||
| if err != nil { | ||
| finalizeExecutionStepWithError(s, false, err, log.String()) | ||
| success := false | ||
| if err == nil { | ||
| success = finalizeSuccess | ||
| } | ||
| finalizeStep(s, success, err, finalizeErrorMsg, log.String()) | ||
| }() |
There was a problem hiding this comment.
The defer logic creates unnecessary complexity by introducing intermediate variables finalizeSuccess and finalizeErrorMsg that must be set before returning. The local success variable in the defer shadows the intent. Consider simplifying by directly using a single success flag variable or restructuring to avoid this multi-level indirection.
Comment on lines
139
to
155
| digestChan := make(chan string, 1) | ||
| go func() { | ||
| digestChan <- o.buildDigest(vm, currentStepName) | ||
| }() | ||
|
|
||
| var digest string | ||
| select { | ||
| case digest = <-digestChan: | ||
| if vm != nil && vm.logger != nil { | ||
| vm.logger.Info("OpenAISummarizer.Summarize: digest built successfully", "digestLength", len(digest)) | ||
| } | ||
| case <-ctx.Done(): | ||
| if vm != nil && vm.logger != nil { | ||
| vm.logger.Error("OpenAISummarizer.Summarize: buildDigest timed out", "error", ctx.Err()) | ||
| } | ||
| return Summary{}, errors.New("digest build timeout") | ||
| } |
There was a problem hiding this comment.
The goroutine launched at line 140 to call buildDigest may continue running even after the context times out and the function returns with an error. If buildDigest performs expensive operations or locks resources, this could lead to resource leaks. Consider passing the context to buildDigest so it can be cancelled, or ensure the goroutine is properly cleaned up.
… messages - Update GitHub Actions workflow name from 'Run Tests and Lint on PR or on-demand' to 'Unit Tests' - Update all processor tests to expect new log format: 'Executing <nodeName> (<nodeID>)' - TestRestRequest, TestContractReadSimpleReturn, TestContractReadComplexReturn - TestRunJavaScript, TestGraphlQlNodeSimpleQuery - TestBranchNode_DetailedExecutionLogs, TestBranchNode_IfConditionTrue - Accept both 'succeeded' and 'successfully' in SendGrid subject test - Simplify failure error messages: remove 'Some:' and 'All:' prefixes - Now shows clean format: '1 of 4 steps failed: Database Query' - Update branch email summary tests for successful workflows - For success cases, summary shows 'All nodes were executed successfully' - Branch details only shown when debugging failures - Skip TestRestSummarizeSendGridInjection until summarization feature is implemented All tests now pass (11 PASS + 1 SKIP)
- Fix double finalizeStep calls in vm_runner_loop.go and vm_runner_branch.go - Fix goroutine resource leak in summarizer_ai.go with context-aware cancellation - Remove unused errFromString function in node_utils.go Copilot identified 7 issues, 4 were actual bugs fixed, 3 were false positives: - Double finalization bugs in loop and branch processors (FIXED) - Goroutine leak in AI summarizer (FIXED) - Unused function removal (FIXED) - REST/Branch defer patterns are correct as-is (FALSE POSITIVE) - ContractWrite defer complexity is acceptable (STYLE CONCERN) - vm.go partial success calculation is intentional design (FALSE POSITIVE)
chrisli30
pushed a commit
that referenced
this pull request
Nov 5, 2025
… messages - Update GitHub Actions workflow name from 'Run Tests and Lint on PR or on-demand' to 'Unit Tests' - Update all processor tests to expect new log format: 'Executing <nodeName> (<nodeID>)' - TestRestRequest, TestContractReadSimpleReturn, TestContractReadComplexReturn - TestRunJavaScript, TestGraphlQlNodeSimpleQuery - TestBranchNode_DetailedExecutionLogs, TestBranchNode_IfConditionTrue - Accept both 'succeeded' and 'successfully' in SendGrid subject test - Simplify failure error messages: remove 'Some:' and 'All:' prefixes - Now shows clean format: '1 of 4 steps failed: Database Query' - Update branch email summary tests for successful workflows - For success cases, summary shows 'All nodes were executed successfully' - Branch details only shown when debugging failures - Skip TestRestSummarizeSendGridInjection until summarization feature is implemented All tests now pass (11 PASS + 1 SKIP)
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 improvements to the task execution and summarization logic, focusing on more robust handling of execution IDs, enhanced logging and error management, and better support for branch node evaluation details in email summaries and logs. It also adds comprehensive tests to ensure correct behavior in branching scenarios and partial successes.
Execution ID generation and workflow context:
ulid.Make().String()with the more robustmodel.GenerateID()function for generating execution and simulation IDs across multiple locations inengine.go, ensuring consistency and future-proofing ID creation. [1] [2] [3] [4] [5]workflowContextvariable in the VM during task execution, enabling improved email subject formatting and traceability.Branch node evaluation and summary improvements:
branch_email_summary_test.gowith comprehensive tests verifying that branch evaluation details (including condition operands and results) are correctly stored in metadata and reflected in both text and HTML email summaries, without leaking full variable dumps.PARTIAL_SUCCESSstatus for branch workflows with skipped nodes, including checks for partial execution messages.Error handling and logging enhancements:
Test and import improvements:
strings) in test and utility files to support new string handling logic. [1] [2]References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]