From 5d6dccfb183bbbc8b267465256021408bc27e992 Mon Sep 17 00:00:00 2001 From: hongming Date: Wed, 20 May 2026 03:21:22 -0700 Subject: [PATCH] fix(delegation): emit error_detail alongside error in /delegations rows (P1 #348) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Python poll-mode A2A consumer in a2a_tools_delegation.py:184 reads `terminal.get("error_detail")` from `/workspaces/:id/delegations` rows, but the Go listDelegations handlers emitted only `error`. When a delegation failed, polling silently lost the failure reason and fell through to the generic "delegation failed" string — leaving the canvas "FAILED TO DELIVER" surface with no actionable detail. Both ledger-path (listDelegationsFromLedger) and activity_logs-path (listDelegationsFromActivityLogs) now emit BOTH keys: `error_detail` (canonical, what poll-mode reads) and `error` (kept for back-compat with existing canvas UI consumers). Regression tests pin both keys for both code paths. Refs P1 #348, RFC #2829 PR-2 follow-up. --- .../internal/handlers/delegation.go | 10 +++ .../internal/handlers/delegation_test.go | 75 ++++++++++++++++++- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/workspace-server/internal/handlers/delegation.go b/workspace-server/internal/handlers/delegation.go index 3d1b6e5db..06c1eb969 100644 --- a/workspace-server/internal/handlers/delegation.go +++ b/workspace-server/internal/handlers/delegation.go @@ -747,6 +747,14 @@ func (h *DelegationHandler) listDelegationsFromLedger(ctx context.Context, works entry["response_preview"] = textutil.TruncateBytes(resultPreview.String, 300) } if errorDetail.Valid && errorDetail.String != "" { + // Emit both keys: `error_detail` is the canonical field the + // Python poll-mode consumer (a2a_tools_delegation.py:184) + // reads from /delegations rows — without it, poll-mode + // silently loses the failure reason and falls through to + // the generic "delegation failed" string. `error` is kept + // for back-compat with existing UI surfaces that read the + // shorter name. + entry["error_detail"] = errorDetail.String entry["error"] = errorDetail.String } if lastHeartbeat != nil { @@ -808,6 +816,8 @@ func (h *DelegationHandler) listDelegationsFromActivityLogs(ctx context.Context, entry["delegation_id"] = delegationID } if errorDetail != "" { + // Emit both keys per the rename: see listDelegationsFromLedger. + entry["error_detail"] = errorDetail entry["error"] = errorDetail } if responseBody != "" { diff --git a/workspace-server/internal/handlers/delegation_test.go b/workspace-server/internal/handlers/delegation_test.go index 223b8de77..863f4fd0f 100644 --- a/workspace-server/internal/handlers/delegation_test.go +++ b/workspace-server/internal/handlers/delegation_test.go @@ -1546,6 +1546,71 @@ func TestListDelegations_LedgerEmptyFallsBackToActivityLogs(t *testing.T) { } } +// ---------- ListDelegations: activity_logs failed row emits BOTH error + error_detail ---------- + +// Field-rename pin (P1 #348 / RFC #2829 PR-2 follow-up): the legacy +// activity_logs fallback path must also emit `error_detail` alongside +// the historical `error` key. Without this, poll-mode (which reads +// `error_detail`) silently loses the failure reason when the ledger +// is empty and the handler falls back to activity_logs. +func TestListDelegations_ActivityLogsFailedEmitsBothErrorKeys(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + wh := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + dh := NewDelegationHandler(wh, broadcaster) + + // Ledger empty → fall back to activity_logs. + mock.ExpectQuery("SELECT d.delegation_id, d.caller_id, d.callee_id, d.task_preview"). + WithArgs("ws-source"). + WillReturnRows(sqlmock.NewRows([]string{ + "delegation_id", "caller_id", "callee_id", "task_preview", + "status", "result_preview", "error_detail", "last_heartbeat", + "deadline", "created_at", "updated_at", + })) + + now := time.Now() + activityRows := sqlmock.NewRows([]string{ + "id", "activity_type", "source_id", "target_id", + "summary", "status", "error_detail", "response_body", + "delegation_id", "created_at", + }).AddRow( + "act-failed", "delegate_result", "ws-source", "ws-target", + "Delegation failed", "error", "codex runtime timed out", "", + "del-failed-002", now, + ) + mock.ExpectQuery("SELECT id, activity_type"). + WithArgs("ws-source"). + WillReturnRows(activityRows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-source"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-source/delegations", nil) + + dh.ListDelegations(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp []map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to parse response: %v", err) + } + if len(resp) != 1 { + t.Fatalf("expected 1 row, got %d", len(resp)) + } + if resp[0]["error"] != "codex runtime timed out" { + t.Errorf("expected `error` field set, got %v", resp[0]["error"]) + } + if resp[0]["error_detail"] != "codex runtime timed out" { + t.Errorf("expected `error_detail` field set (poll-mode contract), got %v", resp[0]["error_detail"]) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // ---------- ListDelegations: both ledger and activity_logs empty → [] ---------- func TestListDelegations_BothEmptyReturnsEmptyArray(t *testing.T) { @@ -1744,7 +1809,15 @@ func TestListDelegations_LedgerFailedIncludesErrorDetail(t *testing.T) { t.Errorf("expected status 'failed', got %v", resp[0]["status"]) } if resp[0]["error"] != "Callee workspace not reachable" { - t.Errorf("expected error detail, got %v", resp[0]["error"]) + t.Errorf("expected error detail under `error`, got %v", resp[0]["error"]) + } + // Field-rename pin (P1 #348 / RFC #2829 PR-2 follow-up): the + // Python poll-mode consumer in a2a_tools_delegation.py:184 reads + // `error_detail`, not `error`. Both keys MUST be present so polling + // surfaces the real failure reason instead of falling through to + // the generic "delegation failed" string. + if resp[0]["error_detail"] != "Callee workspace not reachable" { + t.Errorf("expected error detail under `error_detail`, got %v", resp[0]["error_detail"]) } if err := mock.ExpectationsWereMet(); err != nil { t.Errorf("unmet sqlmock expectations: %v", err) -- 2.52.0