[bniladridas]

Human overview

A quiet first run could keep its first-run state set because the clear only happened when a backlog was pushed. On reconnect, a missed old event could then be mistaken for replayed history and dropped. This clears the flag when the catch-up completes, whether or not anything was skipped. Refs #122 (item 2 of 4).

AI reviewer

Details for agents: what the diff does, invariants, verification, checklist

What the diff does. Two commits on top of 8b6ce9b, both in src/pepper_cli/claude_channel.py and tests/test_claude_channel.py (+44/-2 across the two files; the source adds one comment block and changes one condition). b0cb937 moves self.first_run = False out of _flush_backlog, where it sat after the early return for an empty backlog, and into _caught_up, which runs when the replay boundary is reached. 3dd17ca changes the comment branch of _handle_frame: the catch-up now ends only when the comment text is exactly ping, where the old check ended it for every comment except connected. Two tests pin the work. test_an_old_event_replayed_after_a_first_run_still_owes_a_resync covers a quiet first run: connect, receive the scope and a ping, drop, reconnect with an old unknown event and a ping, and assert one resync because the first ping cleared first_run. test_a_non_ping_comment_does_not_end_the_first_run_catch_up covers the comment boundary: a skipped old message and : keepalive before a drop, then an old group.renamed and a real ping after the reconnect, asserting one first-run backlog and no resync.

Invariants. For --start beginning, only the catch-up ends first-run suppression; the head-start path ends it in _live_edge when the stream reaches the live edge. Only a : ping is a catch-up boundary: the server sends one only while it has nothing past the caller's position, the contract recorded in #250. : connected opens a connection, and any other comment, including a proxy keepalive or a frame from a newer server, establishes neither. A cursor frame is still a skip marker that advances the position and never ends the replay. On the beginning-replay path, first_run now means the process has not completed its first replay, not that a backlog was pushed. _flush_backlog still clears the skipped-message state, _live_edge still clears first_run for a head start, and _is_first_run_history is unchanged. The catch-up path is the single place that ends first-run suppression for --start beginning; it also flushes any owed backlog or resync notice and persists the cursor.

Alternatives rejected. Leaving the clear in _flush_backlog leaks the flag on a quiet first run. Treating any comment except connected as the boundary, which is the old behavior, lets an unrelated comment end the replay early: it clears first_run, flushes the backlog mid-replay, and makes an old event replayed after it owe a spurious resync. Adding replay_pending, _end_replay, a cursor-boundary rule, or a ping-before-head guard was dropped after the maintainer confirmed that a ping means caught up, that start=head opens with the head cursor before any event, and that a cursor frame is only a skip marker. The docs half of that decision landed separately in #250 (b4d1693).

Failure posture. Ending the catch-up too early can produce a spurious resync and the wrong backlog wording, and it can drop an old event without a trace; ending it only on a ping makes an unrelated comment harmless. Failing to end it at all can silently drop a missed old event on reconnect, which is the bug in b0cb937. If a server never sends a ping after the replay, the catch-up stays open, which is the existing behavior for a quiet stream; the contract sends a ping about every 15 seconds while idle, and this PR adds no timer of its own.

Where to look hardest. The comment branch of _handle_frame must compare exactly with ping and must still ignore connected without ending the catch-up. The head-start live-edge path uses the scope and cursor frames, not comments, so it is untouched; the new boundary test covers --start beginning, and the existing head-start tests cover the rest. The new test fails on the old != "connected" check (1 failed locally) and passes with the exact comparison. Also check that no other path clears first_run for a --start beginning replay.

Verification. macOS, Python 3.13 via uv, on 3dd17ca:

• uv run python -m pytest tests/test_claude_channel.py -q: 135 passed in 20.95s
• The new test alone against the old comment check: 1 failed, then 1 passed with the fix restored
• Targeted lifecycle selection with the fix: 6 passed
• uv run python -m pytest -n auto -q: 3324 passed, 2 skipped in 146.14s
• uv run ruff format --check: 148 files already formatted
• uv run ruff check: All checks passed!
• uv run ty check src/pepper_cli/: All checks passed!

All checks passed on 3dd17ca (run 43550566728), including the ci job's coverage ratchet. Both automated reviews left no findings on the two changed files. Nothing was run against a live Pepper server.

Deferred work. Items 1, 3, and 4 of #122 remain separate. This PR does not touch the clock-skew item. The replay-boundary documentation is no longer deferred because #250 landed it. The comment-boundary defect was found in a cross-consumer audit of the event stream and is fixed here rather than deferred.

Checklist (repo-specific)

• uv run ruff format --check and uv run ruff check clean (no new lint ignores): 148 files already formatted, All checks passed!
• uv run ty check src/pepper_cli/ clean: All checks passed!
• uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered: local uv run python -m pytest -n auto -q gave 3324 passed, 2 skipped; the floor is unchanged; CI run 43550566728's Test and measure coverage and Coverage ratchet steps passed on 3dd17ca
• Server-contract changes: /api/v1 usage stays additive-only, # MIRROR: comments updated, unknown fields and frames still tolerated: none; no /api/v1 call or # MIRROR: dataclass changed, and the client now tolerates unknown comments instead of treating them as a boundary
• No token or credential is logged, printed, or put in a fixture
• README and the bundled SKILL.md updated if commands or output changed: not applicable; no command or output changed
• Help: help/cli.md updated for a new or changed command, or not user-facing because the change only affects first-run replay state and adds regression tests

[vycari-review]

Vycari Review

Round 5 on 3dd17ca. The verdict is the Vycari Review check run, and each open finding is an inline thread. Reply on every P0 and P1 thread: Fixed in <sha>, or Disputed: with a reason. P2s are the author's call: fix one, or reply Deferred: #<issue> (the PR's one follow-up issue) or Declined: with a reason, and VR stops re-raising it. From round 2 on, VR raises no new P2s: it checks the fixes and looks for P0s and P1s.

Open: P0 0 · P1 0 · P2 0 · disputed 0 · confirmed fixed 0 · withdrawn 0

Hand-off: Greptile requested on 3dd17ca.

No open findings this round.

P2s, judge's notes and closed findings

Judge's notes

No candidate findings were raised.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-12T03:37:12.007129Z	a50b391	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a50b391031

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[bniladridas]

Replay completion needs a contract decision

Round 3 raises two P1s and one P2, and they share one cause: the current patch uses a cursor frame as both a skip/resume marker and an end-of-replay marker. The client contract does not support the second use.

• FeedFrame describes a cursor as "a resume point past events the agent was not in scope for" (src/pepper_cli/api_client.py:477-480).
• The only opening-cursor guarantee is for start=head: the stream "opens with a cursor frame naming it" (api_client.py:3240-3241).
• Design section 7.4 still names "the first heartbeat after replay" as the backlog boundary (planning/claude-code-channel.md:335-337), not a cursor.
• history_expired is the one explicit "window gone" signal, and its server emission is unverified (src/pepper_cli/claude_channel.py:746-750).

I would separate the work into three independent concerns:

1. First-run suppression. Old replayed events must not trigger resync or change the backlog wording before the replay boundary is known. The current first_run clear belongs in the catch-up path, not _flush_backlog, but only when the catch-up is confirmed.
2. Head initialization. Only the documented opening head cursor should establish the head position. An ordinary event must not silently substitute for it. This is the Codex P2 gap: _handle_frame clears head_pending on any frame (claude_channel.py:887), so an event before the opening cursor lets a later ping call _caught_up.
3. Backlog notification. Give this its own completion state. Do not trigger it from _caught_up() until the replay-completion contract is established. That keeps the backlog trigger separate from the suppression fix.

Candidate client-side changes, pending contract confirmation:

• Keep a ping and a disconnect as non-boundaries.
• Keep the first_run clear out of _flush_backlog, but call it only at a confirmed catch-up boundary.
• Separate "opening head cursor still expected" from "head learned for reconnect purposes".
• Do not treat arbitrary cursor frames as replay completion. Advance the cursor and leave first-run suppression alone unless the frame is a contract-defined boundary.

These are candidate changes, not a complete fix. None of them solves --start beginning, because the client still has no completion signal there.

Two contract questions for the maintainer:

1. For --start beginning, what marks replay completion, and how is that frame or ordering distinguished from a mid-replay skip cursor?
2. For --start head, is the opening cursor guaranteed before every event, and what should the client do with an older server that ignores start=head and sends no cursor?

I have not changed code or resolved the three threads. #232 stays open. I will keep the two P1 threads open until the contract question is settled.

[allenhutchison]

Thanks for digging into this so carefully. Your contract questions were exactly the right ones to ask. Answers:

1. --start beginning: a : ping is only sent while the server has nothing past your position, so the first ping after connecting means replay is complete. A cursor frame is only a skip marker: it can arrive mid-replay, or not at all.
2. --start head: the stream always opens with the head cursor frame, after scope and before any event, so a ping can't come before it.

Given that, could you keep commit 1 (moving the first_run clear into _caught_up, which is the real #122 item 2 fix) and drop the replay_pending/cursor-boundary change and the ping-before-head guard? A ping-ends-replay test with an empty backlog would pin the fix. We'll also make the client docs say "a ping means caught up", so the next reader doesn't hit this ambiguity.

---

Generated by Claude Code

[vycari-review]

@greptileai review

[vycari-review]

@greptileai review