Skip to content

fix(notifications): resolve agent-push dispatch review findings from #4627#4656

Merged
iscekic merged 1 commit into
mainfrom
fix/notification-dispatch-review
Jul 21, 2026
Merged

fix(notifications): resolve agent-push dispatch review findings from #4627#4656
iscekic merged 1 commit into
mainfrom
fix/notification-dispatch-review

Conversation

@iscekic

@iscekic iscekic commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #4627 addressing the two unresolved review comments from @jeanduplessis on NotificationChannelDO. Both were flagged but non-blocking, so #4627 merged approved; this PR resolves them. The agent-notification push path is still dark in production (its CLI companion Kilo-Org/kilocode#12392 is unapproved), so this lands ahead of the feature going live.

1. Concurrent-replay double-send (correctness)

Concurrent replays can send the same agent notification more than once. This writes pending before token lookup and Expo submission, but a request that observes pending is treated as immediately retryable…

The DO serializes turns, but a turn yields the isolate on every external await (presence RPC, Expo send). A replay of the same idempotency key can interleave while the first attempt is mid-flight, read the pending marker the first attempt left, treat it as a reusable retry slot, and submit a second OS push.

Fix: an in-memory in-flight claim (Set<string> keyed by idempotency key). A concurrent replay seeing a live claim returns duplicate; a sequential post-failure retry (claim already released) still reuses the storage pending slot exactly as before. New test drives two concurrent dispatches of the same key with a held-open Expo send and asserts exactly one submission.

2. PII in push-sink logs (privacy)

Push-sink mode logs the complete notification title, agent/user-controlled body, and data object… allows sensitive session content… to be retained in local, CI, or aggregated Worker logs.

The dev/E2E-only sink (PUSH_SINK_MODE=log) logged full title/body/data.

Fix: log content-free metadata only — sizes, data key names, and the fixed type discriminator. The sink still drives E2E to a delivered outcome (its actual purpose); no flow consumed the logged content. Test updated to assert the redacted shape and that no content values appear.

Verification

  • services/notifications: 142 tests pass (incl. new concurrent + redaction tests), tsgo clean, oxlint clean.

Two unresolved review comments from #4627 (jeanduplessis):

1. Concurrent-replay double-send: the DO yields the isolate on every external
   await (presence RPC, Expo send), so a replay of the same idempotency key
   could interleave, read the pending marker the first attempt left, treat it
   as a reusable retry slot, and submit a second OS push. Add an in-memory
   in-flight claim so a concurrent replay returns duplicate; sequential
   post-failure retries still reuse the storage pending slot.

2. PII in push-sink logs: the dev/E2E push sink logged the full title, body,
   and data object (agent/user-controlled content) into local/CI/Worker logs.
   Log content-free metadata instead (sizes, data key names, the type
   discriminator); the sink still drives E2E to a delivered outcome.
@iscekic
iscekic requested a review from jeanduplessis July 21, 2026 15:40
@kilo-code-bot

kilo-code-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Both review findings from #4627 are addressed correctly: the in-memory in-flight claim closes the concurrent-replay double-send window without a check/await race, and the push-sink log now records only content-free metadata (lengths, key names, a fixed schema-enum discriminator) with no PII.

Files Reviewed (2 files)
  • services/notifications/src/dos/NotificationChannelDO.ts
  • services/notifications/src/__tests__/dispatch-push.test.ts

Reviewed by claude-sonnet-5 · Input: 26 · Output: 7.8K · Cached: 582.1K

Review guidance: REVIEW.md from base branch main

@iscekic
iscekic merged commit 761ff5a into main Jul 21, 2026
15 checks passed
@iscekic
iscekic deleted the fix/notification-dispatch-review branch July 21, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants