feat(events-api): implement SessionEvent for compressed AG-UI event persistence - #251
Conversation
jsell-rh
left a comment
There was a problem hiding this comment.
🤖 Amber Review — PR #251: Events API full-stack implementation
This is a well-structured feature — the dual-stream architecture, the compressor design, and the migration with proper rollback are all solid. Three issues need fixing before merge.
Blockers
1. No-op string replace in event_compressor.py — data emitted under wrong event type
In EventCompressor.feed():
results.append(CompressedEvent(
event_type=event_type.replace("_END", "_END"), # no-op: replaces "_END" with "_END"
...
))This replaces "_END" with "_END" — no transformation happens. The intent appears to be emitting the compressed event under the _START event type (e.g., TEXT_MESSAGE_START → compressed as TEXT_MESSAGE_START), not the _END type. Without the correct type, consumers querying for specific event types will miss compressed events.
Fix: either event_type.replace("_END", "_START") to emit under the start type, or drop the replace entirely and use self._active.start_type (which is clearer). Verify the intended behavior against the spec.
2. event_handler.go — no RBAC permission check for ListEvents
ListEvents verifies the session exists via h.session.Get(ctx, id) but does not call requireProjectAccess (used in all other handlers in this file). A user who knows a session ID can list its full AG-UI event history without being in the owning project.
Compare with PushSessionEvent in grpc_handler.go, which correctly gates non-service callers:
if !middleware.IsServiceCaller(ctx) {
session, svcErr := h.service.Get(ctx, req.GetSessionId())
// ...
if err := requireProjectAccess(ctx, derefStr(session.ProjectId)); err != nil {
return nil, err
}
}ListEvents needs the same gate. This is a Blocker per Constitution Principle II (RBAC before resource access).
3. PR is a draft with all test plan items unchecked
Several items remain unverified (Kind cluster deploy, acpctl session events-history, end-to-end dual-push). Not blocking the review per se, but please mark this ready for review and check off verified items before requesting human merge.
Majors
4. Silent event drop in sqlEventService.Push at buffer capacity
select {
case ch <- evt:
default:
}When a subscriber's channel (cap 512) is full, the event is silently dropped. For a replay/history use case this is fine — the subscriber can catch up from the DB. But there's no log line, making it invisible when it happens. Consider a glog.Warningf on the default path so operations can see backpressure.
5. Unbounded subs map — no cleanup for stale session IDs
sqlEventService adds entries to s.subs[sessionID] on Subscribe and removes the channel on cancel. But the sessionID key itself is never deleted, even when its channel list becomes empty. On a high-throughput cluster this is a slow map leak. Add cleanup:
if len(s.subs[sessionID]) == 0 {
delete(s.subs, sessionID)
}6. Migration missing FK constraint on session_id
session_events.session_id has no REFERENCES sessions(id) constraint. Orphaned event rows (session deleted, events remain) are a data integrity concern. If intentional (e.g., soft-delete pattern), document it in the migration comment.
Minors
7. Seq field tagged gorm:"->" (read-only) but Insert relies on BIGSERIAL
gorm:"column:seq;->" makes GORM skip setting seq on write, which is correct — the DB auto-assigns it. But the Returning clause in Insert retrieves the generated seq. If GORM's Returning doesn't populate read-only fields, evt.Seq will remain 0 after insert. Verify this actually works with your GORM version.
8. Go SDK version bump noise
30 files changed, 20 of which are minor SDK version constant bumps (+2 -2 each). Consider separating SDK housekeeping into a distinct commit to keep the history readable.
What's Good
- ✅ No
panic()anywhere in the new code —fmt.Errorfwrapping throughout - ✅
requireProjectAccessin gRPC handler for non-service callers — correct pattern - ✅
EventCompressorflush-on-stream-end handles incomplete accumulations correctly - ✅ Migration has full rollback, proper index set,
BIGSERIALfor monotonic seq - ✅
Subscribe/cancel pattern withsync.Onceprevents double-close - ✅ OpenAPI spec has correct
401,403,404,500responses and pagination params - ✅ RBAC permissions added (
PermSessionEventList,PermSessionEventWatch) - ✅ Full-stack: proto, gRPC, REST, SDK, CLI, frontend hook all added in one PR
Confidence: High on blockers 1 and 2. Blocker 3 is process. Majors 4/5/6 are real but lower urgency than the RBAC gap.
— Amber
Add the Events API for persisted compressed AG-UI events, implementing
the dual-stream architecture from the data model spec. This enables
event replay and audit trail alongside the existing SessionMessage
conversation stream.
Components:
- BE: SessionEvent model, DAO, service (pub/sub), HTTP handler, migration,
RBAC permissions, route at GET /sessions/{id}/events/history
- gRPC: SessionEvent proto message, PushSessionEvent + WatchSessionEvents
RPCs with RBAC-aware handler implementations
- OpenAPI: SessionEvent schema, SessionEventList, /events/history endpoint
- SDK: Auto-generated SessionEvent types (Go/TS/Python), hand-written
ListEvents client method with full filter support
- CLI: `acpctl session events-history` command with --event-type,
--after-seq, --limit flags
- Runner: Context-aware event compressor accumulating START/CONTENT/END
fragments, PushSessionEvent gRPC client, dual-push middleware
- FE: DomainSessionEvent type, SessionEventsPort, fetch adapter with
mapper, useSessionEvents React Query hook, query keys
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
bcc68fd to
c11151e
Compare
|
🤖 Amber Follow-up — PR #251 (HEAD: c11151e) Checked in on the new commit. Three things still need attention before this can merge: Still open from prior review:
New issue in this commit:
No other concerns from me on this commit — the full analysis is in the prior review. Ping me once all three are addressed. — Amber |
jsell-rh
left a comment
There was a problem hiding this comment.
🤖 Amber Re-Review — PR #251 (HEAD: c11151e)
Checking the new squashed commit against my prior blockers. Two of three blockers remain open, and CI is now failing on lint.
Still Blocking
1. No-op string replace in event_compressor.py — unchanged
event_compressor.py line ~126:
results.append(CompressedEvent(
event_type=event_type.replace("_END", "_END"), # still a no-op
...
))This was in the original code and is in this commit unchanged. The replace of "_END" → "_END" does nothing. The emitted event type for a completed accumulation is the _END type (e.g., TEXT_MESSAGE_END) rather than the _START type that consumers would query on. Fix: use self._active.start_type directly, which is clearer and correct:
event_type=self._active.start_type,2. ListEvents in event_handler.go — no RBAC gate — unchanged
func (h *eventHandler) ListEvents(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
id := mux.Vars(r)["id"]
if _, err := h.session.Get(ctx, id); err != nil {
http.Error(w, "session not found", http.StatusNotFound)
return
}
// ... no project access checkAny authenticated user who knows a session ID can retrieve its full AG-UI event history. PushSessionEvent and WatchSessionEvents in grpc_handler.go both gate on requireProjectAccess for non-service callers — ListEvents needs the same. This is a Constitution Principle II violation.
3. Lint CI Gate is failing
Go Lint - API Server is red on this commit. Please fix and re-push before requesting review.
Resolved Since Prior Review
- ✅ PR is now based on a squashed single commit (much cleaner history)
- ✅ All other CI gates (build, tests, CodeQL) are green
- ✅ The gRPC handlers (
PushSessionEvent,WatchSessionEvents) have correct RBAC gating
Summary: Two security/correctness blockers plus a lint failure need to be addressed. Once lint is green and the RBAC gate + event_type fix are in place, I'm happy to re-review immediately — the rest of this feature is solid.
— Amber
Add 60 unit tests for the runner events pipeline (event_compressor, session_events_api, grpc_push_dual). Fix Kind overlay to enable Vertex AI inference by default with correct project/region config. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix no-op replace in event_compressor.py: use start_type for compressed event_type (consistent with flush path) - Add RBAC project access check to ListEvents HTTP handler - Regenerate OpenAPI client to fix codegen drift (SessionEvent model) - Update tests to match corrected event_type semantics 🤖 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 Re-Review — PR #251 (HEAD: 5804193)
All three blockers from my previous reviews are resolved. Approving.
Blockers — All Cleared ✅
1. event_compressor.py no-op replace — Fixed
# Before (no-op):
event_type=event_type.replace("_END", "_END"),
# After (correct):
event_type=self._active.start_type,start_type is set at accumulation start (e.g., TEXT_MESSAGE_START) so compressed events are now emitted under the right type. Tests updated to assert _START types — consistent with the flush path. ✅
2. ListEvents missing RBAC gate — Fixed
authResult := rbac.GetAuthResult(ctx)
if authResult == nil {
http.Error(w, "not authorized", http.StatusForbidden)
return
}
if !authResult.IsGlobalAdmin && !rbac.IsProjectAuthorized(authResult, projectID) {
http.Error(w, "not authorized", http.StatusForbidden)
return
}Project membership is now checked before returning event history. Matches the gRPC handler pattern. Constitution Principle II satisfied. ✅
3. Lint CI Gate — Green
Go Lint - API Server, Lint CI Gate, Unit Tests CI Gate, Ambient Runner Tests, API Server Integration Tests, CodeQL all passing on HEAD. ✅
CI Summary (HEAD: 5804193)
All required gates green. test-local-dev-simulation is still in-progress at time of review — based on the pattern across recent PRs this is a known infra flap, not a code defect.
The open Majors from my original review (#4 silent drop log, #5 subs map leak, #6 FK constraint) are real but not blockers for this PR. I'd suggest tracking them as follow-up issues — the feature is sound and safe to merge.
— Amber
|
Updating label to |
Summary
PushSessionEvent+WatchSessionEventsRPCs, OpenAPI spec, RBAC permissions,GET /sessions/{id}/events/historyendpoint_CONTENT/_ARGSfragments between_START/_ENDboundaries, dual-push middleware emitting to bothsession_messagesandsession_eventsListEventsclient method,acpctl session events-historycommand, hexagonal FE layer (domain type, port, adapter, React Query hook)USE_VERTEX=1,CLOUD_ML_REGION=us-east5,ANTHROPIC_VERTEX_PROJECT_ID)Test plan
go build ./...andgo vet ./...pass for api-server, CLI, SDKmake protoregenerates stubs cleanlymake generate-sdkproduces SessionEvent types in all 3 SDKs_session_events_api,event_compressor)tsc --noEmitshows no new errors (pre-existingambient-sdkmodule errors only)pytest tests/test_event_compressor.py tests/test_session_events_api.py tests/test_grpc_push_dual.py)panic()in Go code, noanytypes in TS, no hardcoded secretsLOCAL_IMAGES=trueand verify/events/historyendpoint returns compressed eventsacpctl session events-history <id>renders table outputsession_messagesandsession_eventsrows🤖 Generated with Claude Code