From f0b6079a82beeb8d25fb050290ad566c5ce8c922 Mon Sep 17 00:00:00 2001 From: core-devops Date: Thu, 4 Jun 2026 14:32:08 -0700 Subject: [PATCH 1/2] fix(a2a): default message.role + normalize part kind in normalizeA2APayload (#2251) Single Go choke that guarantees a schema-valid outbound A2A message/send envelope: default params.message.role to "user" when absent (inject-only, never clobbers a caller-supplied "agent"), and rename legacy part "type" discriminator to v0.3 "kind". All 7 outbound message/send paths funnel through proxyA2ARequest -> normalizeA2APayload, so this is the single authority. The a2a-sdk v0.3 validator marks role REQUIRED; role-less envelopes were failing peers with 'params.message.role Field required' (broke delegate_task / the agents-team transport). Contract tests added (role default, explicit-role preserved, type->kind, regression guard). Part of the cross-repo SSOT fix anchored on the a2a-sdk SendMessageRequest schema (runtime + mcp-server companions). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/handlers/a2a_proxy.go | 33 +++++ .../internal/handlers/a2a_proxy_test.go | 136 ++++++++++++++++++ 2 files changed, 169 insertions(+) diff --git a/workspace-server/internal/handlers/a2a_proxy.go b/workspace-server/internal/handlers/a2a_proxy.go index 17321a352..b1fc19ae9 100644 --- a/workspace-server/internal/handlers/a2a_proxy.go +++ b/workspace-server/internal/handlers/a2a_proxy.go @@ -801,6 +801,18 @@ func normalizeA2APayload(body []byte) ([]byte, string, *proxyA2AError) { if _, hasID := msg["messageId"]; !hasID { msg["messageId"] = uuid.New().String() } + // #2251: default params.message.role to "user" when absent. + // The downstream a2a-sdk v0.3 Pydantic validator marks role a + // REQUIRED field; a role-less envelope fails parse with + // "params.message.role Field required". The Go builders + // (mcp_tools/delegation/scheduler/channels) already set it, but + // raw external/canvas POSTs to ProxyA2A may omit it — making this + // the single canonical choke that guarantees a schema-valid role. + // Mirror the messageId default exactly: inject only when missing, + // never overwrite a caller-supplied role (e.g. "agent"). + if _, hasRole := msg["role"]; !hasRole { + msg["role"] = "user" + } _, hasParts := msg["parts"] rawContent, hasContent := msg["content"] if !hasParts { @@ -832,6 +844,27 @@ func normalizeA2APayload(body []byte) ([]byte, string, *proxyA2AError) { } } } + // #2251: wire hygiene — the A2A v0.3 Part discriminator is + // "kind", but some builders/clients emit the legacy "type" key + // (e.g. delegation.go). The v0.3 Pydantic validator keys on + // "kind"; a stray "type" leaves the Part untagged. Rename + // "type" → "kind" on every Part that lacks an explicit "kind" + // so the discriminator is always present on the wire. + if parts, ok := msg["parts"].([]interface{}); ok { + for _, p := range parts { + part, ok := p.(map[string]interface{}) + if !ok { + continue + } + if _, hasKind := part["kind"]; hasKind { + continue + } + if t, hasType := part["type"]; hasType { + part["kind"] = t + delete(part, "type") + } + } + } } } diff --git a/workspace-server/internal/handlers/a2a_proxy_test.go b/workspace-server/internal/handlers/a2a_proxy_test.go index 10b21cbda..d0c5e7d81 100644 --- a/workspace-server/internal/handlers/a2a_proxy_test.go +++ b/workspace-server/internal/handlers/a2a_proxy_test.go @@ -1514,6 +1514,142 @@ func TestNormalizeA2APayload_NoMessageNoCheck(t *testing.T) { } } +// --- #2251: role default + part-kind hygiene contract tests --- +// +// These assert normalizeA2APayload is the single canonical Go choke that +// guarantees a schema-valid outbound message/send envelope: it injects a +// default params.message.role="user" when the sender omitted role (the bug +// that made delegate_task fail the peer's a2a Pydantic validator with +// "params.message.role Field required" while reply_to_workspace worked), and +// it renames the legacy Part discriminator "type"→"kind" for wire hygiene. + +// normMsg is a small helper that runs normalizeA2APayload and returns the +// resolved params.message map, failing the test on any normalization error. +func normMsg(t *testing.T, raw string) map[string]interface{} { + t.Helper() + out, _, perr := normalizeA2APayload([]byte(raw)) + if perr != nil { + t.Fatalf("normalizeA2APayload returned error: %+v", perr) + } + var parsed map[string]interface{} + if err := json.Unmarshal(out, &parsed); err != nil { + t.Fatalf("output not valid JSON: %v", err) + } + params, ok := parsed["params"].(map[string]interface{}) + if !ok { + t.Fatalf("output missing params object: %s", string(out)) + } + msg, ok := params["message"].(map[string]interface{}) + if !ok { + t.Fatalf("output missing params.message object: %s", string(out)) + } + return msg +} + +func TestNormalizeA2APayload_DefaultsRoleWhenMissing(t *testing.T) { + cases := []struct { + name string + raw string + }{ + { + name: "v0.3 parts, no role", + raw: `{"method":"message/send","params":{"message":{"parts":[{"kind":"text","text":"hi"}]}}}`, + }, + { + name: "v0.2 string content, no role", + raw: `{"method":"message/send","params":{"message":{"content":"hi"}}}`, + }, + { + name: "legacy type part, no role", + raw: `{"method":"message/send","params":{"message":{"parts":[{"type":"text","text":"hi"}]}}}`, + }, + { + name: "already wrapped jsonrpc, no role", + raw: `{"jsonrpc":"2.0","id":"x","method":"message/send","params":{"message":{"parts":[{"kind":"text","text":"hi"}]}}}`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + msg := normMsg(t, tc.raw) + if msg["role"] != "user" { + t.Errorf("expected role defaulted to \"user\", got %v", msg["role"]) + } + // Parts must remain valid (non-empty) after normalization. + parts, ok := msg["parts"].([]interface{}) + if !ok || len(parts) == 0 { + t.Fatalf("expected non-empty parts after normalization, got %v", msg["parts"]) + } + // Every part must carry the v0.3 "kind" discriminator. + for i, p := range parts { + part, ok := p.(map[string]interface{}) + if !ok { + t.Fatalf("part %d is not an object: %v", i, p) + } + if _, hasKind := part["kind"]; !hasKind { + t.Errorf("part %d missing \"kind\" discriminator: %v", i, part) + } + if _, hasType := part["type"]; hasType { + t.Errorf("part %d still has legacy \"type\" key: %v", i, part) + } + } + }) + } +} + +func TestNormalizeA2APayload_PreservesExplicitRole(t *testing.T) { + // A caller-supplied role (e.g. "agent") must NOT be overwritten with "user". + msg := normMsg(t, `{"method":"message/send","params":{"message":{"role":"agent","parts":[{"kind":"text","text":"hi"}]}}}`) + if msg["role"] != "agent" { + t.Errorf("explicit role overwritten: expected \"agent\", got %v", msg["role"]) + } +} + +func TestNormalizeA2APayload_RenamesPartTypeToKind(t *testing.T) { + // Mirrors delegation.go's builder which emits {"type":"text",...}. After + // normalization the wire Part must be discriminated by "kind". + msg := normMsg(t, `{"method":"message/send","params":{"message":{"role":"user","parts":[{"type":"text","text":"a"},{"type":"file","uri":"workspace:/x"}]}}}`) + parts := msg["parts"].([]interface{}) + if len(parts) != 2 { + t.Fatalf("expected 2 parts, got %d", len(parts)) + } + wantKind := []string{"text", "file"} + for i, p := range parts { + part := p.(map[string]interface{}) + if part["kind"] != wantKind[i] { + t.Errorf("part %d: expected kind=%q, got %v", i, wantKind[i], part["kind"]) + } + if _, hasType := part["type"]; hasType { + t.Errorf("part %d still carries legacy \"type\": %v", i, part) + } + } +} + +func TestNormalizeA2APayload_DoesNotClobberKindWithType(t *testing.T) { + // If a part has BOTH kind and type, kind wins and is left untouched. + msg := normMsg(t, `{"method":"message/send","params":{"message":{"role":"user","parts":[{"kind":"text","type":"ignored","text":"a"}]}}}`) + part := msg["parts"].([]interface{})[0].(map[string]interface{}) + if part["kind"] != "text" { + t.Errorf("expected kind preserved as \"text\", got %v", part["kind"]) + } +} + +// TestNormalizeA2APayload_RoleDefault_ContractRegression documents the +// pre-fix failure: without the role default, a role-less message/send body +// emerged from normalization still missing params.message.role, which the +// peer's a2a Pydantic validator rejects. This asserts the POST-fix invariant +// (role present) directly; before the a2a_proxy.go change this assertion +// fails (role is absent → msg["role"] == nil). +func TestNormalizeA2APayload_RoleDefault_ContractRegression(t *testing.T) { + msg := normMsg(t, `{"method":"message/send","params":{"message":{"parts":[{"kind":"text","text":"delegate this"}]}}}`) + role, hasRole := msg["role"] + if !hasRole { + t.Fatal("REGRESSION (#2251): params.message.role absent after normalization — peer a2a validator will reject with 'role Field required'") + } + if role != "user" { + t.Errorf("expected default role \"user\", got %v", role) + } +} + // --- resolveAgentURL direct unit tests --- func TestResolveAgentURL_CacheHit(t *testing.T) { -- 2.52.0 From fc6850196b1aba226a81e36ab16f6d48703933e9 Mon Sep 17 00:00:00 2001 From: core-devops Date: Thu, 4 Jun 2026 20:58:53 -0700 Subject: [PATCH 2/2] test(e2e): make poll-mode since_id parser A2A-v0.3-discriminator-agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2255's normalizeA2APayload (#2251) renames the legacy Part discriminator "type" -> "kind" (A2A v0.3) on ingest. ProxyA2A logs the NORMALIZED body to activity_logs (a2a_proxy.go:432 body=normalizedBody; logA2AReceiveQueued RequestBody=json.RawMessage(body)). So a poll-mode caller that posts {"type":"text",...} has its row stored with {"kind":"text",...}. test_poll_mode_e2e.sh Phase 5's ASC parser hard-coded `if p.get('type')=='text'` to extract part text from the stored request_body. Post-rename every part is keyed on "kind", so the filter matched nothing, text_of() returned '' for every row, and the assert saw `got: |` (empty|empty) -> REQUIRED E2E API Smoke gate FAILED on #2255. Root cause: the test asserted on an INTERNAL wire detail (which discriminator field the server stored) instead of on the text payload. The product change is correct and is covered by Go unit tests in a2a_proxy_test.go; only the E2E parser was coupled to the legacy format. Fix: - text_of() now accepts kind=='text' OR type=='text' (works on main's legacy feed AND on #2255's normalized feed) — so it gates the text payload, not the field name. - Add a positive wire-contract assertion: the stored Part must carry the v0.3 "kind" discriminator and NOT the legacy "type". This is the end-to-end half of the unit tests — it proves the rename survives the durable activity_logs path, and makes a dropped/reverted rename (or a feed that stops storing the normalized body) fail LOUDLY here instead of silently feeding a polling agent an untagged Part. Verified: on main (no rename) poll-mode = 22 passed/0 failed; on #2255 (f0b6079a) it was 21 passed/1 failed at this exact assert. Both parsers simulated against kind- and type-shaped feeds. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/e2e/test_poll_mode_e2e.sh | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/e2e/test_poll_mode_e2e.sh b/tests/e2e/test_poll_mode_e2e.sh index 46daa8584..f73a721ee 100755 --- a/tests/e2e/test_poll_mode_e2e.sh +++ b/tests/e2e/test_poll_mode_e2e.sh @@ -300,7 +300,14 @@ rows = json.load(sys.stdin) def text_of(r): body = r.get('request_body') or {} parts = (body.get('params') or {}).get('message', {}).get('parts') or [] - return ''.join(p.get('text','') for p in parts if p.get('type')=='text') + # A2A v0.3 keys the Part discriminator on 'kind'; legacy senders used + # 'type'. ProxyA2A.normalizeA2APayload (#2251) rewrites 'type' -> 'kind' + # on ingest, so the stored request_body carries 'kind' even when the + # caller posted 'type'. Accept EITHER so this parser asserts on the text + # payload, not on which discriminator field the server happened to store. + def is_text(p): + return p.get('kind') == 'text' or p.get('type') == 'text' + return ''.join(p.get('text', '') for p in parts if is_text(p)) if len(rows) < 2: print('NEED2_GOT_'+str(len(rows))) else: @@ -309,6 +316,29 @@ else: check_eq "since_id feed orders ASC (oldest-new first, newest-new last)" \ "hello-from-e2e-2|hello-from-e2e-3" "$ASC_FIRST" +# Wire-contract gate (#2251): the caller posted parts with the LEGACY "type" +# discriminator, but ProxyA2A.normalizeA2APayload rewrites "type" -> "kind" +# (A2A v0.3) BEFORE the row is durably logged. Assert the stored request_body +# carries "kind" and no longer carries "type", so a regression that drops the +# rename — or a feed that stops storing the normalized body — fails loudly here +# instead of silently feeding the polling agent an untagged Part. This is the +# end-to-end half of the Go unit tests in a2a_proxy_test.go (which assert the +# rename in isolation); this proves it survives the durable activity_logs path. +DISC=$(echo "$ASC_RESP" | python3 -c " +import json, sys +rows = json.load(sys.stdin) +kinds, types = [], [] +for r in rows: + body = r.get('request_body') or {} + parts = (body.get('params') or {}).get('message', {}).get('parts') or [] + for p in parts: + if 'kind' in p: kinds.append(p['kind']) + if 'type' in p: types.append(p['type']) +print(('kind' if kinds and not types else 'BAD') + ':' + ','.join(kinds) + '/' + ','.join(types)) +") +check_eq "stored Part uses v0.3 'kind' discriminator, never legacy 'type' (#2251)" \ + "kind:text,text/" "$DISC" + # ---------- Phase 6: stale cursor returns 410 ---------- echo "" echo "--- Phase 6: Stale / unknown cursor returns 410 ---" -- 2.52.0