2 Commits

Author	SHA1	Message	Date
claude-ceo-assistant (Claude Opus 4.7 on Hongming's MacBook)	75a72bf5a2	feat(canvas/chat-server): canvas consumes /chat-history + server-side row-aware reverse (RFC #2945 PR-C-2) ... Closes the SSOT story shipped in PR-C/D: canvas now consumes the typed /chat-history endpoint instead of /activity?type=a2a_receive, and the server emits messages in display-ready chronological order so the client doesn't have to re-order them. ## Canvas (consumer migration) - loadMessagesFromDB swaps from /activity to /chat-history. - Drops type=a2a_receive + source=canvas params (server applies the filter centrally now). - Drops [...activities].reverse() — wire is already display-ready. - Drops the local INTERNAL_SELF_MESSAGE_PREFIXES constant + isInternalSelfMessage helper. Server-side IsInternalSelfMessage applies the same predicate before emitting rows. - Drops the activityRowToMessages + ActivityRowForHydration imports from historyHydration.ts. The TS parser stays in tree because message-parser.ts is still load-bearing for live A2A WebSocket messages (ChatTab.tsx:805, AgentCommsPanel.tsx, canvas-events.ts). ## Server (row-aware wire-order fix) The pre-PR-C-2 client did `[...activities].reverse()` over ROWS, then flattened each row into [user, agent] messages. The reversal was ROW-aware. After PR-C/D, the server returned a flat ChatMessage slice in `ORDER BY created_at DESC` order, with [user, agent] within each row. A naive client-side flat reverse would FLIP each pair (agent before user at same timestamp). Two ways to fix it: A) Server emits oldest-first within page; canvas does NOT reverse. B) Canvas does row-aware reversal (group by timestamp, reverse). Option A is cleaner — server owns the wire-order responsibility, every client trusts `for m of messages` to render chronologically. Server adds reverseRowChunks() that: 1. Groups consecutive same-Timestamp messages into row chunks (1-2 messages per row). 2. Reverses the chunk order (newest-row-first → oldest-row-first). 3. Flattens. Within-chunk [user, agent] order is preserved. Single-message rows (agent reply not yet recorded, attachments-only user upload) collapse to 1-element chunks and reverse correctly too. ## Tests Server: 3 new unit tests on reverseRowChunks (paired across rows, single-message rows, empty input) + 1 sqlmock integration test on List() that drives the full SQL → reverse → wire path. Mutation-tested: removed `messages = reverseRowChunks(messages)` from List(), confirmed the integration test fires red with all 4 misordered indices flagged. Restored, all 25 messagestore tests + 9 chat-history handler tests green. Canvas: 8 lazyHistory pagination tests refactored to mock /chat-history (not /activity) and assert against the new wire shape ({messages, reached_end} not raw activity rows). All 1389/1389 vitest tests green; tsc --noEmit clean. ## Three weakest spots (hostile-reviewer self-pass) 1. reverseRowChunks groups by Timestamp string equality. If two distinct rows had the SAME timestamp (legitimately possible at sub- millisecond granularity), the algorithm would treat them as one chunk and not reverse them relative to each other. Mitigated: activity_logs.created_at uses microsecond resolution; concurrent inserts at exact-same microsecond are vanishingly rare. If a collision happens, the within-chunk order is whatever the SQL returned — both rows render at the same timestamp, no user-visible misordering. 2. The pre-existing TS parser files (historyHydration.ts + message-parser.ts) stay in tree. historyHydration.ts is now dead code (no consumers post-migration); deletion is parked as a follow- up after a one-week observation window confirms no live-message consumer reaches it. 3. canvas's loadMessagesFromDB returns `resp.messages ?? []`. If the server were ever to return `null` instead of `[]` (it currently doesn't — handler defensively coerces nil to []), the nullish coalesce keeps the canvas from crashing. A stricter wire schema would assert the never-null invariant; for today's pragmatic safety, the ?? is enough. ## Security review - Untrusted input? Same as PR-C — agent JSON parsed defensively in the messagestore parser. No new exposure. - Trust boundary? Same. Canvas → /chat-history → wsAuth → messagestore. - Output sanitization? Plain text + opaque attachment URIs as before. No security-relevant changes beyond what /chat-history already exposes via PR-C. Considered, not skipped. ## Versioning / backwards compat - /activity endpoint unchanged. - /chat-history endpoint shape unchanged (still {messages, reached_end}); only the wire ORDER within a page changed (newest-first row → oldest- first row). Canvas is the only consumer in tree; no API consumers depend on the previous order. - canvas's loadMessagesFromDB call signature unchanged — internal refactor. 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-05-06 16:55:00 -07:00
Hongming Wang	a33c879017	feat(messagestore): MessageStore interface + Postgres impl (RFC #2945 PR-D) ... Closes #3026. Final piece of RFC #2945. ## What's new New package internal/messagestore/ holds: - MessageStore interface — single read-side contract operators implement to plug in alternative chat-history backends. - ChatMessage / ChatAttachment / ListOptions types — canonical data shapes returned by any impl, mirrors canvas's TS ChatMessage. - PostgresMessageStore — platform-default impl wrapping the activity_logs query + A2A-envelope parser ported in PR-C. Behavior is byte-identical to the pre-PR-D handler. ## What moves The activity_logs query, the parser (activityRowToChatMessages, extractRequestText, extractChatResponseText, extractFilesFromTask, etc.), and the internal-self-message predicate all migrate from internal/handlers/chat_history.go into the new package. handlers/ chat_history.go becomes a thin HTTP-shape adapter: parse query params → store.List(ctx, workspaceID, opts) → emit JSON Compile-time interface assertion in postgres_store.go catches future drift if the interface evolves and the impl falls behind. ## Why this PR OSS operators wanting to: - Tier hot/warm/cold storage (recent in Postgres, archival in S3) - Use a vector store with hybrid search (Pinecone, Weaviate) - Run an in-memory store for ephemeral test environments - Federate history across regions …had no extension point — they'd have to fork the handler. This PR makes that a constructor swap at router.go. ## Tests Parser-level (22 tests, MOVED to internal/messagestore/postgres_ store_test.go): every TS test case in canvas/src/components/tabs/chat/__tests__/historyHydration.test.ts has a Go counterpart. Timestamp preservation, user/agent extraction, internal-self filter, role decision (status=error vs agent-error prefix), v0/v1 file shapes, malformed JSON resilience. Handler-level (9 NEW tests in internal/handlers/chat_history_test.go): thin adapter coverage using a fake MessageStore. UUID validation, before_ts RFC3339 validation, default limit, max-limit clamp, invalid-limit fallback, before_ts passthrough, empty-array (not null) JSON shape, attachment shape preservation, store-error → 502 mapping. Compile-time interface conformance: PostgresMessageStore satisfies MessageStore, fakeStore (test fake) satisfies MessageStore. Mutation-tested. Removed UUID validation in the handler; confirmed TestChatHistoryHandler_RejectsNonUUIDWorkspaceID fires red (status 200 instead of 400, non-UUID reaches the store). Restored, all green. Full handlers + messagestore + router test runs green; full repo go test ./... green. ## SSOT decision ChatMessage / ChatAttachment / parser / DB query all live in internal/messagestore/ ONLY. handlers/chat_history.go imports the package and uses the types via messagestore.ChatMessage etc. — no re-declaration anywhere. ## Three weakest spots (hostile-reviewer self-pass) 1. The internal-self prefix list (Delegation results are ready...) is a package var in messagestore/postgres_store.go. A future impl that wants to override the predicate must reach into the package to use IsInternalSelfMessage or define its own. Acceptable: the predicate is part of the contract; if an impl wants different semantics it owns that decision explicitly. 2. ListOptions has Limit + BeforeTS + HasBefore; future paging needs (after_ts, peer_id filter, role filter) require additive struct field additions, which is a soft API break for any impl that handles ListOptions positionally. Mitigated by Go's struct-literal convention (named fields by default); also flagged in the interface comment for impl authors. 3. The handler does NOT log when a store returns an error — it just maps to 502. An impl that wants to surface its error class up the stack can't, today. If/when an impl needs that, the interface can add a typed-error contract in a follow-up. Today's coverage is sufficient: most ops issues land in the store impl's own logs. ## Security review - Untrusted input? Same as PR-C — agent-emitted JSON parsed defensively. New fakeStore in tests can't reach production. - Trust boundary? Same. Interface lives BEHIND wsAuth; impls only see workspace IDs already authenticated. - Auth/authz? Inherited from handler; the interface doesn't authenticate. - PII / secrets in logs? Documented in the interface contract: impls MUST NOT log full message bodies / attachment URIs. The Postgres impl logs nothing on the happy path. - Output sanitization? Same plain-text + opaque-URI surface as PR-C. Canvas validates attachment-URI schemes. No security-relevant changes beyond what /chat-history already exposes via PR-C. Considered, not skipped. ## Versioning / backwards compat - New internal package. Zero public API change. - Single caller site in router.go updated (one-line constructor change). NewChatHistoryHandler() → NewChatHistoryHandler(store). - No schema change, no migration. - Existing /chat-history endpoint unchanged on the wire — clients don't notice the refactor. ## Phasing This is the final RFC #2945 piece. Follow-ups parked: - PR-C-2 (canvas migration): swap canvas loadMessagesFromDB to call /chat-history instead of /activity. Independent of this PR; blocked only by canvas team's calendar. - Sample alternative impls (S3, in-memory) for OSS docs: separate PR when the first OSS consumer materializes; demonstration code untested against a real workload is anti-pattern. 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-05-05 23:38:14 -07:00