feat: add ack_events so a filtered drain can commit what it surfaced

[evansenter]

Closes #134.

649 tests (627 on main), lint and format clean.

The problem

A bounded consume cannot bound anything under a server-side filter. min_level filters the events returned while the cursor advances over the raw batch behind them — so "consume the N I just saw" advances past a different window than the peek showed. Events consumed but never surfaced.

That is why the dotfiles drain hook could not migrate off its client-side denylist (evansenter/dotfiles#328), defeating part of #129's purpose of one canonical noise policy.

The primitive

ack_events(session_id, cursor) sets the saved cursor to an id the caller already holds, so peek and ack name the same window by construction:

peek (non-consuming, filtered) → act → ack(peek.next_cursor)

Verified end to end against a live bus — a peek at min_level=actionable shows one event of two, acking its next_cursor marks both seen (the lifecycle noise included, which is the point of a server-side noise policy), and the next normal poll returns nothing:

1. peek  → ['REAL SIGNAL'] next_cursor: 3
2. ack   → Cursor acked: 1 → 3
3. poll  → No events
4. ahead → Error: Cursor 999999 is ahead of the newest event (3)                exit=1
5. back  → Error: Cursor 1 is behind the session's current position (3).        exit=1
6. dead  → Error: Session deleted                                               exit=1

Five refusals, two kinds

A silently wrong cursor loses events, so each of these fails loudly instead. They split into two groups, and the split is what a caller has to branch on:

Cursor refusals — all report cursor, the session's saved position, which is inert to re-ack:

• not an event id — Invalid cursor ...: expected an event id; where a jq -r artifact lands
• negative — parses, so it gets its own Invalid cursor ...: must not be negative
• ahead of the newest event — would mark events that don't exist yet as seen. Also reports tip under its own key, for a caller that means to clamp deliberately
• behind the current position — needs allow_rewind=true to replay on purpose

cursor is null for a session that has never acked. No concrete id is both inert and lossless there: "0" persists and flips the next resume from tip-relative to a full-history replay, and the tip is read at ack time, so a publish between the peek and the ack would make re-acking it commit events the session never saw. The honest answer is that such a session has no position to restore — recovery is the next_cursor the caller still holds from its own peek.

Session refusals — neither carries a cursor key at all, because there is no position to return to. Recovery is register_session:

• deleted session — reuses fix: reject get_events polls from soft-deleted sessions (#140) #142's contract rather than inventing a second one, so a drain hook gets the same shape and hint its polls already get
• never registered — a plain {error: "Session not found", session_id}

For the deleted case, _deleted_session_error grows a tool argument that names the caller in the warning line only. Sharing the implementation is deliberate: a second copy would drift on the response shape clients branch on. tool is also part of the warn-dedup key, so a hook that both polls and acks learns that both are failing rather than only whichever raced first:

ack_events: rejecting calls from deleted session jovial-kentro (drain-1..., deleted_at=...)

Acking also refreshes the heartbeat, since it is session activity — a consumer that only acked would otherwise be swept as stale out from under itself.

Two hazards, pinned as tests

Neither is something ack_events can detect from its own arguments, so both live in guide.md and in executable form:

Peek	next_cursor is	Safe to ack?
unfiltered, or min_level	the raw batch max	Yes — hidden noise counts as seen, deliberately
channel / event_types / correlation_id	the matched batch max	No — commits every lower-id non-match
order="desc"	the tip, while the batch is the newest slice	No — discards the never-surfaced backlog

Measured: a 20-event backlog peeked at limit=5 under desc surfaces 5 and loses the other 15 (TestAckOrderingHazard); events 1-10 with only 3 and 7 matching an event_types filter returns next_cursor: 8, burying 1, 2, 4, 5, 6 (TestAckNarrowingHazard).

CLI

agent-event-bus-cli ack --cursor N [--session-id ID] [--allow-rewind] [--json]

Uses the session-id env fallback the other verbs use ($AGENT_EVENT_BUS_SESSION_ID, else $CLAUDE_CODE_SESSION_ID — the #137 case, which is exactly the tool-spawned drain hook this verb serves), and exits non-zero on a refused ack so a hook cannot mistake one for success.

guide.md, README.md and CLAUDE.md move with the API, per CLAUDE.md. The guide's drain example and its shell twin in cli.py are both the loop, since one pass drains at most limit.

Scope note

This branch also carried a fix for #140 — deleted sessions polling silently, and error results logging as successes via _format_result's branch order. #142 landed both first, and more thoroughly: rejecting on every read path rather than flagging a success, with a rate-limited warning and a display_id lookup that makes deleted sessions greppable.

I dropped that work rather than merging it — duplicating merged code to resolve a conflict would have been waste. Only ack_events was unique, and it is rebuilt here on top of #142 using its helpers.

No schema change, no migration, no backup needed before deploying.

---

Generated by Claude Code

[claude]

[Important] This example omits order="asc", so it uses the default desc. The section directly above already warns why that matters ("With order="desc" the page is the newest slice, so older backlog events are skipped by the next cursor call") — but under ack_events the consequence is worse than a skipped page: the skip is committed to the saved cursor and is unrecoverable without allow_rewind.

Fails when: a drain hook copies this snippet verbatim and 200 events are pending → the peek returns the newest 50 (filtered) and next_cursor is the tip → ack_events marks the ~150 older, never-surfaced events as seen, and a later resume poll never returns them. That is the consumed-but-never-surfaced loss this primitive exists to prevent.

Both the CLI example in cli.py and test_peek_then_ack_makes_progress_under_min_level pass order="asc"; the guide snippet looks like the one place it was dropped. Suggest get_events(session_id=sid, resume=True, peek=True, min_level="actionable", order="asc"), plus a one-line note that an ack is only as bounded as the peek ordering.

[claude]

[Suggestion] The ahead-of-tip guard is inert when the events table is empty: get_cursor() returns None, so tip is not None and ... short-circuits and any non-negative cursor is accepted and persisted. A session that acks 5 against a fresh bus then never sees events 1-5 once they are published. Low impact (fresh install only), but the guard reads as absolute and is not — if target > int(tip or 0) would close it.

[claude]

[Suggestion] update_session_cursor returns False when the row is gone or deleted_at IS NOT NULL, and the return value is dropped here. If the session is deleted between _load_polling_session above and this write, the ack is a no-op but the caller still gets {"success": true}. Narrow race, but since cmd_ack deliberately exits non-zero so a drain hook cannot mistake a refusal for success, honoring the boolean would keep that contract exact.

[claude]

[Suggestion] Now that tool is parameterized, the surrounding text is still poll-specific: the warning says "an orphaned client is still polling" and this notification says "deleted session polled". For ack_events that is slightly misleading in the log an operator greps. Something neutral ("deleted session call" / "an orphaned client is still calling the bus") would match the shared implementation the new docstring paragraph describes.

[claude]

[Suggestion] Stray # notify comment left above # ack — the real # notify marker is a few lines below at the p_notify block.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 1

Summary

ack_events is a well-scoped primitive that closes issue 134 cleanly: peek and ack name the same raw window by construction, the three refusals are the right ones, reusing _deleted_session_error rather than forking the 142/140 contract is the correct call, and the heartbeat refresh closes a real self-sweep hole. Server tests, CLI tests, and the tool-coverage list all move with it, and guide.md / README.md / CLAUDE.md are updated per CLAUDE.md.

One blocking finding: the canonical example in guide.md — the document served as an MCP resource into every session that reads it — omits order="asc", so copying it reproduces exactly the consumed-but-never-surfaced loss this primitive exists to prevent. The CLI example in cli.py and test_peek_then_ack_makes_progress_under_min_level both get this right; only the guide snippet does not.

Previously Addressed (Filtered)

None — first automated round on this PR.

Verdict

REQUEST_CHANGES - guide.md:185 demonstrates the peek-then-ack flow at the default order="desc"; with a backlog larger than limit the peek returns only the newest slice while its next_cursor is the tip, so the ack permanently marks the older, never-surfaced events as seen. One-argument fix. The four suggestions posted inline are non-blocking.

---

Automated review by Claude Code

[evansenter]

All five fixed in ea81263. The blocking one was right, and worse than your estimate — I reproduced it before touching anything:

peek(desc) surfaced: ['event-20', 'event-19', 'event-18', 'event-17', 'event-16']  next_cursor: 21
after ack, still pending: []
>>> events surfaced: 5   events LOST: 15

A 20-event backlog at limit=5: fifteen events silently gone, permanently. In the file served as an MCP resource into every session that reads it, demonstrating the exact loss the primitive exists to prevent. Good catch.

The example now passes order="asc" and says why with that number — "ordering matters" reads like style advice without it. I also added TestAckOrderingHazard pinning both directions, so the hazard survives as executable knowledge rather than prose someone tidies away: the desc flow loses the 15 unsurfaced events, the asc flow drains all 20 with nothing lost and nothing repeated. ack_events can't fix this itself — it never sees how the peek was ordered.

Empty-bus ceiling — correct, the guard was inert. Worth noting the test needs its own database: the suite shares one DB and never deletes events, so ids only climb and no shared-fixture test can reach that branch. My first two attempts at it passed vacuously against a non-empty table before I noticed.

Dropped update_session_cursor return — fixed. It now re-reads and returns the same rejection shape a poll gets, rather than inventing a third one, so the contract cmd_ack's non-zero exit depends on stays exact.

Poll-specific warning text — fixed, now neutral. That wording was mine to correct: I'm the one who parameterized tool for a second caller and left the surrounding sentence describing only the first.

Stray # notify — removed.

Both guard fixes are mutation-tested: reverting either fails its own test and nothing else. 632 passing, lint and format clean.

---

Generated by Claude Code

[claude]

[Suggestion] This drops the hint on a deleted-session rejection, where cmd_events deliberately keeps it (lines 349-352: the hint carries the actionable half — re-register or stop polling — and dropping it leaves an orphaned poller with no next step).

The PR body frames the reuse as: a drain hook gets the same shape and hint its polls already get. Through MCP it does, and --json passes it through, but the human-readable path here prints only Error: Session deleted. An operator debugging a swept drain hook sees the exit code and the error text with no indication that re-registering is the fix.

The three-line block from cmd_events would drop in unchanged.

[claude]

[Suggestion] On a bus with no events at all, next_cursor is null: the resume-with-peek branch sets cursor = storage.get_cursor(), which returns None on an empty events table, the batch comes back empty, and storage.get_events then returns next_cursor = cursor. jq -r prints the literal string null, so the last line of this snippet runs ack --cursor null and gets Invalid cursor "null" with exit 1.

Harmless in the steady state — once the session has a saved cursor, an empty peek echoes it back and the ack is a clean no-op — but a drain hook copied verbatim onto a fresh bus reports a failure on its first run. Guarding with jq -r ".next_cursor // empty" plus a non-empty test keeps the example a no-op instead of an error.

[claude]

[Suggestion] The docstring promises "Auto-refreshes heartbeat" unconditionally, but this call sits after every rejection path — an ack refused for being ahead of the tip, behind the cursor, or non-numeric never touches last_heartbeat. _get_events_impl refreshes before its checks (line 657), so the two session-scoped reads differ on when the promise holds.

Probably fine in practice, since a refused ack is a transient condition and a consumer that only ever gets refused acks has a larger problem. But either hoisting the call to just after the deleted/not-found checks, or qualifying the docstring as "Auto-refreshes heartbeat on success", would make the contract match the code.

[claude]

[Suggestion] This persists the raw cursor string rather than the str(target) the guards above actually validated. int() is permissive — " 42 ", "+42", "4_2", and unicode digit forms all parse — so any of those lands in last_cursor verbatim.

It round-trips correctly today, because every reader re-parses with int() (storage.get_events at storage.py:732, and the rewind comparison a few lines up), so this is not a defect. But the stored string is also what surfaces in previous_cursor, in list_sessions, and in the rewind rejection message, which would read as Cursor 41 is behind the session current position (4_2). Writing str(target) makes the canonical form the one that persists.

[claude]

[Suggestion] The tool roster and this API table both move with ack_events, but the Auto-heartbeat design-decision bullet further down still names only publish_event and get_events. ack_events refreshes the heartbeat too, deliberately — a consumer that only acks would otherwise be swept as stale out from under itself, which test_acking_refreshes_the_heartbeat pins.

Worth adding ack_events to that bullet, and perhaps a one-line Design Decisions entry for the peek/ack pairing: the reason it exists (a bounded consume cannot bound anything under a server-side filter, because the cursor advances over the raw batch behind the filtered view) is exactly the kind of non-obvious rationale that section carries, and right now it lives only in guide.md.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 2

Summary

All five round-1 findings are fixed, and the blocking one is fixed better than it was reported: guide.md now specifies order="asc" with the measured 15-event loss, which is what turns "ordering matters" from style advice into a reason, and TestAckOrderingHazard pins both directions so the hazard survives as executable knowledge. The empty-bus ceiling (target > (int(tip) if tip else 0)) and the honored update_session_cursor return — which now re-reads and returns the same rejection shape a poll gets, keeping exact the contract that cmd_ack non-zero exit depends on — each have a test that fails when reverted.

Re-checked the primitive itself this round rather than only the deltas. Guard order is right: the ahead-of-tip ceiling is evaluated before allow_rewind, so a deliberate rewind cannot climb past the tip. previous_cursor is read before the write. The heartbeat refresh is correctly skipped on the lost-deletion-race path, so a no-op ack does not look like activity. And the raw-versus-filtered split in _get_events_impl — next_cursor taken over the raw batch, min_level applied after cursor bookkeeping — is precisely what makes peek and ack name one window; an ack of a peek whose events list is empty but whose raw window was not still correctly commits the noise. No test, doc, or bridge reference depended on the old "rejecting polls" warning text.

Five non-blocking suggestions posted inline: a dropped hint in the CLI deleted-session path (where cmd_events deliberately keeps it), a null next_cursor edge in the shell example on a fresh bus, an unconditional "Auto-refreshes heartbeat" docstring claim, cursor-string normalization on the write, and one stale CLAUDE.md bullet.

Previously Addressed (Filtered)

All five round-1 items — the guide.md order omission (Important), the inert empty-bus ceiling, the dropped update_session_cursor return, the poll-specific warning wording, and the stray comment marker — are Implemented and verified in the current diff. Not re-raised.

Verdict

APPROVE - No blocking findings. Nothing here names a way the code produces a wrong result; the five suggestions are message-completeness and documentation polish.

---

Automated review by Claude Code

[evansenter]

All five round-2 suggestions fixed in 3153c2a.

The dropped hint was the one worth catching — it made this the single place the shared #140 contract wasn't actually shared, and it contradicted my own PR body, which claims a drain hook gets the same shape and hint its polls already get. Through MCP and --json it did; the human-readable path printed Error: Session deleted and nothing else. Same three-line block as cmd_events now, with a test.

Heartbeat timing — hoisted to just after the deleted/not-found checks, matching where _get_events_impl refreshes. I took the "hoist" option over "qualify the docstring" because a refused ack really is the session saying it's alive; qualifying would have documented an inconsistency rather than removed one.

Cursor normalization — fixed, and the rewind message was the convincing part: behind the session's current position (4_2) is the kind of thing you'd stare at for a while. Stores and echoes str(target), pinned by a test that acks " +N " and asserts both the stored value and the message.

CLAUDE.md — added ack_events to the auto-heartbeat bullet and a Design Decisions entry for the peek/ack pairing, including the order="asc" requirement. Agreed that's where it belongs; that section is where this repo keeps rationale you can't infer from the code.

The null next_cursor — guarded with // empty and a non-empty test, but I should be straight that the scenario doesn't reproduce through the documented flow. register_session publishes session_registered, so any bus with a session has a tip:

events on a virgin bus: None
peek by a foreign id, zero events -> next_cursor: None
after ANY register, tip is: '1'   (registration publishes an event)

A drain hook needs a registered session, so its peek always has a tip to echo. It's reachable only by a foreign session id against a literally empty events table. The guard costs nothing and the snippet is correct for that case now, so I kept it — but the "copied verbatim onto a fresh bus reports a failure on its first run" framing isn't right, and I'd rather say so than let it stand.

635 passing, lint and format clean.

---

Generated by Claude Code

[claude]

[Suggestion] This warning box covers the ordering hazard thoroughly, but the other way a peek and its next_cursor stop naming the same window is narrowing filters, and that is not mentioned anywhere in the section.

storage.get_events applies channels / event_types / correlation_id in SQL, so next_cursor comes back as the max id of the matched batch. Acking it commits every lower-id non-matching event too. That is exactly the loss _get_events_impl refuses to perform on its own (server.py:721-724 - advancing the cursor would mark every non-matching lower-id event as seen and silently drop it from a later resume), which is why narrowed reads are non-consuming in the first place. ack_events hands that decision back to the caller, and the sentence above - Peek and ack name the same window by construction - reads as unconditional.

Concretely: events 1-10 pending, ids 3 and 7 are help_needed. A peek with event_types=["help_needed"], order="asc" returns 3 and 7 with next_cursor: 7; acking 7 marks 1, 2, 4, 5, 6 seen without ever having surfaced them. Reachable straight from the CLI as events --peek --resume --channel session:$SID --json followed by ack --cursor, which is a plausible shape for a DM-only drain hook.

The distinction is worth stating explicitly, because the two filter kinds differ in exactly the way that matters here: min_level filters after cursor bookkeeping, so acking its next_cursor is correct and deliberate (the noise counts as seen); the SQL-level filters filter before it, so acking theirs is a loss. One sentence in this box - ack only what a min_level-filtered or unfiltered peek returned - would cover it.

Not raised as blocking: the code does what it is asked, the documented example is correct, and this is a gap in the warning rather than a defect in the primitive.

[claude]

[Suggestion] This error path ignores --json, where cmd_events honors it (lines 347-357: the error dict is printed as JSON to stdout when --json is set, and only the human-readable form goes to stderr otherwise).

So ack --json on a refused ack prints nothing to stdout and a human-formatted Error: ... line to stderr. A drain hook consuming --json output gets an empty stdout and a jq parse error rather than the session_deleted flag it would branch on - the one field the PR body promises is shared with polls (a drain hook gets the same shape and hint its polls already get). Through MCP it does; through --json it does not.

The exit code still signals failure, so nothing silently succeeds - this is about the machine-readable half of the same contract the round-2 hint fix closed for the human-readable half. Mirroring the cmd_events branch would make the two verbs identical on error.

[claude]

[Suggestion] The // empty guard fixes the ack --cursor null failure from round 2, but the [ -n "$CUR" ] && cmd form is itself the last command in the snippet, so the whole thing still exits 1 on the empty-bus run - a hook that ends here reports failure in exactly the case the guard was added for, and under set -e it aborts.

The if ... then ... fi form is a clean no-op with status 0 instead. Worth the extra line since this snippet is written to be copied into a hook verbatim.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 3

Summary

All five round-2 suggestions landed: the hint is kept on the CLI deleted-session path, the shell example guards next_cursor with // empty, _auto_heartbeat is hoisted above the validation so the unqualified Auto-refreshes heartbeat docstring is now true, str(target) is what persists, and CLAUDE.md gained both the heartbeat mention and a peek/ack Design Decisions bullet. TestAckNormalizesAndReportsConsistently pins the last two.

Re-verified the core contract this round rather than only the deltas. storage.get_events computes next_cursor before _get_events_impl applies min_level, so an ack of a filtered peek next_cursor genuinely commits the raw window - the property the whole primitive rests on. The hoisted heartbeat is safe on the lost-deletion-race path because update_heartbeat carries the same deleted_at IS NULL guard as update_session_cursor, so a session deleted mid-call gets neither write. Guard order still puts the ahead-of-tip ceiling before allow_rewind, so a deliberate rewind cannot climb past the tip, and the empty-bus ceiling still rejects. Test imports (Session, SQLiteStorage, patch) are all present.

Three non-blocking suggestions posted inline. The one worth reading is the first: the guide claim that peek and ack name the same window by construction holds under min_level, but not under channel / event_types / correlation_id, where next_cursor is the max of the SQL-filtered batch. That combination is reachable from documented CLI flags and reproduces the exact loss the narrowed-read rule in _get_events_impl exists to refuse. It is a gap in the warning box, not a defect in the code - the primitive does what it is asked - so per section 6 it is a Suggestion, but it is the one I would fix.

Previously Addressed (Filtered)

All five round-2 items (CLI hint, null next_cursor in the shell example, unconditional heartbeat docstring, cursor normalization, stale CLAUDE.md bullet) are Implemented and verified in the current diff. All five round-1 items remain fixed. None re-raised.

Verdict

APPROVE - No blocking findings. Nothing here names a way the code produces a wrong result; the three suggestions are documentation completeness and one CLI output asymmetry.

---

Automated review by Claude Code

[evansenter]

All three fixed in 1a0ebd3. You were right that the first is the one to fix — I reproduced it before touching anything:

narrowed peek surfaced: ['event-3', 'event-7']  next_cursor: 8
still pending after ack: ['event-8', 'event-9', 'event-10']
>>> silently committed without surfacing: event-1, event-2, event-4, event-5, event-6

My sentence "peek and ack name the same window by construction" was stated unconditionally and is only true for min_level and unfiltered reads. The failure is worse than a documentation gap in one respect: the two shapes are indistinguishable in the response — both return events and a next_cursor — so a caller has no way to tell a safe ack from a lossy one by inspection.

The warning box now covers both hazards, with a table for the filter split since that's the part you can't infer:

Filter	Applied	next_cursor is	Safe to ack?
min_level	after bookkeeping	raw batch max	Yes
channel / event_types / correlation_id	in SQL, before	matched batch max	No

TestAckNarrowingHazard pins both halves — the narrowed ack burying five events, and min_level's next_cursor equalling the raw tip.

--json on the error path — fixed, mirrors cmd_events exactly now. This was the machine-readable half of the same contract round 2 closed for the human-readable half; I fixed one and left the other, which is a poor way to close a contract gap.

[ -n "$CUR" ] && cmd — good catch, and it's the more embarrassing one: my round-2 guard introduced a new failure of the same kind it was fixing, exiting 1 on precisely the nothing-to-ack run. Now an if block, with a comment saying why, since the && form is the one a reader would naturally reach for.

638 passing, lint and format clean.

---

Generated by Claude Code

[claude]

[Suggestion] warn_key is (session_id, deleted_at_str) - it does not include tool. The tool parameter was added this PR so the warning names the caller, but in the case it was added for (a drain hook that polls and acks with the same deleted session id) the poll wins the race to _warned_deleted_sessions, and every subsequent ack_events rejection is suppressed. The operator sees one line naming get_events and never learns acks are being rejected too.

Adding tool to the key gives one line per (session, deletion, tool) - still bounded, still not the 100k-line problem the set exists to prevent. Non-blocking: the middleware logs each rejection per-call regardless, so nothing is invisible, just less legible at the WARNING level.

[claude]

[Suggestion] The two cursor rejections report "where you actually are" under different keys: ahead-of-tip returns it as next_cursor (line 840), rewind returns it as cursor (line 856). A client that wants to recover programmatically - clamp to the tip, or resync to the current position - has to branch on which error it got, or parse the message.

Since both answer the same question, one key for both (cursor, matching the success shape) would let a caller read it unconditionally. Worth weighing against the fact that neither is a documented contract yet, so this is the cheap moment to pick one.

[claude]

[Suggestion] The docstring pins order="asc" but is silent on the other half of the hazard the guide now documents - that only unfiltered or min_level peeks are safe to ack, and a channel / event_types / correlation_id peek commits every lower-id non-match beneath its next_cursor.

An MCP caller sees this docstring in every conversation; agent-event-bus://guide is only in context if the session read it. Both hazards cost the same thing (events committed without being surfaced) and only one is inline. CLAUDE.md caps docstrings at non-obvious behavior, which this is - a clause on the existing Pairs-with-peek line would cover it, e.g. peek with order="asc" and no channel/event_types/correlation_id filter. Noting round 3 already got the guide right; this is placement, not a re-raise.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 4

Summary

All three round-3 findings landed. The narrowing hazard is now documented where it belongs — the warning box splits the two filter kinds by which side of the cursor bookkeeping they run on, which is the part a caller cannot infer, since both shapes return events and a next_cursor identically. TestAckNarrowingHazard pins both halves: the event_types peek burying events 1, 2, 4, 5, 6, and the min_level peek whose next_cursor equals the raw tip. cmd_ack --json on the error path now mirrors cmd_events exactly, with a test asserting stderr stays empty. The shell example guard is an if block, with a comment naming the set -e reason.

Re-verified the primitive against storage this round rather than only the deltas. storage.get_events computes next_cursor as the max id over the rows SQL returned, before _get_events_impl applies min_level — so the guide table is accurate on both rows, and the safe row is safe for the stated reason rather than incidentally. The empty-batch branch returns the input cursor unchanged, so a nothing-to-ack peek yields an ack equal to the current position: not backwards, accepted as a no-op, and the non-empty test in the shell example means the CLI never issues it at all. Guard order still evaluates the ahead-of-tip ceiling before allow_rewind, so a deliberate rewind cannot climb past the tip. update_heartbeat and update_session_cursor carry the same deleted_at IS NULL guard, so a session deleted mid-call gets neither write and the raced path returns the same rejection shape a poll gets rather than a success.

Three non-blocking suggestions posted inline, all about the shape of the rejections rather than whether they fire.

Previously Addressed (Filtered)

All three round-3 items (guide narrowing hazard, --json error path in cmd_ack, the exit-1 on the nothing-to-ack run in the shell example) are Implemented and verified in the current diff. All round-1 and round-2 items remain fixed. None re-raised.

Verdict

APPROVE - No blocking findings. Nothing here names a way the code produces a wrong result; the three suggestions are log attribution and response-shape consistency.

---

Automated review by Claude Code

[evansenter]

All three round-14 items taken, in 84d9403.

The deleted-session refusal isn't a cursor refusal. Verified against _deleted_session_error — it returns error / session_deleted / session_id / display_id / deleted_at / hint, and no cursor at all. So the summary sentence over-claimed by exactly one list item, and the item that broke it is the one round 12 added to the list directly above it. Scoped to the four cursor refusals, with the deleted case recovering via register_session — which its own hint already told the caller. Worth noting test_every_refusal_reports_a_re_ackable_position enumerates precisely those four, so the code and the test were already in agreement; the prose was the only thing out.

The drain snippet never said a pass is bounded by limit. Taken. Correct and lossless either way, but a Stop hook written from that snippet needs ten invocations to clear a 500-event backlog, and the question "is one call per hook enough?" gets answered right there, not seven sections up where has_more is explained for plain polling. Step 4 now says it and points at has_more.

--session-id help on ack. Taken, and the argument for it is specific rather than generic consistency: ack's stated consumer is a Claude Code hook, the #137 fallback exists precisely for tool-spawned subprocesses where the rc-file mapping never runs, and this was the one verb whose help stopped at $AGENT_EVENT_BUS_SESSION_ID. Someone writing that hook from --help was being told to arrange a variable they may not need. Fixed in both the help string and the missing-id error at cli.py:409, which had the same gap; publish and events already named both.

649 tests pass, format and lint clean.

---

Generated by Claude Code

[claude]

[Suggestion] The refusal list covers four cases, and the sentence below names the deleted-session refusal as the one exception that carries no cursor. There is a fifth: an ack whose session_id was never registered returns {error: "Session not found", session_id: ...} (server.py:809), and it carries no cursor key either — so it is a second exception to the claim that recovery does not branch on which refusal you got.

Reachable in the shape this verb is built for: the documented CLAUDE_CODE_SESSION_ID fallback can supply an id that never registered with the bus. In practice such a client hits the same rejection on its --resume peek first, so the ack is not usually where it surfaces — which is exactly why one line here ("an unregistered session_id — no position either; register_session first") is cheap insurance against the next reader treating the four-way claim as exhaustive. test_unknown_session_is_a_plain_not_found already pins the behaviour; only the prose omits it.

[claude]

[Suggestion] The help for --cursor says "the next_cursor from an order=asc, unfiltered peek", but a min_level-filtered peek is safe to ack — that is the case the primitive was built for. The guide table says so explicitly (min_level → safe to ack), and the shell example in this same module docstring peeks with --min-level actionable and then acks it.

So the one line a hook author reads from --help steers them away from the exact flow the example demonstrates. Naming the filters that actually matter would fix it: "from an --order asc peek with no --channel/--event-types/--correlation-id".

[claude]

[Suggestion] Step 4 of the guide snippet now says a pass drains at most limit and to loop on has_more; this shell twin — the one a hook author actually copies — still shows a single peek-ack pass with no mention of the bound.

Same gap round 14 closed on the Python side, in the other file. A # one pass drains at most 50; loop on .has_more to clear a backlog line, or wrapping the block in a while, would keep the two snippets saying the same thing.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 15

Summary

All three round-14 items landed. The summary sentence is scoped to the four cursor refusals now, with the deleted case recovering via register_session — matching what test_every_refusal_reports_a_re_ackable_position already enumerated. The drain snippet names the limit bound and points at has_more, and the --session-id help on ack plus its missing-id error both name the CLAUDE_CODE_SESSION_ID fallback, which is the case #137 exists for. The follow-on commits are consistent with them: registered_tools() is shared from conftest rather than duplicated, and TestToolColorRoster states its own one-directional limit in the docstring rather than implying coverage it does not have.

Re-verified the primitive against storage this round rather than only the deltas. next_cursor is still the max over the SQL-returned rows, computed before min_level filters, so the guide table is accurate and its safe row is safe for the stated reason. Guard order still evaluates the ahead-of-tip ceiling before allow_rewind. The ack branch in _format_result keys on cursor and previous_cursor together, which is what keeps it off register_session — that result carries cursor alone — and it sits below the error check, so refusals still render as ERROR.

Three non-blocking suggestions, all documentation: one refusal shape the guide list does not cover, and two places where the cli.py help and its shell example are narrower than what the guide now says.

Previously Addressed (Filtered)

All three round-14 items (deleted-session refusal scoped out of the cursor list, limit/has_more in the drain snippet, --session-id help on ack) are Implemented and verified in the current diff. Rounds 1-13 remain fixed; none re-raised.

Verdict

APPROVE - No blocking findings. Converged at round 15 — remaining feedback is non-blocking. Nothing here names a way the code produces a wrong result.

---

Automated review by Claude Code

[evansenter]

All three round-15 items taken, in dea9148. The middle one is a straight error of mine.

The fifth refusal shape. Confirmed: an ack from a session_id that never registered returns a plain {error: "Session not found", session_id} at server.py:809 — no cursor key — so my round-14 fix left a second exception to the "recovery does not branch" claim while asserting there was one. Restructured rather than patched: the list now splits into the cursor refusals (recover by re-acking a position) and the session refusals (recover by registering), and says outright that reading cursor unconditionally will KeyError on the latter, so branch on error first. test_unknown_session_is_a_plain_not_found already pinned the behaviour.

--cursor help said "unfiltered peek". This one was wrong, not merely narrow: a min_level-filtered peek is the case the primitive exists for, and the shell example twelve lines above in the same module docstring performs exactly that. I introduced the phrasing in round 11 as shorthand and it excluded the intended flow. Now names the three filters that actually break the pairing, with --min-level marked safe. Swept guide.md, CLAUDE.md and the MCP docstring for the same shorthand — they all already said "unfiltered or min_level", so this was the only site.

The shell twin missing the limit/has_more note. Taken. Round 14 added it to the Python snippet only, which left the two halves of the same flow documented at different levels of care — the same asymmetry that motivated the // empty guard suggestion several rounds back, and the shell one is what a hook author copies.

649 tests pass, format and lint clean.

---

Generated by Claude Code

[claude]

[Suggestion] This mutates the real module-level set in place and never restores it. The equivalent setup in TestDeletedSessionPolling::test_warning_is_logged_once_per_deletion (test_server.py:1631) uses monkeypatch.setattr(server, "_warned_deleted_sessions", set()), which pytest unwinds after the test; .clear() leaves whatever this test added in the global for every test that runs after it.

Not a failure today — nothing downstream asserts on the set prior contents, and the entries this test adds are keyed on a session id it just deleted. But it makes the ordering assumption load-bearing in a suite that is otherwise careful about it, and the neighbouring test already shows the restoring form. Swapping .clear() for the monkeypatch.setattr line would make the two read the same and drop the assumption entirely.

[claude]

[Suggestion] Round 15 added the bound, which was the important half. The remaining gap is that neither snippet is loopable as written: pending is assigned once at step 1 and never reassigned, so "loop while pending[has_more]" over this exact code either runs once or spins on a stale value. The cli.py twin (cli.py:85-86) has the same shape — $OUT is captured once, and "Loop while jq -r .has_more is true" has nothing to re-run.

Since the sentence is prose and the block above it is copyable, a reader is most likely to copy the block and add the loop themselves — which makes re-peeking the one step they have to infer. Wrapping steps 1-3 in a while True: with a break when has_more is false would make the whole thing copyable, and the if pending[next_cursor] guard already inside step 3 keeps working unchanged.

[claude]

[Suggestion] This bullet folds two distinct server refusals into one. int(cursor) failing returns Invalid cursor ... expected an event id (server.py:840-844), but a negative value parses fine and falls to a separate branch returning Invalid cursor ... must not be negative (server.py:845-850). A caller matching on the documented text — which this section elsewhere quotes verbatim, e.g. the 999999 example at line 254 — would miss the negative case.

Both carry cursor: position, so the recovery advice above is unaffected either way; it is only the error string that is under-described. TestAckRejectionShape::test_every_refusal_reports_a_re_ackable_position already exercises the two branches separately, so the distinction is pinned in tests but not in the doc.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 16

Summary

All three round-15 findings landed: guide.md step 4 and the cli.py shell twin both state the limit bound, the --cursor help now admits --min-level as safe, and the refusal list names the never-registered session as the second cursor-less shape. I checked that last one against _deleted_session_error (server.py:622-632) and the Session not found return (server.py:809) — neither carries a cursor key, so the guide "branch on error first" caveat is exactly right and covers both.

Re-checked the primitive itself rather than only the deltas. Guard order still evaluates the ahead-of-tip ceiling before the rewind check, so a deliberate rewind cannot climb past the tip. position = previous is read before the write and is what all four cursor refusals report, matching guide.md:240-243. The int(tip) if tip else 0 ceiling still fires on an empty bus. update_session_cursor is a deleted_at IS NULL guarded UPDATE returning rowcount > 0, so the raced path returns a rejection rather than a success — and since SQLite counts matched rows on UPDATE, re-acking the current position still returns True, so the nothing-to-ack no-op the CLI example can emit is not misreported as a lost race. The new _format_result branch requires both cursor and previous_cursor, which only the ack success dict carries, and it sits below the error branch so refusals still log as ERROR.

Three non-blocking suggestions inline — one test-isolation nit, two documentation precision points.

Previously Addressed (Filtered)

All round-15 items (guide has_more/limit bound, cli.py shell twin bound, --cursor help admitting min_level, the fifth refusal shape) are Implemented and verified in the current diff. Rounds 1-14 remain fixed; none re-raised.

Verdict

APPROVE - No blocking findings. Converged at round 16 — remaining feedback is non-blocking. Nothing here names a way the code produces a wrong result, and nothing is a regression against an earlier round.

---

Automated review by Claude Code

[evansenter]

All three round-16 items taken, in 8a1c452.

Neither snippet was loopable. This is the right criticism of my round-15 fix: I added the bound as prose above blocks that capture pending / $OUT exactly once, so a reader copying the block and adding the loop themselves has to infer the one step that actually matters — that the peek goes inside, because the ack moves the cursor and the next pass is a fresh window. Stating the bound while leaving that to inference was the worse half of the job. Both snippets are now the loop, with that reason written down rather than implied. Shell twin checked with bash -n.

One knock-on: the if-over-&& comment justified itself with "as the last command of a hook", which stopped being literally true once the break test follows it. Reworded to the durable form — the hazard is real if the block is lifted out of the loop, which is exactly what someone adapting this will do.

The malformed-cursor bullet folded two branches. Correct, and it matters here specifically because this section quotes error text verbatim elsewhere (the 999999 example). expected an event id and must not be negative are separate branches with separate strings, so a caller matching on the documented text would miss the negative case. Split — and that made the "first three bullets" reference below stale, now four.

The warn-set .clear(). Taken. Swapped for the monkeypatch.setattr(server, "_warned_deleted_sessions", set()) form the neighbouring TestDeletedSessionPolling already uses on the same global. It wasn't failing, but it made ordering load-bearing in a suite that's otherwise careful about it, and the restoring form was sitting forty lines up.

649 tests pass, format and lint clean.

---

Generated by Claude Code

[claude]

[Suggestion] Now that this is a loop, it indexes pending unguarded on every pass — but the same section, 55 lines down, tells callers the opposite: "Reading cursor unconditionally on a refusal will KeyError on these two, so branch on error first" (guide.md:255-256). A session deleted or timed out mid-drain makes get_events return the {error, session_deleted, hint} shape, and this snippet reaches pending["next_cursor"] on it.

The cli.py twin survives this by accident of jq semantics — .next_cursor // empty and .has_more both yield nothing/null on an error object, so it skips the ack and breaks. The Python form has no such fallback, and it is the one an agent reading agent-event-bus://guide copies. An if "error" in pending: break as step 0 would make the canonical snippet obey the rule the section states.

The multi-pass loop widens this: a single-pass drain could only lose the race between register and poll, whereas this one holds the window open for the whole backlog.

[claude]

[Suggestion] Worth one clause saying why the loop conditions on has_more rather than on pending["events"], because under min_level those two genuinely differ: has_more is computed over the raw batch (storage.py:783, len(events) == limit) before the level filter runs (server.py:738-742), so a pass whose 50 raw events are all lifecycle noise returns events: [] with has_more: true and a non-null next_cursor.

That is exactly the case this primitive was built for, and it is the one where the obvious "optimization" — break when nothing was surfaced — reintroduces a stall: the drain would exit with a full backlog of noise still un-acked, then re-peek the identical window on the next hook run and stall again. The has_more/events split is the load-bearing detail here, and right now only the comment above the loop hints at it.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 17

Summary

Both round-16 findings landed. The drain snippets are genuinely loopable now — the peek sits inside the loop in both the guide.md Python form and the cli.py shell twin, with a comment saying why (the ack moves the cursor, so the next pass is a fresh window). The refusal list splits the two Invalid cursor branches, matching server.py:840-850 verbatim on both messages, and the count sentence below it moved from "first three" to "first four" with it. test_each_tool_warns_once_for_the_same_dead_session now uses monkeypatch.setattr(server, "_warned_deleted_sessions", set()), the same form TestDeletedSessionPolling uses for that global.

I checked termination on the new loops rather than only reading them. Progress is guaranteed: has_more is limit > 0 and len(events) == limit over the raw SQL batch (storage.py:783), so has_more=True implies a non-empty batch, which implies next_cursor = max(id) > cursor — the ack therefore always advances, and the next peek reads a strictly higher window. The exit paths hold too: an empty bus yields next_cursor=None / has_more=False and breaks on the first pass, and a session deleted mid-drain makes the shell form jq -r .has_more print null, which is not true, so it breaks rather than spinning on an un-ackable cursor.

Re-verified the primitive itself rather than only the deltas. Guard order still evaluates the ahead-of-tip ceiling before the rewind check, int(tip) if tip else 0 still fires on an empty bus, position = previous is read before the write and is what all four cursor refusals report, and the _format_result ack branch still requires cursor and previous_cursor while sitting below the error branch, so refusals still log as ERROR.

Previously Addressed (Filtered)

Both round-16 items (loopable drain snippets in guide.md and cli.py; the negative-cursor refusal split out with its own message and the count sentence updated) are Implemented and verified in the current diff. Rounds 1-15 remain fixed; none re-raised.

Verdict

APPROVE - No blocking findings. Converged at round 17 — remaining feedback is non-blocking. Nothing here names a way the code produces a wrong result, and nothing is a regression against an earlier round. Two documentation suggestions posted inline, both on the newly-looping snippet.

---

Automated review by Claude Code

[evansenter]

Both round-17 items taken, in 5c9fff8. Both are consequences of making the snippets loop last round — the loop widened a window that a single pass barely had.

The unguarded index. Right, and the internal contradiction is the damning part: the snippet reached pending["next_cursor"] unconditionally while the same section, 55 lines down, says to branch on error first. Added the bail as step 2, with the reason — a session deleted or timed out mid-drain fails the poll rather than returning an empty batch, and looping holds that window open across the whole backlog instead of one pass.

On the shell twin: it's better off than "accident of jq semantics". cmd_events exits non-zero on a server-side error, so under set -e the hook aborts at the assignment, loudly; without set -e, .has_more reads null on the error object and the loop breaks. Either way it can't spin. But that took me reading two files to establish, which is the argument for writing it down — so the snippet now says it rather than leaving the next reader to re-derive it.

Why has_more and not the event count. Verified the ordering: has_more is limit > 0 and len(events) == limit over the raw batch (storage.py:783), and min_level filters afterwards (server.py:738), so 50 lifecycle events return events: [] with has_more: true and a non-null next_cursor. The "break when nothing surfaced" optimization therefore exits with that noise un-acked and re-peeks the identical window on every subsequent run — a stall, in precisely the configuration this primitive was built for. Stated in both snippets, at the condition rather than in the preamble.

Shell twin re-checked with bash -n. 649 tests pass, format and lint clean.

---

Generated by Claude Code

[claude]

[Suggestion] The bail is right, but it exits the loop silently — a drain hook that copies this treats a deleted-session refusal exactly like a clean drain, with no trace that the backlog was abandoned. The CLI twin does better here: cmd_ack and cmd_events exit non-zero, so under set -e the hook aborts loudly.

Worth one more line inside the branch showing what to do with pending before breaking — surfacing pending["error"] and pending.get("hint"). The hint is the actionable half ("re-register or stop polling"), and it is the reason the deleted-session contract carries one at all. Non-blocking; the code as written is correct.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 18

Summary

Both round-17 items landed, and 5c9fff8 is documentation-only — the two drain snippets gain the refusal bail and the has_more-not-event-count reason, with no behavioral surface added. The guide.md loop now branches on error before indexing pending, which removes the contradiction with the "branch on error first" caveat 55 lines below, and the renumbering to 1-5 is consistent throughout. The new comment on the shell twin records the two-file derivation (non-zero exit at the assignment under set -e; .has_more reading null on the error object otherwise) rather than leaving the next reader to redo it.

Re-verified the primitive against storage rather than only the deltas. next_cursor is still the max over the SQL-returned rows, computed before min_level filters (storage.py:783 / server.py:738), so the safe/unsafe filter table in the guide is accurate for the stated reason. Guard order still puts the ahead-of-tip ceiling before the rewind check, int(tip) if tip else 0 still fires on an empty bus, position = previous is read before the write and is what all four cursor refusals report, and the _format_result ack branch still requires cursor and previous_cursor while sitting below the error branch, so refusals still render as ERROR. Loop termination still holds on both snippets: has_more=True implies a full raw batch, so the ack strictly advances and the next peek reads a strictly higher window.

Previously Addressed (Filtered)

Both round-17 items (the unguarded pending index, and stating why the loop tests has_more rather than the surfaced event count) are Implemented and verified in the current diff. Rounds 1-16 remain fixed; none re-raised. Whether the shell twin should bail explicitly rather than rely on set -e / jq semantics was discussed and settled in the round-17 exchange, so it is not re-raised either.

Verdict

APPROVE - No blocking findings. Converged at round 18 — remaining feedback is non-blocking. Nothing here names a way the code produces a wrong result, and nothing is a regression against an earlier round. One documentation suggestion posted inline.

---

Automated review by Claude Code

[evansenter]

Round 18 taken, in 113c6b1.

Fair catch on my own fix: the bail I added last round exited without a word, so a hook copying it would treat an abandoned backlog as indistinguishable from a clean drain — the same class of silent-failure problem #140 was filed about, reintroduced in the example rather than the code. The snippet now surfaces error and hint before breaking.

The comparison to the CLI twin is the useful framing: that one is loud by construction because events and ack exit non-zero, and the Python form had no equivalent — so the two halves of the same documented flow disagreed on whether a mid-drain refusal is worth mentioning. And hint carrying the actionable half ("re-register or stop polling") is exactly why the deleted-session contract has one; dropping it on the floor wastes the thing that shape exists to deliver.

649 tests pass, format and lint clean.

---

Generated by Claude Code

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 19

Summary

The only change since round 18 is 113c6b12: three comment lines inside the existing if "error" in pending: branch of the guide.md drain loop — log(pending["error"], pending.get("hint")) plus the two-line reason. No behavioral surface, no new API, no test-visible change. It closes the round-18 item exactly as scoped: the bail was already there, and a bare break reads identically to a clean drain from the outside, so naming what to emit — and that hint is the actionable half — is the part a hook author would otherwise have to reconstruct.

Re-verified the primitive this round rather than only the delta. _ack_events_impl still evaluates the ahead-of-tip ceiling before the rewind check, so allow_rewind cannot climb past the tip. int(tip) if tip else 0 still refuses any positive cursor on an empty bus. position = previous is still read before the write and is what all four cursor refusals report, with tip under its own key on the one refusal that has a ceiling to report. _deleted_session_error still keys its warn set on (session_id, deleted_at, tool), so a drain hook that both polls and acks gets a line for each. The _format_result ack branch still requires cursor and previous_cursor and still sits below the error branch, so refusals render as ERROR: rather than as a cursor move. Loop termination holds on both snippets: has_more=True implies a full raw batch, so each pass ack strictly advances and the next peek reads a strictly higher window; the guide guard if pending["next_cursor"] and the shell twin guard if [ -n "$CUR" ] both leave the nothing-to-ack pass a no-op rather than an error.

Previously Addressed (Filtered)

The round-18 item (the refusal branch breaking silently) is Implemented and verified in the current diff. Rounds 1-17 remain fixed; none re-raised.

Verdict

APPROVE - No blocking findings. Converged at round 19 — remaining feedback is non-blocking. Nothing in this delta, or in the re-verification of the primitive behind it, names a way the code produces a wrong result, and nothing is a regression against an earlier round. No inline comments this round: a documentation-only three-line addition that does exactly what the prior round asked has no finding in it worth raising.

---

Automated review by Claude Code