fix: loop node GraphQL runner extraction and execution status hardening (#524)

Co-authored-by: Wei Lin <wei@avaprotocol.org>

Author: chrisli30

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
 }
 

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)
+		}
+	}
+}

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.