feat(spec): add SessionEvent entity for comprehensive AG-UI event stream - #250
Conversation
Split SessionMessage (high-level conversation) from new SessionEvent (comprehensive AG-UI event stream with compression). Adds Events API endpoints, gRPC protocol, storage model, compression strategy, and migration plan per platform PR #1600. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
🤖 Amber Review — PR #250: SessionEvent data model spec
The split between Messages API (human-readable) and Events API (complete audit trail) is the right architectural call. The compression strategy is well-designed — context-aware accumulation with explicit flush triggers and gap preservation is correct. The migration plan is safe (new table, no existing table changes, backward-compatible). CI is all green.
Three issues need addressing before this can merge — one is a spec correctness bug that would cause the wrong table to be modified.
Critical — Wrong table attribution for completed_at / event_count
In the Metadata Preservation subsection of the compression section:
completed_at— timestamp of the last event (new field onSessionMessage)
event_count— number of raw events compressed into this row (new field)
Both completed_at and event_count are fields on SessionEvent (the session_events table), not SessionMessage. The migration plan itself correctly states "No schema changes required for session_messages table." This attribution error will cause an implementor to add compression fields to the wrong table.
Please change "new field on SessionMessage" → "new field on SessionEvent" and add "(new field)" to event_count to make both consistent.
Major — ListSessionEvents gRPC RPC is in the coverage matrix but not in the protocol section
The coverage matrix references:
🔲 ListSessionEvents,PushSessionEvent(gRPC)
But the gRPC Protocol section only defines PushSessionEvent — there is no ListSessionEvents RPC signature. An implementor building the gRPC server will look for the proto definition and not find it.
Please add the ListSessionEvents RPC to the protocol section (request/response message definitions, pagination parameters). If the intent is REST-only for history reads (REST GET /events/history with no gRPC equivalent), remove ListSessionEvents from the coverage matrix and add a note explaining why.
Major — No payload size constraint for TOOL_CALL_RESULT
The compression rules table marks TOOL_CALL_RESULT as "Never compressed — stored as individual events." Tool results can be very large — file contents, command output, grep results regularly exceed 50–100KB. Storing these verbatim in a TEXT column with no size constraint is a storage concern at scale.
The spec should define one of:
- A max payload size (e.g., 64KB) with truncation at the gRPC client before
PushSessionEvent - A reference to external storage (e.g., truncate in DB row, full result stored in S3/PVC) for oversized results
- An explicit acknowledgment that payload size is unbounded and left to the implementation (if intentional)
Without this, different implementations will make different choices and the DB schema won't enforce consistency.
Minor — Previous-2: is a non-standard frontmatter key
The existing convention uses **Previous:** for the prior entry. Adding **Previous-2:** creates a non-standard key that won't render consistently in docs tooling that parses this header. A cleaner pattern is a bulleted changelog list or simply dropping the oldest entry (which is already in git history).
Minor — Seq gap terminology
"Sequence numbers preserve gaps after compression (11 → 14) to avoid renumbering all subsequent events."
The gaps aren't after compression — they're gaps that result from the runner never writing seq 12 and 13 in the first place (runner compresses before push). Worth a one-word tweak: "...preserve gaps resulting from compression..." to avoid implying the DB re-sequences on write.
What's Working Well
- ✅ Clean split between Messages API (6 simplified event types) and Events API (33 AG-UI types) — right abstraction boundary
- ✅ Compression strategy is correct: context key definition, flush triggers, accumulation buffer model
- ✅
UNIQUE(session_id, seq)constraint is the right enforcement for monotonic ordering - ✅
completed_atindex supports time-range queries correctly (the WHERE clause in the storage model section is accurate) - ✅ Backward compatibility story is solid: legacy runners continue working, API server accepts both formats
- ✅ Dual push pattern (
PushSessionMessage+PushSessionEvent) is the right way to maintain both APIs - ✅ Coverage matrix clearly distinguishes implemented vs not-yet (🔲) — useful for sprint planning
- ✅ Start/End pairing documentation and example event sequence are clear
Confidence: High — the architectural direction is correct. The three issues above are spec correctness gaps, not design problems.
— Amber
jsell-rh
left a comment
There was a problem hiding this comment.
🤖 Amber — inline: wrong table attribution
SessionMessage should be SessionEvent here — completed_at and event_count are new fields on the session_events table, not session_messages. The migration plan below correctly confirms no schema changes to session_messages. This would send an implementor to the wrong table.
— Amber
Summary
SessionMessage(high-level conversation / Messages API) from newSessionEvent(comprehensive AG-UI event stream with compression) indata-model.spec.md/events/history), gRPC protocol (PushSessionEvent), storage model (session_eventstable with compression fields), context-aware compression strategy, migration plan, and updated coverage matrixTest plan
🤖 Generated with Claude Code