diff --git a/.github/workflows/e2e-api.yml b/.github/workflows/e2e-api.yml index a0238dcd..05f9ab52 100644 --- a/.github/workflows/e2e-api.yml +++ b/.github/workflows/e2e-api.yml @@ -97,6 +97,8 @@ jobs: echo "Migrations OK" - name: Run E2E API tests run: bash tests/e2e/test_api.sh + - name: Run notify-with-attachments E2E + run: bash tests/e2e/test_notify_attachments_e2e.sh - name: Dump platform log on failure if: failure() run: cat workspace-server/platform.log || true diff --git a/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts b/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts index 809c37ab..238f5bb6 100644 --- a/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts +++ b/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts @@ -262,4 +262,36 @@ describe("extractFilesFromTask", () => { const files = extractFilesFromTask(task); expect(files[0]).toMatchObject({ name: "out.txt", uri: "workspace:/workspace/out.txt" }); }); + + it("hydrates a notify-with-attachments response_body — both text caption AND file chips", () => { + // Pins the exact wire shape the platform's Notify handler persists + // when send_message_to_user passes attachments (activity.go writes + // response_body = {"result": , "parts": [{kind:"file",...}]}). + // The chat history loader runs both extractors over this object on + // reload — without this contract holding, refreshing the page after + // an agent attached a file would lose either the caption or the chips. + const responseBody = { + result: "Done — see attached.", + parts: [ + { + kind: "file", + file: { + name: "build-output.zip", + mimeType: "application/zip", + uri: "workspace:/tmp/build-output.zip", + size: 12345, + }, + }, + ], + }; + expect(extractResponseText(responseBody)).toBe("Done — see attached."); + expect(extractFilesFromTask(responseBody)).toEqual([ + { + name: "build-output.zip", + mimeType: "application/zip", + uri: "workspace:/tmp/build-output.zip", + size: 12345, + }, + ]); + }); }); diff --git a/canvas/src/store/__tests__/canvas-events.test.ts b/canvas/src/store/__tests__/canvas-events.test.ts index a8b767e2..84f81d57 100644 --- a/canvas/src/store/__tests__/canvas-events.test.ts +++ b/canvas/src/store/__tests__/canvas-events.test.ts @@ -598,6 +598,143 @@ describe("handleCanvasEvent – AGENT_MESSAGE", () => { expect(set).not.toHaveBeenCalled(); }); + + // Attachment passthrough — the broadcast payload's `attachments` array + // is the wire format the platform's Notify handler emits (activity.go: + // 318-326). These tests pin the canvas-side filtering / shape coercion + // so the chat reliably renders download chips for agent-sent files. + + it("passes through valid attachments onto the new message", () => { + const node = makeNode("ws-1"); + const { get, set } = makeStore([node], [], null, {}); + const att = { + uri: "workspace:/tmp/build.zip", + name: "build.zip", + mimeType: "application/zip", + size: 12345, + }; + + handleCanvasEvent( + makeMsg({ + event: "AGENT_MESSAGE", + workspace_id: "ws-1", + payload: { message: "see attached", attachments: [att] }, + }), + get, + set, + ); + + const { agentMessages } = set.mock.calls[0][0] as { + agentMessages: Record }>>; + }; + const msg = agentMessages["ws-1"][0]; + expect(msg.attachments).toEqual([att]); + }); + + it("appends an attachments-only message (empty content) when at least one attachment present", () => { + // Regression: previously the AGENT_MESSAGE handler short-circuited on + // empty `message`, dropping a notify whose intent was "here's the + // file" with no caption. The fix renders the bubble whenever EITHER + // text or attachments are present. + const node = makeNode("ws-1"); + const { get, set } = makeStore([node], [], null, {}); + + handleCanvasEvent( + makeMsg({ + event: "AGENT_MESSAGE", + workspace_id: "ws-1", + payload: { + message: "", + attachments: [{ uri: "workspace:/x.txt", name: "x.txt" }], + }, + }), + get, + set, + ); + + expect(set).toHaveBeenCalledOnce(); + const { agentMessages } = set.mock.calls[0][0] as { + agentMessages: Record>; + }; + expect(agentMessages["ws-1"]).toHaveLength(1); + expect(agentMessages["ws-1"][0].content).toBe(""); + expect(agentMessages["ws-1"][0].attachments).toHaveLength(1); + }); + + it("filters out attachments with empty uri or name (defence-in-depth for missing gin `dive`)", () => { + // Server-side per-element validation rejects empty uri/name, but the + // canvas defence-in-depth filter exists because the broadcast path + // skips that handler — a malformed broadcast (or a future regression) + // could still emit empty entries. Drop them rather than rendering + // blank/broken chips. + const node = makeNode("ws-1"); + const { get, set } = makeStore([node], [], null, {}); + + handleCanvasEvent( + makeMsg({ + event: "AGENT_MESSAGE", + workspace_id: "ws-1", + payload: { + message: "ok", + attachments: [ + { uri: "workspace:/good.txt", name: "good.txt" }, + { uri: "", name: "missing-uri" }, + { uri: "workspace:/missing-name", name: "" }, + { uri: "workspace:/wrong-types", name: 42 }, // non-string name + ], + }, + }), + get, + set, + ); + + const { agentMessages } = set.mock.calls[0][0] as { + agentMessages: Record }>>; + }; + const atts = agentMessages["ws-1"][0].attachments!; + expect(atts).toHaveLength(1); + expect(atts[0].name).toBe("good.txt"); + }); + + it("ignores non-array attachments payloads", () => { + const node = makeNode("ws-1"); + const { get, set } = makeStore([node], [], null, {}); + + handleCanvasEvent( + makeMsg({ + event: "AGENT_MESSAGE", + workspace_id: "ws-1", + payload: { message: "hi", attachments: "not-an-array" }, + }), + get, + set, + ); + + const { agentMessages } = set.mock.calls[0][0] as { + agentMessages: Record>; + }; + expect(agentMessages["ws-1"][0].content).toBe("hi"); + // No attachments key when input was malformed (rather than [] which + // would render an empty "0 files" header in some chat UIs). + expect("attachments" in agentMessages["ws-1"][0]).toBe(false); + }); + + it("is a no-op when both message and attachments are empty", () => { + const node = makeNode("ws-1"); + const { get, set } = makeStore([node]); + + handleCanvasEvent( + makeMsg({ + event: "AGENT_MESSAGE", + workspace_id: "ws-1", + payload: { message: "", attachments: [] }, + }), + get, + set, + ); + + expect(set).not.toHaveBeenCalled(); + }); }); // --------------------------------------------------------------------------- diff --git a/tests/e2e/test_notify_attachments_e2e.sh b/tests/e2e/test_notify_attachments_e2e.sh new file mode 100755 index 00000000..5999ce5b --- /dev/null +++ b/tests/e2e/test_notify_attachments_e2e.sh @@ -0,0 +1,220 @@ +#!/usr/bin/env bash +# E2E test: agent → user notify with attachments (PR #2130). +# +# Exercises the wire contract that workspace/a2a_tools.tool_send_message_to_user +# uses to push files into the canvas chat. Pure platform test — no workspace +# container or LLM key required, so it runs against a bare workspace-server +# in <2s and pins the contract that real runtimes depend on. +# +# What this proves: +# 1. POST /notify (no attachments) persists a chat row that survives reload. +# 2. POST /notify with attachments persists parts[].kind=file in the SAME +# shape extractFilesFromTask reads → chips re-render after reload. +# 3. Per-element attachment validation rejects empty uri/name (regression +# for the bug where gin's binding:"required" on slice elements was a +# no-op without `dive` — see activity.go:299-306). +# 4. Empty `attachments: []` does NOT inject a stray `parts` key (would +# otherwise render a "0 files" header in the canvas). +# 5. Real /chat/uploads → /notify chain round-trips the URI verbatim. +# +# Usage: tests/e2e/test_notify_attachments_e2e.sh +# Prereqs: workspace-server on http://localhost:8080, MOLECULE_ENV != production + +set -euo pipefail + +source "$(dirname "$0")/_lib.sh" + +PASS=0 +FAIL=0 +WSID="" + +cleanup() { + if [ -n "$WSID" ]; then + curl -s -X DELETE "$BASE/workspaces/$WSID?confirm=true" > /dev/null || true + fi +} +trap cleanup EXIT + +assert() { + local label="$1" + local actual="$2" + local expected="$3" + if [ "$actual" = "$expected" ]; then + echo " PASS — $label" + PASS=$((PASS+1)) + else + echo " FAIL — $label" + echo " expected: $expected" + echo " actual: $actual" + FAIL=$((FAIL+1)) + fi +} + +assert_contains() { + local label="$1" + local haystack="$2" + local needle="$3" + if echo "$haystack" | grep -qF "$needle"; then + echo " PASS — $label" + PASS=$((PASS+1)) + else + echo " FAIL — $label" + echo " haystack: $haystack" + echo " needle: $needle" + FAIL=$((FAIL+1)) + fi +} + +echo "=== Setup ===" +R=$(curl -s -X POST "$BASE/workspaces" -H "Content-Type: application/json" \ + -d '{"name":"Notify E2E","tier":1}') +WSID=$(echo "$R" | python3 -c 'import json,sys;print(json.load(sys.stdin)["id"])' 2>/dev/null || true) +[ -n "$WSID" ] || { echo "Failed to create workspace: $R"; exit 1; } +echo "Created workspace $WSID" + +# Mint a bearer token so the wsAuth-grouped endpoints (notify, activity, +# chat/uploads) accept us. Local dev mode skips auth, but CI enforces it +# — so we always send the header to keep the test portable. The +# admin/test-token endpoint is only enabled when MOLECULE_ENV != production. +TOKEN=$(e2e_mint_test_token "$WSID") +[ -n "$TOKEN" ] || { echo "Failed to mint test token"; exit 1; } +AUTH="Authorization: Bearer $TOKEN" + +echo "" +echo "=== Test 1: notify without attachments persists row ===" +CODE=$(curl -s -o /tmp/notify1.json -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" \ + -d '{"message":"Working on it"}') +assert "POST /notify (text only) returns 200" "$CODE" "200" + +# Read it back via /activity. The notify handler writes a2a_receive, +# method=notify, response_body.result=. +# Notify writes source_id=NULL → use source=canvas (matches the chat +# panel's history loader filter). source=agent would correctly hide it. +ACT=$(curl -s -H "$AUTH" "$BASE/workspaces/$WSID/activity?source=canvas&limit=10") +ROW=$(echo "$ACT" | python3 -c ' +import json, sys +rows = json.load(sys.stdin) or [] +for r in rows: + if r.get("method") == "notify": + print(json.dumps(r)) + break +') +[ -n "$ROW" ] || { echo " FAIL — could not find notify row in activity"; FAIL=$((FAIL+1)); } + +if [ -n "$ROW" ]; then + RESULT=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["result"])') + assert "persisted response_body.result matches message" "$RESULT" "Working on it" + PARTS=$(echo "$ROW" | python3 -c 'import json,sys;b=json.load(sys.stdin)["response_body"];print("parts" in b)') + assert "no stray parts[] when message has no attachments" "$PARTS" "False" +fi + +echo "" +echo "=== Test 2: notify with attachments persists parts[].kind=file ===" +CODE=$(curl -s -o /tmp/notify2.json -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" \ + -d '{ + "message": "Done — see attached.", + "attachments": [ + {"uri":"workspace:/tmp/build-output.zip","name":"build-output.zip","mimeType":"application/zip","size":12345} + ] + }') +assert "POST /notify (with attachment) returns 200" "$CODE" "200" + +ACT=$(curl -s -H "$AUTH" "$BASE/workspaces/$WSID/activity?source=canvas&limit=10") +ROW=$(echo "$ACT" | python3 -c ' +import json, sys +rows = json.load(sys.stdin) or [] +# Most recent matching notify row +for r in rows: + rb = r.get("response_body") or {} + if r.get("method") == "notify" and "parts" in rb: + print(json.dumps(r)) + break +') +[ -n "$ROW" ] || { echo " FAIL — could not find notify-with-attachments row"; FAIL=$((FAIL+1)); } + +if [ -n "$ROW" ]; then + KIND=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["parts"][0]["kind"])') + URI=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["parts"][0]["file"]["uri"])') + NAME=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["parts"][0]["file"]["name"])') + MIME=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["parts"][0]["file"]["mimeType"])') + SIZE=$(echo "$ROW" | python3 -c 'import json,sys;print(json.load(sys.stdin)["response_body"]["parts"][0]["file"]["size"])') + assert "parts[0].kind == file" "$KIND" "file" + assert "parts[0].file.uri preserved" "$URI" "workspace:/tmp/build-output.zip" + assert "parts[0].file.name preserved" "$NAME" "build-output.zip" + assert "parts[0].file.mimeType preserved" "$MIME" "application/zip" + assert "parts[0].file.size preserved" "$SIZE" "12345" +fi + +echo "" +echo "=== Test 3: per-element validation rejects empty uri/name ===" +# Critical regression: gin's binding:"required" on slice elements is a no-op +# without `dive`. activity.go:299 explicitly loops and rejects. Keep in lock- +# step here so a future refactor that drops the loop fails this test. +CODE=$(curl -s -o /tmp/notify3.json -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" \ + -d '{"message":"x","attachments":[{"uri":"","name":""}]}') +assert "empty uri/name attachment is 400" "$CODE" "400" +ERR=$(cat /tmp/notify3.json | python3 -c 'import json,sys;print(json.load(sys.stdin).get("error",""))') +assert_contains "error mentions attachment[0]" "$ERR" "attachment[0]" + +CODE=$(curl -s -o /tmp/notify3b.json -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" \ + -d '{"message":"x","attachments":[{"uri":"workspace:/ok.txt","name":"ok.txt"},{"uri":"","name":"bad"}]}') +assert "second-element empty uri rejects whole call" "$CODE" "400" +ERR=$(cat /tmp/notify3b.json | python3 -c 'import json,sys;print(json.load(sys.stdin).get("error",""))') +assert_contains "error mentions attachment[1]" "$ERR" "attachment[1]" + +echo "" +echo "=== Test 4: missing message field rejected ===" +CODE=$(curl -s -o /dev/null -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" -d '{}') +assert "POST /notify with no message returns 400" "$CODE" "400" + +echo "" +echo "=== Test 5: real /chat/uploads → /notify round-trip ===" +TMPF=$(mktemp -t notify-e2e-XXXX.txt) +echo "round-trip-marker-$(date +%s)" > "$TMPF" +UP=$(curl -s -X POST "$BASE/workspaces/$WSID/chat/uploads" -H "$AUTH" -F "files=@$TMPF") +URI=$(echo "$UP" | python3 -c ' +import json,sys +try: + print(json.load(sys.stdin)["files"][0]["uri"]) +except Exception: + pass +') +NAME=$(basename "$TMPF") + +if [ -z "$URI" ]; then + # /chat/uploads requires a running container in some configs. Skip the + # round-trip if the platform refused — the synthetic-URI tests above + # already pin the wire contract. + echo " SKIP — /chat/uploads not available in this env (response: $UP)" +else + CODE=$(curl -s -o /dev/null -w "%{http_code}" -X POST "$BASE/workspaces/$WSID/notify" \ + -H "Content-Type: application/json" -H "$AUTH" \ + -d "{\"message\":\"see file\",\"attachments\":[{\"uri\":\"$URI\",\"name\":\"$NAME\"}]}") + assert "uploaded URI round-trips through notify" "$CODE" "200" + + ACT=$(curl -s -H "$AUTH" "$BASE/workspaces/$WSID/activity?source=canvas&limit=10") + STORED_URI=$(echo "$ACT" | python3 -c " +import json, sys +rows = json.load(sys.stdin) or [] +for r in rows: + rb = r.get('response_body') or {} + parts = rb.get('parts') or [] + for p in parts: + f = p.get('file') or {} + if f.get('name') == '$NAME': + print(f.get('uri','')) + sys.exit(0) +") + assert "stored URI matches uploaded URI" "$STORED_URI" "$URI" +fi + +rm -f "$TMPF" + +echo "" +echo "=== Results: $PASS passed, $FAIL failed ===" +[ "$FAIL" -eq 0 ]