internal/service/chatbridge/monitor_state.go (1)

55-65: Nil destination entries remain in the map after normalization.

When destination == nil, the loop continues without removing or initializing the entry. This leaves nil pointers in the Destinations map, which could cause nil-pointer dereferences in code that iterates over the map expecting initialized entries.

Consider either removing nil entries or initializing them:

♻️ Option: Initialize nil destination entries

 for _, destination := range s.Destinations {
   if destination == nil {
-    continue
+    // Note: Can't fix in-place during range; consider using a key-based loop
+    // or removing nil entries after iteration
   }
   if destination.Pending == nil {
     destination.Pending = make(map[string]NotificationEvent)
   }
   if destination.Delivered == nil {
     destination.Delivered = make(map[string]time.Time)
   }
 }

Alternative approach using keys:

+for key, destination := range s.Destinations {
+  if destination == nil {
+    s.Destinations[key] = &notificationDestinationState{
+      Pending:   make(map[string]NotificationEvent),
+      Delivered: make(map[string]time.Time),
+    }
+    continue
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/chatbridge/monitor_state.go` around lines 55 - 65, The loop
over s.Destinations leaves nil pointer entries in the map; change it to iterate
with key, destination := range s.Destinations, and when destination == nil call
delete(s.Destinations, key) to remove that entry, otherwise ensure
destination.Pending and destination.Delivered are initialized
(make(map[string]NotificationEvent) and make(map[string]time.Time)
respectively); this ensures s.Destinations contains no nil pointers and all
remaining entries have initialized Pending and Delivered maps.

internal/service/eventstore/eventstore.go (1)

210-216: Shallow clone shares nested mutable values.

cloneData uses maps.Copy, which is a shallow clone. If data contains nested maps or slices, those are shared with the original. This is typically fine if callers don't mutate data after calling NewDAGRunEvent, but could lead to subtle bugs if they do.

If event data immutability is guaranteed by convention, this is acceptable. Otherwise, consider a deep-clone or documenting the ownership transfer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/eventstore/eventstore.go` around lines 210 - 216, The
current cloneData call performs a shallow copy (via maps.Copy) so nested
maps/slices remain shared; update cloneData to perform a deep clone of
map[string]any (recursively cloning nested maps and slices) and use that deep
clone where data is assigned before attaching the notification snapshot (i.e.,
keep the existing usage around cloneData in the block that touches
newNotificationStatusSnapshot and notificationStatusSnapshotDataKey), or
alternatively document in NewDAGRunEvent that ownership is transferred and
callers must not mutate the passed-in data; pick one approach and apply
consistently.

internal/service/chatbridge/monitor_test.go (1)

252-256: Replace fixed sleep with an eventual assertion to reduce flakiness.

Line 252 uses a fixed time.Sleep(80 * time.Millisecond), which can intermittently fail on slower CI runners. Prefer require.Eventually on the expected monitor behavior.

Suggested change

-	time.Sleep(80 * time.Millisecond)
-
-	headCalls, readCalls := store.stats()
-	assert.Greater(t, headCalls, 0)
-	assert.Equal(t, 0, readCalls)
+	require.Eventually(t, func() bool {
+		headCalls, readCalls := store.stats()
+		return headCalls > 0 && readCalls == 0
+	}, time.Second, 10*time.Millisecond)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/chatbridge/monitor_test.go` around lines 252 - 256, Replace
the fixed time.Sleep with a polling assertion: call require.Eventually (from
testify/require) to repeatedly invoke store.stats() in a closure and assert that
headCalls > 0 and readCalls == 0, providing a reasonable timeout (e.g.,
200–500ms) and tick interval (e.g., 10–20ms); locate the sleep and the following
store.stats() call in monitor_test.go and replace them with this eventual check
so the test waits until the monitor behavior is observed instead of sleeping a
fixed duration.

ui/src/pages/event-logs/index.tsx (1)

273-277: Effect dependencies may cause extra updates.

The lastUpdatedAt update effect has dependencies [currentNextCursor, firstEntryID, hasHeadResponse, lastEntryID] but only checks hasHeadResponse before updating. The additional dependencies (firstEntryID, lastEntryID, currentNextCursor) will trigger the effect but won't change the outcome since hasHeadResponse is the gating condition.

This is harmless (just sets the same timestamp) but could be simplified:

♻️ Simplified effect dependencies

   React.useEffect(() => {
     if (hasHeadResponse) {
       setLastUpdatedAt(new Date());
     }
-  }, [currentNextCursor, firstEntryID, hasHeadResponse, lastEntryID]);
+  }, [hasHeadResponse, data]);

Using data as a dependency more directly reflects the intent: update timestamp when new data arrives.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/pages/event-logs/index.tsx` around lines 273 - 277, The effect that
updates lastUpdatedAt only checks hasHeadResponse but lists extra deps
(currentNextCursor, firstEntryID, lastEntryID) causing unnecessary runs; modify
the React.useEffect for setLastUpdatedAt so its dependency list reflects the
real trigger—either change the deps to [hasHeadResponse] or to the actual data
source (e.g., [data]) used to indicate new entries—and keep the body as: if
(hasHeadResponse) setLastUpdatedAt(new Date()); targeting the useEffect that
references setLastUpdatedAt and hasHeadResponse.

internal/service/chatbridge/monitor.go (1)

582-585: Timestamp comparison may be overly strict.

Line 582 checks if deliveredAt, ok := destState.Delivered[event.Key]; !ok || !deliveredAt.Equal(now). Since now is freshly captured at line 568, the !deliveredAt.Equal(now) condition will almost always be true for existing entries (they would have a different timestamp from a previous delivery).

This means re-marking an already-delivered event will update its timestamp. This is likely intentional (refresh the TTL for eviction), but the condition is confusing. Consider simplifying or adding a comment explaining the intent:

♻️ Clarify the intent

-			if deliveredAt, ok := destState.Delivered[event.Key]; !ok || !deliveredAt.Equal(now) {
+			// Update timestamp on each delivery to refresh TTL for eviction
+			if _, ok := destState.Delivered[event.Key]; !ok {
 				destState.Delivered[event.Key] = now
 				changed = true
 			}

Or if refreshing the timestamp is intentional:

+			// Always update timestamp to refresh eviction TTL
 			if deliveredAt, ok := destState.Delivered[event.Key]; !ok || !deliveredAt.Equal(now) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/chatbridge/monitor.go` around lines 582 - 585, The timestamp
check for destState.Delivered[event.Key] (using deliveredAt, ok, now) is
confusing because it always updates the timestamp; either clarify intent or
change behavior: if the goal is only to record first delivery, set
destState.Delivered[event.Key]=now and changed=true only when !ok; if the goal
is to refresh TTL on every touch, always set destState.Delivered[event.Key]=now
but add a clear comment above the block explaining “refreshing delivery
timestamp to extend eviction TTL” and decide whether to set changed=true on
refresh (keep or remove changed update accordingly). Ensure references to
deliveredAt, event.Key, now and changed are adjusted consistently.

internal/service/chatbridge/monitor_state_test.go (1)

506-520: Consider potential test flakiness from fixed sleeps.

The time.Sleep(100 * time.Millisecond) calls at lines 506 and 520 are used to wait for the monitor to attempt delivery and observe the save failure. While understandable for testing persistence failure scenarios, fixed sleeps can cause flakiness on slower CI systems.

Consider whether these could be replaced with more deterministic synchronization, such as waiting for a specific internal state or using a hook/callback mechanism in test mode. However, given the complexity of instrumenting save failure observation, the current approach is acceptable if the timeouts provide sufficient margin.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/chatbridge/monitor_state_test.go` around lines 506 - 520,
Replace the two fixed time.Sleep(100 * time.Millisecond) waits with
deterministic assertions to avoid CI flakiness: instead of the first sleep
before checking delivered, use require.Eventually (or a channel/hook) to assert
delivered remains empty for a short interval (checking mu and delivered), and
instead of the final sleep use require.Never or another require.Eventually that
confirms no additional deliveries occur after the expected "run-save-retry"
(using the same mu and delivered variables and the stateDir save-failure
scenario); this targets the checks around delivered/mu in monitor_state_test.go
and avoids arbitrary sleeps.

internal/service/eventstore/notifications.go (1)

184-193: Consider normalizing at the service boundary for API contract clarity. All current implementations handle CommittedOffsets safely (fileeventstore normalizes on return or immediately on entry), so this is not a bug fix. However, normalizing cursors in NotificationHeadCursor (line 192) and ReadNotificationEvents (line 203) before delegation would make the contract clearer and prevent future issues if custom reader implementations skip normalization.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/eventstore/notifications.go` around lines 184 - 193,
Normalize NotificationCursor.CommittedOffsets at the service boundary before
delegating to store readers: update Service.NotificationHeadCursor and
Service.ReadNotificationEvents to defensively normalize the cursor (e.g., ensure
CommittedOffsets is canonical/trimmed/validated) prior to calling
reader.NotificationHeadCursor(ctx) and reader.ReadNotificationEvents(ctx,
cursor) respectively so custom NotificationReader implementations can rely on a
normalized API contract; use the NotificationCursor/CommittedOffsets
normalization routine already used by fileeventstore to implement this change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes