Conversation
Add 9 direct cases for the replay reconciliation matcher: identity vs structural equality, supersession via both the pending-call-match path and the self-contained-call path, transient preservation timing, batch starts, name-mismatch rejection, and unresolved pending calls. Only the two prior smoke cases previously exercised this module directly; the rest of its coverage came indirectly through conversation.test.ts.
Case 9 was titled as pinning that a user-message boundary drops stale pending tool calls, but its staleCall/freshCall shared a toolCallId, so id-based eviction (removePendingCallsWithId) and toolCallsById-based supersession masked the boundary logic entirely — freshCall was also self-contained, so no result ever needed to match against pendingCalls. Mutants deleting the boundary flush or the user-visible-content check left the whole suite green. Kept the old fixture under an honest title (it does correctly pin toolCallsById supersession of an already-evicted call) and added a new case with a fixture that actually needs pendingCalls to retain a boundary- crossing entry: staleCall and its late result share a toolCallId used nowhere else, so same-id eviction can't do the work for it. Also added a one-line positive control to the isCompatibleToolResultName mismatch case so it can't pass vacuously against a rotted fixture.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds test fixtures and comprehensive coverage for tool replay reconciliation, including matching, identity checks, supersession, transient calls, stale-call eviction, batch boundaries, and user-message boundaries. ChangesTool replay reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
1eba258
into
refactor/chat-tool-replay-extraction
|
Closing as superseded by #3439. The two commits from this branch were squashed onto Both PRs target No work is lost. |
|
Correction to my previous comment: this was already merged, not superseded, and my note misread the state. It merged into The rest stands: #3439 carries this work plus the review nitpicks addressed in 67ffa89. |
Writes the first real test suite for
findProviderVisibleToolReplayMatches. Tests only — no source changes.Why
#3439 extracted a ~100-line algorithm that decides which tool-call/result occurrences in replay history are authoritative for provider conversion — matching by part object identity via seven
WeakSet/WeakMapcollections. It shipped with 2 smoke tests (isTransientToolStateclassification, and empty history). The algorithm itself was covered only indirectly, through provider conversion inconversation.test.ts/message-prep.test.ts.That was the point of extracting it. This is the follow-through.
What was added
10 cases (2 existing smoke tests kept; 12 total): pair matching, supersession via both paths, transient preservation, name-mismatch rejection, unmatched pending call, batch start,
toolCallsByIdsupersession after pending-queue eviction, the user-message batch boundary, and object identity.The case that matters most
Identity discrimination. It builds a matched call/result pair, then checks structurally identical but distinct clones against
matchedToolCallParts,matchedToolResultParts, andmatchedToolResultNames— asserting the real object reads true/has-a-name and the clone reads false/undefined.Its exclusive value: it is the only case that fails if the returned
WeakSet/WeakMapcollections are swapped for structural-equality lookups. No consumer's compile would catch that — callers only use.has()and.get(). Without this case, a change that "optimized" by comparing parts structurally, or by cloning them internally, would pass every other test while silently breaking replay matching.Test quality was verified by mutation, not by reading
The suite was reviewed by building 9 mutants of the implementation (as copies — tracked source was never modified) and mapping which cases catch which:
isCompatibleToolResultName→ always truematchedToolCallPartsThat exercise found a real defect in the first draft: a case titled as fencing the user-message boundary survived both boundary mutants. Its assertions were correct, but they pinned something else — its fixture shared a
toolCallId, soremovePendingCallsWithIdevicted the stale entry regardless and the boundary was never exercised. It has been retitled to what it actually pins, and a genuine boundary case added alongside it (verified to flip under the mutant).A documented limitation
The new boundary case fences the "user text stops counting as provider-visible content" regression. It does not fence deleting the earlier-message flush inside the visible-content branch: for a call-less user message,
pendingCountBeforeSameMessageVisibleContentstill captures the pre-message length, so the end-of-loopsplicefallback evicts exactly the same entries — the two paths are provably equivalent in that shape. Discriminating it would need a boundary message with calls interleaved with visible content. This is stated in the case's comment rather than left implied.Behaviour pinned, not fixed
A call already evicted from
pendingCallsas stale still lands insupersededToolCallPartswhen a later same-toolCallIdoccurrence arrives —toolCallsByIdaccumulates every occurrence and is never pruned, whilependingCallsis pruned by three separate paths.Reviewed as harmless, and arguably the safer behaviour: the flag only fires when another occurrence with the same
toolCallIdappears later, and emitting two tool-call blocks under one id is exactly what this module exists to prevent. Consumers checked:conversation.ts:816(pushToolCall, reached only after the transient-skip guard),:889/:928/:949(provider tool messages), andmessage-prep.ts:883-885(stripPendingToolParts). Pinned as characterization; no source change.Evidence
deno task test:unit: branch baseline 3804 passed / 27918 steps → 3804 / 27928, zero regressions.deno lint,deno task verify:quick,deno task lint:chat-ratchetsall exit 0.src/chat/tool-replay-reconciliation.test.ts.Summary by CodeRabbit