fix: clear stale completedRequests after conversation compaction#18
Open
LucasRoesler wants to merge 5 commits intomainfrom
Open
fix: clear stale completedRequests after conversation compaction#18LucasRoesler wants to merge 5 commits intomainfrom
LucasRoesler wants to merge 5 commits intomainfrom
Conversation
agentState.completedRequests accumulated an entry for every approved/denied tool call and was never pruned. After the SDK compacted conversation history, the corresponding entries remained, causing orphan tool cards to render in the frontend. The server-side fix (nulling agentState on microcompact_boundary) had a race condition: the CLI's buffered updateAgentState calls would immediately re-post the stale state after the server cleared it. Fix: move cleanup to the CLI, which is the only party with live knowledge of compaction events: - claudeRemoteLauncher.ts: detect microcompact_boundary in onMessage and call updateAgentState to reset requests and completedRequests to empty objects - claudeRemote.ts: add onCompact callback, fired when a user-triggered /compact command result arrives; the launcher wires this to the same reset - sessionHandlers.ts: remove the racing handleMessageHistoryModification call from the microcompact_boundary handler (the CLI reset is now authoritative) - syncEngine.ts: add handleMessageHistoryModification to archiveSession and deleteSession so agentState is wiped on those explicit lifecycle events Adds integration tests for both the server handler and the CLI compaction callbacks, and documents the approach with tradeoffs in docs/planning/STRAY_TOOL_CALLS.md.
LucasRoesler
force-pushed
the
hapi-fix-old-stray-tool-calls
branch
from
d8c1d21 to
f745cee
Compare
…e tests - Remove 'microcompact' from MessageHistoryModificationReason — no remaining call site after the CLI-driven cleanup replaced the server-side handler - Update jsdoc to drop the microcompact bullet from the when-to-call list - Remove spurious refreshSession() from deleteSession success branch — the call did a DB read and cache insert immediately before deleteSession wiped the entry, emitting a spurious session-updated event in the process; refreshSession remains correct in archiveSession where the session persists - Add handleMessageHistoryModification store-level tests covering the agentState null-clear path that archiveSession and deleteSession rely on
… boundary After a /compact or autonomous microcompact, the server records the boundary message's seq on the session as compaction_boundary_seq. The messages API uses that value as an afterSeq floor so clients see only post-compaction messages on initial load. When the user scrolls to the top, hasMoreBeforeBoundary signals that pre-compaction history exists. The next "load older" request sends includeAll=true to bypass the floor and fetch pre-boundary messages (the existing microcompact event card already serves as a visible divider). /clear resets compaction_boundary_seq to NULL so clearing still shows an empty conversation.
Three issues found in code review: 1. hasMessagesBeforeSeq used row !== undefined but bun:sqlite .get() returns null (not undefined) on no match. Fixed to row !== null. 2. The boundary existence check was seq <= ? (inclusive of the boundary message itself), making hasMoreBeforeBoundary always true when any boundary is set, even for sessions with no pre-boundary history. Fixed to seq < ? to match the seq > afterSeq filter on the visible window — the boundary message is excluded in both directions. 3. archiveSession did not clear compaction_boundary_seq. A resumed session would silently hide all messages behind the stale boundary. Now clears on archive alongside the agentState wipe. Also adds regression tests covering the edge case where the boundary message is the only message in the session.
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
1 participant
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.
Summary
agentState.completedRequestsaccumulated every approved/denied tool call forever, never pruned after the SDK compacted conversation history — causing orphan tool cards in the frontendmicrocompact_boundary) raced with the CLI's bufferedupdateAgentStatecalls, which re-posted stale state immediately afterChanges:
claudeRemoteLauncher.ts: detectmicrocompact_boundaryinonMessage, callupdateAgentStateto resetrequests/completedRequestsclaudeRemote.ts: addonCompactcallback, fired when a user-triggered/compactresult arrives; launcher wires it to the same resetsessionHandlers.ts: remove racinghandleMessageHistoryModificationcall from themicrocompact_boundarypath (CLI reset is now authoritative)syncEngine.ts: addhandleMessageHistoryModificationtoarchiveSessionanddeleteSessionfor explicit lifecycle eventsdocs/planning/STRAY_TOOL_CALLS.md: documents root cause, approach, tradeoffs, and next stepsTest plan
bun test server/src/socket/handlers/cli/sessionHandlers.test.ts— 11 tests passvitest run cli/src/claude/claudeRemote.compaction.test.ts— 5 tests pass