Skip to content

fix(tui): dropped DM turn takes the reply after its own user row - #170

Merged
allenhutchison merged 3 commits into
mainfrom
dm-reply-after-user-row
Oct 29, 2026
Merged

allenhutchison merged 3 commits into
mainfrom
dm-reply-after-user-row

Conversation

@allenhutchison

@allenhutchison allenhutchison commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Human overview

When a Pepper DM stream drops, the TUI re-reads history to find the reply. It used to take any new assistant row, so a reply to another device's message or a proactive message could be shown as the answer. It now takes only the first assistant row created after the turn's own user row and in the same trajectory (the server already sends trajectory_id on history rows and links a turn's user row and reply in one transaction), and does not guess when it can't find that row. Client-only, per the decision on the issue.

Fixes #168

AI reviewer

  • DmTurn in src/pepper_cli/tui/state.py: new _own_user_row finds the earliest user row not in the turn's snapshot whose stripped text equals the stripped text sent. reply_id (dropped stream, phase checking) returns the earliest assistant row not in the snapshot with created_at strictly after that user row and trajectory_id equal to the user row's, else None.
  • ChatHistoryEntry in api_client.py now mirrors trajectory_id (already in the vendored schema as a required nullable field; defaults to None when absent). Server side, chat_service.finalize_turn links the user row and writes the assistant row with the same id in one transaction, and reminders/scheduled announcements are written with trajectory_id=None. So a linked user row matches only its own reply. Residual: while the user row is unlinked (reply not written yet, the error path, or a server without the field), a later row with no trajectory can still match. That is the limit accepted on tui: a dropped DM turn takes any new assistant row as its reply #168. reply_landed now delegates to reply_id, and found_reply already did, so the match is single-sourced.
  • Consistency: app._on_dm (seen set for the Pepper dot), _settle_turns (assistant_id for mark_seen) and the P1b strip all use reply_id/found_reply, so they follow the new match with no app changes.
  • Failure posture: no identifiable user row (text altered by the server, row not yet in history) leaves the turn checking until the existing give-up window, then the existing "no reply yet" outcome. Strict > on created_at means a timestamp tie also does not match; this errs toward not guessing.
  • Known limit: two turns with identical text where an earlier one is not in the snapshot could match the earlier user row. The snapshot includes earlier answered ids and only one turn is active at a time, so this is narrow.
  • user_landed: for a checking turn it now uses the same own-row match, so another device's new user row no longer hides this turn's pending bubble. streaming keeps the old rule; the reply frame names the user row there.
  • Tests: tests/test_tui_state.py adds the unrelated-row-first case, the unidentifiable-user-row case, the same-trajectory case (a reminder and another device's reply between the user row and the answer), the unlinked-user-row case, and the pending-bubble case; tests/test_api_client.py checks trajectory_id parsing; both fail on the old code (checked by stashing the source change). The existing tests in test_tui_state.py and test_tui_apps_state.py were updated for realistic timestamps and a user row.
  • code-review (medium), run on this worktree's origin/main...HEAD diff: no change-worthy findings. It re-raised the accepted limits (text-equality match, midnight, identical earlier text, the unlinked window), the decided-against ids-first option, a moot user_id-on-settle point (a settled turn leaves turns), and a minor repeated scan per poll. /simplify: one pass, replaced sorts with min.
  • Verification: ruff format --check, ruff check, ty check src/pepper_cli/ clean; pytest --cov: 2656 passed, 2 skipped, 95.10% coverage (floor 88).

Checklist (repo-specific)

  • uv run ruff format --check and uv run ruff check clean (no new lint ignores)
  • uv run ty check src/pepper_cli/ clean
  • uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered
  • Server-contract changes: none, client-only
  • No token or credential is logged, printed, or put in a fixture
  • README and the bundled SKILL.md updated if commands or output changed: no command or output change

🤖 Generated with Claude Code

https://claude.ai/code/session_01QNDeh8M8jKfHhtWGJwkxTm

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no new actionable issues remain.

Summary

Dropped DM streams now find the reply after their own user row and require a matching trajectory_id.

  • Dropped DM turns match replies to their own history rows.

In the PR description, allenhutchison accepts the remaining limits: rows without a trajectory can still match an unlinked user row, altered text or timestamp ties can prevent a match, and an earlier identical message outside the snapshot can be mistaken for the current one.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[DM stream drops] --> B[Read history]
    B --> C{Matching new user row?}
    C -->|No| F[Keep checking until timeout]
    C -->|Yes| D{Later assistant row with same trajectory?}
    D -->|No| F
    D -->|Yes| E[Use reply and finish turn]
Loading

Reviews (2) · Last reviewed commit: "fix(tui): a dropped turn's sending bubbl..." · Reviewed by Greptile

After a stream drop the turn took the first new assistant row as its reply, so an
unrelated row (another device's reply, a proactive message) settled it and showed
in the reply strip. It now identifies its own user row (first new user row whose
text is what it sent) and takes the first new assistant row created after it. With
no such user row it stays "checking".

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNDeh8M8jKfHhtWGJwkxTm
@vycari-review

vycari-review Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Vycari Review (shadow)

Shadow round 2 on 109d231. This comment is non-gating and needs no response: it is collected to measure agreement with the current reviewer. The round's verdict is the Vycari Review (shadow) check run.

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

Judge's notes

The pending-bubble finding is fixed, and the trajectory check now stops replies to other turns from being taken. One narrow P1 is left, "An unlinked turn still settles on a reminder": before the real reply links the user row, a reminder can still be shown as the answer. The PR discloses this as an accepted limit.

P1 · VR-a4d21b20 · An unlinked turn still settles on a reminder

src/pepper_cli/tui/state.py lines 997–1000 · symbol DmTurn.reply_id · lens concurrency-time · status fix-incomplete

The new trajectory guard treats two absent trajectory IDs as a match. While the user's row remains unlinked, an unrelated assistant row with no trajectory still satisfies every predicate and permanently settles the dropped turn.

Evidence: state.py:999 allows None == None; tests/test_tui_state.py:568-578 explicitly exercises an unlinked user row. The history schema permits null trajectory_id (contracts/api-v1.openapi.json:2127-2138), so field presence does not exclude this state. app.py:813-814 considers the selected row seen, and app.py:2349-2355 removes the checking turn and stores the selected text and assistant ID. A later transaction linking the user row cannot correct the retired turn. The added guard therefore fixes the prior unrelated-trajectory case but leaves the same harm reachable before linkage.

Re-review (fix-incomplete): The trajectory_id guard now rejects replies to other turns that carry their own trajectory. But the server links the user row only when it writes the reply. Until then both ids are None, so a reminder that lands between the user row and the real reply (the prior's exact scenario) still matches and settles the turn.

Failure scenario

The terminal sends 'q' and loses its stream. History contains its user row with trajectory_id=None while Pepper is still working. A reminder then lands with trajectory_id=None. The next poll selects the reminder as this turn's answer and removes the turn from reconciliation. When the real reply subsequently lands with its trajectory, the pane's reply strip continues showing the reminder as the answer.

How this was verified (read-only-reasoning)

Read state.py reply_id lines 989-1001. The trajectory_id equality accepts None == None, as the candidate says. Did not open the app.py settle path.

Proving test

Create a checking pane turn and feed _on_dm history containing its matching user row and a later reminder, both with trajectory_id=None. Assert the turn remains checking and the reminder is not marked seen. Then feed history linking the user row to T1 and adding its T1 reply; assert the strip shows that reply. The first assertion fails on head.

Confirmed fixed

  • VR-126592a3 · Pending user bubble is hidden while a dropped turn's own row is missing, because user_landed still matches any new user row

Comment thread src/pepper_cli/tui/state.py Outdated
allenhutchison and others added 2 commits October 29, 2026 03:35
…ajectory

The server links a turn's user row to its trajectory and writes the reply
with the same trajectory_id in one transaction, and /chat/history already
sends the field. Mirror it on ChatHistoryEntry and require a dropped turn's
candidate reply to share its user row's trajectory_id, so a reminder or a
reply to another device created after the user row is no longer taken.
The reply_id docstring now states the guarantee as it is.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNDeh8M8jKfHhtWGJwkxTm
While a dropped turn is checking, user_landed took any new user row, so
another device's message hid this turn's provisional bubble. Use the same
own-row match as the reply.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNDeh8M8jKfHhtWGJwkxTm
@allenhutchison

Copy link
Copy Markdown
Contributor Author

Round 1 summary (head 109d231):

  • Greptile P2, reply_id docstring overstates the guarantee: fixed in e8872e8 (narrowed; see the thread).
  • Vycari Review P1 VR-a4d21b20, an unrelated assistant row between the user row and the real reply still matches: the first-after-user-row rule is the client-only approach decided on tui: a dropped DM turn takes any new assistant row as its reply #168, and its residual is accepted there. Tightened without guessing in e8872e8: /chat/history already sends trajectory_id, and finalize_turn links the user row and writes the reply with the same id in one transaction, so a dropped turn now also requires the candidate's trajectory_id to equal its user row's. VR's scenario (reminder at 10:01:02 between the user row at 10:01:00 and the answer at 10:01:05) now returns the answer; test_a_dropped_turn_takes_only_the_reply_in_its_user_rows_trajectory is that proving test. What remains: while the user row is not linked yet (reply not written, the error path, or a server without the field), a later row with no trajectory can still match. That is within the accepted limit.
  • Vycari Review P2 VR-126592a3, user_landed takes any new user row while checking: fixed in 109d231. A checking turn's bubble now waits for its own user row; streaming keeps the old rule, since the reply frame names the row there.
  • code-review (medium) on this worktree's diff: no change-worthy findings; details in the PR body.

@allenhutchison

Copy link
Copy Markdown
Contributor Author

Round 2 (head 109d231): Greptile 5/5 on the head, checks green, no open threads.

  • Vycari Review (shadow) VR-a4d21b20, re-raised as fix-incomplete: an unlinked user row (reply not written yet) still matches a later reminder with no trajectory. This is the residual accepted on tui: a dropped DM turn takes any new assistant row as its reply #168 and named in the reply_id docstring. Held at 2 rounds (review.impasseRounds = 2), so I am not changing it again here. The option a maintainer could choose: settle an unlinked match only at the give-up window, so a linked reply wins if it lands in time, at the cost of the error-path reply showing about 120s later.

@allenhutchison
allenhutchison added this pull request to the merge queue Oct 29, 2026
@allenhutchison

Copy link
Copy Markdown
Contributor Author

Maintainer decision (2026-10-29): keep e8872e8's trajectory_id match; leave the no-trajectory window open (VR-a4d21b20 accepted as-is, no follow-up). Ready to merge.

Merged via the queue into main with commit 052a216 Oct 29, 2026
5 checks passed
@allenhutchison
allenhutchison deleted the dm-reply-after-user-row branch October 29, 2026 04:39
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.

tui: a dropped DM turn takes any new assistant row as its reply

1 participant