Per user request: audit all similar tools + write comprehensive tests
including E2E for the persistence-of-AGENT_MESSAGE-broadcasts contract.
Audit (all BroadcastOnly call sites in workspace-server/internal/):
| Site | Event | Persisted? | Notes |
|---|---|:---:|---|
| a2a_proxy_helpers.go:275 | A2A_RESPONSE | ✓ | LogActivity above |
| activity.go:486 (Notify) | AGENT_MESSAGE | ✓ | INSERT line 535 |
| activity.go:701 (LogActivity) | ACTIVITY_LOGGED | ✓ | self-emits inside DB write |
| mcp_tools.go:341 (toolSendMessageToUser) | AGENT_MESSAGE | ✓ NEW (this PR) |
| registry.go:575 | TASK_UPDATED | N/A | transient progress, not chat |
| registry.go:596 | WORKSPACE_HEARTBEAT | N/A | infra ping, not chat |
Only one chat-bearing broadcast was missing persistence (the just-
fixed mcp bridge path). No other regressions found.
Tests added (4 new, total 5 send_message_to_user tests):
1. TestAgentMessageBroadcastsArePersisted — AST gate that walks every
non-test .go in the package, finds funcs that BroadcastOnly with
"AGENT_MESSAGE", asserts each ALSO contains an
"INSERT INTO activity_logs". Forward-looking regression block:
any future chat tool that broadcasts without persisting fails the
test with a clear file:func diagnostic. Mutation-tested locally:
removing the INSERT block from toolSendMessageToUser reliably
produces the expected failure.
2. TestMCPHandler_SendMessageToUser_DBErrorLogsAndStill200s — pins
the "best-effort persistence" contract. DB INSERT failures must
NOT abort the tool response (the WS broadcast already succeeded;
retrying would double-render in the live chat). Matches /notify.
3. TestMCPHandler_SendMessageToUser_ResponseBodyShape — pins the
exact `{"result": "<message>"}` JSON shape stored in
response_body. The canvas hydrater (extractResponseText in
historyHydration.ts) reads body.result; any drift here silently
breaks chat history without failing the INSERT. Per memory
feedback_assert_exact_not_substring.md, asserts the literal JSON
shape, not a substring.
4. TestMCPHandler_SendMessageToUser_PersistsToActivityLog (existing,
from previous commit) — pins INSERT shape with regex on
'a2a_receive' + 'notify' literals.
5. TestMCPHandler_SendMessageToUser_Blocked_WhenEnvNotSet (existing)
— env-gate aborts before DB.
Test fixture cleanup: newMCPHandler now uses newTestBroadcaster (real
ws.Hub) instead of events.NewBroadcaster(nil) — the latter nil-panics
inside hub.Broadcast on the AGENT_MESSAGE path. Same broadcaster
shape every other handler test uses.
E2E note: the AST gate is the strongest forward-looking guarantee.
A real-DB integration test would add value for CI but is largely
duplicative of the sqlmock contract tests above (sqlmock pins SQL
shape with much faster feedback). Left as a future enhancement when
the handlers Postgres-integration suite extends MCP coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
899c53550d