From 140fc5fb10eee5a91d0c76521fdf64b026a614f1 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 29 Apr 2026 22:01:22 -0700 Subject: [PATCH] =?UTF-8?q?fix(a2a):=20v0.2=20=E2=86=92=20v0.3=20compat=20?= =?UTF-8?q?shim=20at=20proxy=20edge=20(#2345)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2345. ## Symptom Design Director silently dropped A2A briefs whose sender used the v0.2 message format (`params.message.content` string) instead of v0.3 (`params.message.parts` part-list). The downstream a2a-sdk's v0.3 Pydantic validator rejected with "params.message.parts — Field required" but the rejection only landed in tenant-side logs; the sender saw HTTP 200/202 and assumed delivery. UX Researcher therefore never received the kickoff. Multi-agent pipeline silently idle. ## Fix Convert at the proxy edge in normalizeA2APayload. Two cases handled, one explicitly rejected: v0.2 string content → wrap as [{kind: text, text: }] (the canonical v0.2 case from the dogfooding report) v0.2 list content → preserve list as parts (some older clients put a list under `content`; treat as "client meant parts, used wrong field name") v0.3 parts present → no-op (hot path for normal traffic) Neither present → return HTTP 400 with structured JSON-RPC error pointing at the missing field Why at the proxy edge: every workspace gets the compat for free without each one bumping a2a-sdk separately. The SDK's own compat adapter is strict about `parts` and rejects v0.2 senders. Why reject loud on missing-both: pre-fix the SDK's Pydantic rejection was post-handler-dispatch and invisible to the original sender. Now misshapen payloads return a structured 400 to the actual caller — kills the entire silent-drop class for this payload-shape category. ## Tests 7 new cases on normalizeA2APayload (#2345) + 1 fixture update on the existing _MissingMethodReturnsEmpty test: TestNormalizeA2APayload_ConvertsV02StringContentToParts TestNormalizeA2APayload_ConvertsV02ListContentToParts TestNormalizeA2APayload_PreservesV03Parts (hot path) TestNormalizeA2APayload_RejectsMessageWithNeitherContentNorParts TestNormalizeA2APayload_RejectsContentWithUnsupportedType TestNormalizeA2APayload_NoMessageNoCheck (e.g. tasks/list bypasses) All 11 normalizeA2APayload tests pass + full handler suite (no regressions). ## Refs Hard-gates discussion: this is exactly the class of failure (silent-drop on schema mismatch) that #2342 (continuous synthetic E2E) would catch automatically. Tier 2 RFC item from #2345 (caller gets structured JSON-RPC error on parse failure) is delivered above via the loud-reject path. --- .../internal/handlers/a2a_proxy.go | 43 ++++++++ .../internal/handlers/a2a_proxy_test.go | 102 +++++++++++++++++- 2 files changed, 144 insertions(+), 1 deletion(-) diff --git a/workspace-server/internal/handlers/a2a_proxy.go b/workspace-server/internal/handlers/a2a_proxy.go index 4a7c8026..8520f564 100644 --- a/workspace-server/internal/handlers/a2a_proxy.go +++ b/workspace-server/internal/handlers/a2a_proxy.go @@ -486,11 +486,54 @@ func normalizeA2APayload(body []byte) ([]byte, string, *proxyA2AError) { } // Ensure params.message.messageId exists (required by a2a-sdk) + // AND v0.2→v0.3 compat (#2345): when sender supplies + // params.message.content (v0.2) instead of params.message.parts + // (v0.3), wrap the content as a single text Part so the downstream + // a2a-sdk's v0.3 Pydantic validator accepts the message. + // + // Pre-fix: Design Director silently dropped briefs whose sender + // used v0.2 shape — Pydantic rejected at parse time, the rejection + // went only to logs, and the sender saw a happy 200/202. + // + // Reject loud (HTTP 400) when neither content nor parts is present; + // previously the SDK's own rejection happened post-handler-dispatch + // and was invisible to the original sender. if params, ok := payload["params"].(map[string]interface{}); ok { if msg, ok := params["message"].(map[string]interface{}); ok { if _, hasID := msg["messageId"]; !hasID { msg["messageId"] = uuid.New().String() } + _, hasParts := msg["parts"] + rawContent, hasContent := msg["content"] + if !hasParts { + if hasContent { + switch v := rawContent.(type) { + case string: + msg["parts"] = []interface{}{ + map[string]interface{}{"kind": "text", "text": v}, + } + case []interface{}: + msg["parts"] = v + default: + return nil, "", &proxyA2AError{ + Status: http.StatusBadRequest, + Response: gin.H{ + "error": "invalid params.message.content type", + "hint": "content must be a string (v0.2 compat) or omitted in favour of parts (v0.3)", + }, + } + } + delete(msg, "content") + } else { + return nil, "", &proxyA2AError{ + Status: http.StatusBadRequest, + Response: gin.H{ + "error": "params.message must contain either 'parts' (v0.3) or 'content' (v0.2 compat)", + "hint": "v0.3 example: {\"parts\":[{\"kind\":\"text\",\"text\":\"...\"}]}", + }, + } + } + } } } diff --git a/workspace-server/internal/handlers/a2a_proxy_test.go b/workspace-server/internal/handlers/a2a_proxy_test.go index 1a33a866..1b43c402 100644 --- a/workspace-server/internal/handlers/a2a_proxy_test.go +++ b/workspace-server/internal/handlers/a2a_proxy_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "time" @@ -1137,7 +1138,10 @@ func TestNormalizeA2APayload_PreservesExistingMessageId(t *testing.T) { } func TestNormalizeA2APayload_MissingMethodReturnsEmpty(t *testing.T) { - raw := []byte(`{"params":{"message":{"role":"user"}}}`) + // Method extraction returns empty string when method is absent, + // regardless of message validity. Include parts: [] so the v0.2→v0.3 + // compat check (#2345) doesn't reject before method extraction. + raw := []byte(`{"params":{"message":{"role":"user","parts":[]}}}`) _, method, perr := normalizeA2APayload(raw) if perr != nil { t.Fatalf("unexpected error: %+v", perr) @@ -1147,6 +1151,102 @@ func TestNormalizeA2APayload_MissingMethodReturnsEmpty(t *testing.T) { } } +// --- v0.2 → v0.3 compat shim (#2345) --- + +func TestNormalizeA2APayload_ConvertsV02StringContentToParts(t *testing.T) { + raw := []byte(`{"method":"message/send","params":{"message":{"role":"user","content":"hello world"}}}`) + out, _, perr := normalizeA2APayload(raw) + if perr != nil { + t.Fatalf("unexpected error: %+v", perr) + } + var parsed map[string]interface{} + if err := json.Unmarshal(out, &parsed); err != nil { + t.Fatalf("output not valid JSON: %v", err) + } + msg := parsed["params"].(map[string]interface{})["message"].(map[string]interface{}) + if _, stillHasContent := msg["content"]; stillHasContent { + t.Error("v0.2 'content' field should be removed after conversion") + } + parts, ok := msg["parts"].([]interface{}) + if !ok || len(parts) != 1 { + t.Fatalf("expected 1 part, got %v", msg["parts"]) + } + part := parts[0].(map[string]interface{}) + if part["kind"] != "text" || part["text"] != "hello world" { + t.Errorf("expected {kind:text, text:'hello world'}, got %v", part) + } +} + +func TestNormalizeA2APayload_ConvertsV02ListContentToParts(t *testing.T) { + raw := []byte(`{"method":"message/send","params":{"message":{"role":"user","content":[{"kind":"text","text":"hi"}]}}}`) + out, _, perr := normalizeA2APayload(raw) + if perr != nil { + t.Fatalf("unexpected error: %+v", perr) + } + var parsed map[string]interface{} + _ = json.Unmarshal(out, &parsed) + msg := parsed["params"].(map[string]interface{})["message"].(map[string]interface{}) + parts, ok := msg["parts"].([]interface{}) + if !ok || len(parts) != 1 { + t.Fatalf("expected list preserved as parts, got %v", msg["parts"]) + } +} + +func TestNormalizeA2APayload_PreservesV03Parts(t *testing.T) { + raw := []byte(`{"method":"message/send","params":{"message":{"role":"user","parts":[{"kind":"text","text":"hi"}]}}}`) + out, _, perr := normalizeA2APayload(raw) + if perr != nil { + t.Fatalf("unexpected error: %+v", perr) + } + var parsed map[string]interface{} + _ = json.Unmarshal(out, &parsed) + msg := parsed["params"].(map[string]interface{})["message"].(map[string]interface{}) + if _, hasContent := msg["content"]; hasContent { + t.Error("did not expect content field in v0.3-shaped payload output") + } + parts := msg["parts"].([]interface{}) + if len(parts) != 1 { + t.Errorf("expected 1 part preserved, got %d", len(parts)) + } +} + +func TestNormalizeA2APayload_RejectsMessageWithNeitherContentNorParts(t *testing.T) { + raw := []byte(`{"method":"message/send","params":{"message":{"role":"user","metadata":{}}}}`) + _, _, perr := normalizeA2APayload(raw) + if perr == nil { + t.Fatal("expected error for message with neither content nor parts") + } + if perr.Status != http.StatusBadRequest { + t.Errorf("expected 400, got %d", perr.Status) + } + errMsg, _ := perr.Response["error"].(string) + if !strings.Contains(errMsg, "parts") || !strings.Contains(errMsg, "content") { + t.Errorf("error message should mention both 'parts' and 'content', got: %q", errMsg) + } +} + +func TestNormalizeA2APayload_RejectsContentWithUnsupportedType(t *testing.T) { + raw := []byte(`{"method":"message/send","params":{"message":{"role":"user","content":42}}}`) + _, _, perr := normalizeA2APayload(raw) + if perr == nil { + t.Fatal("expected error for non-string non-list content") + } + if perr.Status != http.StatusBadRequest { + t.Errorf("expected 400, got %d", perr.Status) + } +} + +func TestNormalizeA2APayload_NoMessageNoCheck(t *testing.T) { + raw := []byte(`{"method":"tasks/list","params":{}}`) + _, method, perr := normalizeA2APayload(raw) + if perr != nil { + t.Fatalf("unexpected error on params-message-absent payload: %+v", perr) + } + if method != "tasks/list" { + t.Errorf("expected method=tasks/list, got %q", method) + } +} + // --- resolveAgentURL direct unit tests --- func TestResolveAgentURL_CacheHit(t *testing.T) {