Keep an answered server request answered across a reload - #127
Merged
Conversation
Answering a server request recorded nothing on the server. A request leaves the pending set only when the harness emits its own resolved event, which arrives on the harness's schedule and may never arrive at all. Until then the replayed ServerRequestReceived still reads as outstanding, so a reload rendered the card actionable again — and answering a second time routes a stale id to the harness, which errors. That is the same defect already fixed for approvals, left open on the path beside it. The answer is now recorded against the in-flight turn the moment it is routed, and the reconnect snapshot carries answered_server_requests. Excluding them from pending_server_requests is not enough on its own: the request still rides along in `accumulated`, and replaying that renders an actionable card unless the client is told it was answered. The client seeds the answered set before replaying, exactly as it does for approvals. Answered requests are also excluded from the connect bootstrap, so a latecomer is not told a thread is waiting on input the user already gave. Server requests had no browser coverage at all despite having real UI — a card, per-question controls, Continue/Cancel and a resolved state. They do now, driven by a scripted user-input request whose harness deliberately never resolves it, which is precisely the window in question. Two tests: an answered request comes back resolved and never actionable, and the mirror image, an unanswered one comes back still actionable rather than being swallowed. The first was confirmed to fail without this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe protocol now carries answered server-request IDs in live-turn snapshots. Server routing records answers immediately, reconnect snapshots exclude them from pending activity, and the UI restores their resolved state after reload. Replay, unit, integration, specification, and Playwright coverage were added or updated. ChangesServer-request reconnect persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Routes
participant HarnessRegistry
participant LiveBufferStore
Browser->>Routes: ServerRequestResponse
Routes->>HarnessRegistry: respond_server_request
HarnessRegistry-->>Routes: thread_id
Routes->>LiveBufferStore: resolve_server_request
Browser->>LiveBufferStore: reconnect snapshot
LiveBufferStore-->>Browser: answered_server_requests and filtered pending requests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the same defect AR1 already fixed for approvals, left open on the path beside it.
The problem
Answering a server request recorded nothing on the server. A request leaves the pending set only when the harness emits its own
ServerRequestResolved, which arrives on the harness's schedule and may never arrive at all. Until then the replayedServerRequestReceivedstill reads as outstanding, so a reload rendered the card actionable again — and answering a second time routes a stale id to the harness, which errors.The change
The answer is recorded against the in-flight turn the moment it is routed (
LiveBufferStore::resolve_server_request, called from theServerRequestResponsehandler —respond_server_requestnow returns theThreadIdso the caller knows where to record it).Excluding answered requests from
pending_server_requestsis not sufficient on its own: the request still rides along inaccumulated, and replaying that renders an actionable card unless the client is told it was answered. SoLiveTurnSnapshotalso carriesanswered_server_requests, and the client seeds that set before replaying accumulated events — exactly the shapeanswered_approvalsalready has.Answered requests are also excluded from the connect bootstrap added in #126, so a latecomer isn't told a thread is waiting on input the user already gave.
Coverage
Server requests had no browser coverage at all, despite real UI: a card, per-question controls, Continue/Cancel, and a resolved state. They do now, driven by a scripted
requestUserInputwhose harness deliberately never resolves it — precisely the window in question.resolved, both actions disabled, no error, and the sidebar stops claiming the thread is waiting on input.answered_server_request_is_not_pending_after_reconnectpins the snapshot contract in Rust, with a harness flag that suppresses resolution.The answered test was confirmed to fail without the fix — I disabled the server-side recording, rebuilt, and watched it fail (no
resolvedclass, card actionable) while the unanswered test kept passing.Verification
483 Rust tests passing with every target compiling,
fmt --checkandclippy --all-targets --all-features -D warningsclean, 30/30 Playwright across 10 consecutive full runs.One caveat worth stating: during this work I hit a cluster of 3 identical failures in
thread-selection.spec.ts(a 10s timeout waiting for the scripted reply) that I could not reproduce in 10 subsequent full runs, including one deliberately launched under build contention to test that theory. I don't have an explanation and I'm not claiming one — flagging it rather than pretending the suite is understood.Follow-up worth considering
Approvals and server requests are nearly the same concept, and this PR makes that hard to miss — the two now have parallel resolution state, parallel snapshot fields, and parallel client sets. The codebase already blurs the line: MCP tool-call approvals arrive from Codex as
requestUserInputserver requests and get promoted to approval cards by marker.The concrete cost today is that the "blocked ≠ busy" work from #125 doesn't apply here — a thread (or sub-agent) waiting on user input still ranks and renders as merely running, and fires no notification:
31(same as running)!oNot fixed here to keep this PR to one defect, but it's a small presentation-layer change and probably the right next step.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests