diff --git a/core/taskengine/vm.go b/core/taskengine/vm.go index cbbfb4f0..fc7a8973 100644 --- a/core/taskengine/vm.go +++ b/core/taskengine/vm.go @@ -4188,6 +4188,10 @@ func (eq *ExecutionQueue) executeTask(task *ExecutionTask) *ExecutionResult { nodeType = "customCode" case step.GetRestApi() != nil: nodeType = "restApi" + case step.GetGraphql() != nil: + nodeType = "graphql" + case step.GetEthTransfer() != nil: + nodeType = "ethTransfer" } eq.vm.logger.Info("Loop iteration result extracted", "step_id", task.StepID, @@ -4253,6 +4257,9 @@ func (eq *ExecutionQueue) extractResultData(step *avsproto.Execution_Step) inter txHash := extractTxHashFromFlatMetadata(step.Metadata) return wrapResultWithTxHash(resultData, txHash) } + if graphqlOutput := step.GetGraphql(); graphqlOutput != nil && graphqlOutput.Data != nil { + return graphqlOutput.Data.AsInterface() + } return nil } diff --git a/core/taskengine/vm_runner_graphql_query_test.go b/core/taskengine/vm_runner_graphql_query_test.go index dd53118b..24d50991 100644 --- a/core/taskengine/vm_runner_graphql_query_test.go +++ b/core/taskengine/vm_runner_graphql_query_test.go @@ -171,3 +171,89 @@ func TestGraphlQlNodeSimpleQuery(t *testing.T) { t.Errorf("decimals doesn't match. expected %d got %d", 6, output.Markets[1].InputToken.Decimals) } } + +// Test that Loop node with GraphQL runner correctly extracts iteration results. +// This is a regression test for https://github.com/AvaProtocol/EigenLayer-AVS/issues/523 +// where extractResultData was missing the GraphQL output case, causing all GraphQL +// loop iterations to be counted as "failed" even though the HTTP requests succeeded. +func TestLoopWithGraphQLRunner(t *testing.T) { + requestCount := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{ + "data": { + "country": { + "name": "TestCountry", + "code": "TC" + } + } + }`) + })) + defer mockServer.Close() + + // Build a loop node with GraphQL runner + loopConfig := map[string]interface{}{ + "inputVariable": "{{countryCodes}}", + "iterVal": "value", + "iterKey": "index", + "runner": map[string]interface{}{ + "type": "graphqlDataQuery", + "config": map[string]interface{}{ + "url": mockServer.URL, + "query": `query { country(code: "{{value}}") { name code } }`, + }, + }, + } + + node, err := CreateNodeFromType(NodeTypeLoop, loopConfig, "loop_graphql_test") + if err != nil { + t.Fatalf("failed to create loop node: %v", err) + } + + inputVariables := map[string]interface{}{ + "countryCodes": []interface{}{"US", "JP"}, + } + + vm, err := NewVMWithData(nil, nil, testutil.GetTestSmartWalletConfig(), nil) + if err != nil { + t.Fatalf("failed to create VM: %v", err) + } + vm.WithLogger(testutil.GetLogger()) + + step, err := vm.RunNodeWithInputs(node, inputVariables) + if err != nil { + t.Fatalf("expected loop to succeed but got error: %v", err) + } + + if !step.Success { + t.Fatalf("expected loop step to succeed but got failure: %s", step.Error) + } + + // Verify GraphQL HTTP requests were actually made + if requestCount != 2 { + t.Errorf("expected 2 GraphQL HTTP requests but got %d", requestCount) + } + + // Verify loop output data contains results from both iterations + loopOutput := step.GetLoop() + if loopOutput == nil || loopOutput.Data == nil { + t.Fatal("expected loop output data but got nil") + } + + outputArray, ok := loopOutput.Data.AsInterface().([]interface{}) + if !ok { + t.Fatalf("expected loop output to be an array, got %T", loopOutput.Data.AsInterface()) + } + + if len(outputArray) != 2 { + t.Fatalf("expected 2 results but got %d", len(outputArray)) + } + + // Verify each iteration result is non-nil (the fix for issue #523) + for i, result := range outputArray { + if result == nil { + t.Errorf("iteration %d result is nil - extractResultData is not handling GraphQL output", i) + } + } +} diff --git a/docs/changes/0001-execution-status-redesign.md b/docs/changes/0001-execution-status-redesign.md index dd90efe2..8848af06 100644 --- a/docs/changes/0001-execution-status-redesign.md +++ b/docs/changes/0001-execution-status-redesign.md @@ -72,15 +72,145 @@ enum ExecutionStatus { - Enum value `4` is reserved and will not be reused. - New enum value `ERROR = 5` for system-level failures. -## SDK/Client Migration +## How the Client Should Interpret Execution Status -1. Remove any `PartialSuccess` handling or `isConditionalSkip` workarounds. -2. Treat `SUCCESS` as the only positive outcome. Branch skips no longer - produce a warning status. -3. Treat `FAILED` as the single status for any node-level execution failure, - regardless of whether some or all steps failed. -4. Treat `ERROR` as a system-level problem (not caused by the workflow - configuration itself). +### Current state (SDK still has `PartialSuccess`) + +The SDK (`@avaprotocol/types`) still exposes four runtime statuses: +`Success`, `PartialSuccess`, `Failed`, `Pending`. The backend returns +`partialSuccess` for **both** conditional branch skips and actual step +failures, so the client must disambiguate. + +**Decision tree for `PartialSuccess`:** + +``` +status === PartialSuccess + └─ steps.every(s => s.success) ? + ├─ YES → conditional skip (treat as success) + └─ NO → real step failure (treat as error/warning) +``` + +**Full client interpretation table (current SDK):** + +| `status` | `steps.every(s => s.success)` | Interpretation | UI treatment | +|------------------|-------------------------------|------------------------|----------------------------------------------| +| `Success` | true | All nodes ran, all OK | Green — "Simulation completed" | +| `PartialSuccess` | true | Branch skipped nodes | Green — "Simulation completed — Some steps skipped by condition" | +| `PartialSuccess` | false | Real step failure(s) | Yellow — "Simulation completed with errors" | +| `Failed` | false | All-or-nothing failure | Red — "Simulation completed with errors" | +| `Pending` | n/a | Still running | Neutral — loading state | + +**Where this logic lives in Studio today:** + +1. **Simulation status label** (`components/CanvasToolbarBottom/index.tsx`): + ```ts + const isSuccess = simulationResult?.status === ExecutionStatus.Success; + const isFailed = simulationResult?.status === ExecutionStatus.Failed; + const isConditionalSkip = + simulationResult?.status === ExecutionStatus.PartialSuccess && + simulationResult?.steps?.every((step: StepProps) => step.success); + const hasStepErrors = !isSuccess && !isConditionalSkip; + ``` + - `isConditionalSkip` → green label, normal "Deploy" button + - `hasStepErrors` → yellow/red label, "Deploy Anyway" button with caution tooltip + +2. **Simulation gate for deploy** (`app/types/simulation.ts`): + Only `ExecutionStatus.Success` passes the gate. `PartialSuccess` (even + conditional skips) currently fails the gate — this needs updating after + the SDK change so that the gate also passes for `Success` with skipped + steps. + +3. **Execution time display** (`CanvasToolbarBottom getExecutionTimeDisplay`): + Maps status to text color: `Success` → green, `PartialSuccess` → yellow, + `Failed` → red, `Pending` → yellow. + +4. **Execution history chips** (`components/workflows/StatusChips.tsx`): + - `Success` → "Completed" (green check) + - `PartialSuccess` → "Partial Success" (yellow alert) + - `Failed` → "Has Error" (red X) + +5. **Execution modal header** (`components/workflows/ExecutionModal.tsx`): + - `Success` → green "Success" + - `PartialSuccess` → yellow "Partial Success" + - `Failed` → red "Has Error" + +6. **Manual run callback** (`components/workflows/WorkflowControlButton.tsx`): + Treats both `Success` and `PartialSuccess` as a successful run for + the toast notification. This is the only place that already collapses + the two into a single positive path. + +### Target state (after SDK upgrade) + +Once the SDK ships the `SUCCESS`/`FAILED`/`ERROR` redesign: + +| `status` | Interpretation | UI treatment | +|-----------|-----------------------------------------|---------------------------------------------| +| `Success` | All executed steps passed (skips OK) | Green — "Simulation completed" | +| `Failed` | At least one step failed | Red — "Simulation completed with errors" | +| `Error` | System-level failure (VM crash, etc.) | Red — "Simulation failed — system error" | +| `Pending` | Still running | Neutral — loading state | + +**Branch skip detection** moves from status-level to the `steps` array: +compare `steps.length` against the workflow's total node count. If +`steps.length < nodeCount` and `status === Success`, display an +informational note like "N steps skipped by condition". + +### SDK/Client Migration Checklist + +After upgrading `@avaprotocol/types`: + +1. **Remove `isConditionalSkip` workaround** in `CanvasToolbarBottom` — `Success` + already covers this case. +2. **Remove `PartialSuccess` branches** from `StatusChips.tsx`, + `ExecutionModal.tsx`, `getExecutionTimeDisplay()`. +3. **Add `Error` handling** — new status for system-level failures. + Display distinctly from `Failed` (e.g., "System error — contact support" + vs "Execution failed — check step details"). +4. **Update simulation gate** (`app/types/simulation.ts`) — `Success` is the + only passing status (same as today, but no ambiguity). +5. **Update `WorkflowControlButton`** — remove `PartialSuccess` from the + success path; add `Error` to the failure path. +6. **Update Storybook mocks** — replace `ExecutionStatus.PartialSuccess` + references with `Failed` or `Success` as appropriate. + +## SDK Changes (ava-sdk-js) + +**PR**: https://github.com/AvaProtocol/ava-sdk-js/pull/212 + +### TypeScript enum update + +```typescript +// packages/types/src/enums.ts +export enum ExecutionStatus { + Unspecified = "unspecified", + Pending = "pending", + Success = "success", + Failed = "failed", + Error = "error", // NEW — replaces PartialSuccess +} +``` + +### Backward compatibility + +The SDK conversion functions handle legacy proto value `4` (retired +`PARTIAL_SUCCESS`) by mapping it to `ExecutionStatus.Failed`: + +```typescript +case ProtobufExecutionStatus.EXECUTION_STATUS_FAILED: +case 4 as ProtobufExecutionStatus: // legacy PARTIAL_SUCCESS + return ExecutionStatus.Failed; +``` + +This ensures stored executions written before the migration are +displayed correctly without requiring a data migration. + +### Test updates (18 files) + +- All `ExecutionStatus.PartialSuccess` assertions replaced with + `ExecutionStatus.Failed` (step failures) or `ExecutionStatus.Success` + (branch skips) +- `partialSuccess.test.ts` rewritten — test names and expectations + aligned with the new three-value model ## Consequences @@ -90,3 +220,5 @@ enum ExecutionStatus { - Email summaries for branch-skip workflows now show a green success badge with a note like "3 nodes skipped by Branch condition" instead of a yellow warning badge. +- Legacy stored executions with proto value `4` are transparently + mapped to `Failed` by the SDK — no data migration required.