refactor(chat): give tool replay reconciliation its own module - #3439
Conversation
Fix round 1 on the tool-replay-reconciliation extraction: replace the private duplicated isTextPart/isProviderVisibleReasoningPart/getFilePart in tool-replay-reconciliation.ts (which had already drifted from conversation.ts's isTextPart) with a real move of TextPartLike, ReasoningPartLike, isTextPart, isReasoningPart, isProviderVisibleReasoningPart, and getFilePart into tool-part-parsing.ts. conversation.ts re-exports the public predicate surface (isReasoningPart, isTextPart) so external callers are unaffected.
…exports Zero behavior change. Renames tool-part-parsing.ts to message-part-parsing.ts since it owns text/reasoning/file predicates alongside tool parts, not just tool parts; drops the unused ReasoningPartLike/TextPartLike re-export from conversation.ts; documents why the ChatProviderModelInputMessage import in tool-replay-reconciliation.ts must stay type-only; and inlines isTransientToolState as a direct export instead of a trailing export block. Re-pins the FILE_SIZE_CEILINGS in ban-chat-antipatterns.ts to match.
… lint The comment claimed lint:module-boundaries never scans src/chat/, so a type-only import slip would not be caught. This is false. The lint script scans all of src/ and runs findCyclicEdges on the entire graph, which correctly detects cycles including this import. Updated comment to accurately state: the import must stay type-only because a value import creates a real cycle with conversation.ts. deno check won't catch this (types erase at emit), but lint:module-boundaries will.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change centralizes chat field access and message-part parsing, adds tool replay reconciliation, and updates conversation and message preparation to use the shared modules. Tests cover parsing, field access, and reconciliation behavior. ChangesChat processing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderMessages
participant ToolReplayReconciliation
participant MessagePartParsing
participant Conversation
ProviderMessages->>ToolReplayReconciliation: Supply replayed messages
ToolReplayReconciliation->>MessagePartParsing: Parse tool calls and results
MessagePartParsing-->>ToolReplayReconciliation: Return normalized parts
ToolReplayReconciliation-->>Conversation: Return match and supersession collections
Conversation->>MessagePartParsing: Convert provider-visible parts
MessagePartParsing-->>Conversation: Return normalized provider content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/chat/part-field-access.test.ts (1)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the JSON fallback branch of
stringifyUnknown.The current cases exercise the string,
undefined, andbigintbranches. The final branch delegates tostringifyChatJson, which is the only branch with an external dependency. Add a record or array case, and a case fortoJsonValue, so a change injson-value.tsfails here rather than in a provider conversion test.♻️ Proposed additional cases
it("returns strings unchanged and stringifies other primitives", () => { assertEquals(stringifyUnknown("already"), "already"); assertEquals(stringifyUnknown(undefined), "undefined"); assertEquals(stringifyUnknown(10n), "10"); + assertEquals(stringifyUnknown({ a: 1 }), '{"a":1}'); + assertEquals(stringifyUnknown(null), "null"); }); + + it("converts values into JSON-safe values", () => { + assertEquals(toJsonValue({ a: [1, "b"] }), { a: [1, "b"] }); + });Add
toJsonValueto the import list at lines 4-11.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/part-field-access.test.ts` around lines 39 - 43, Extend the stringifyUnknown test in the existing test case to cover the JSON fallback via a record or array input, and add a separate assertion using toJsonValue after importing it. Verify both cases produce the expected stringified output, ensuring changes to the JSON conversion path are caught here.src/chat/tool-replay-reconciliation.test.ts (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the matching pass, not the shape of its return value.
Lines 19-20 assert that a
WeakSethashasand aWeakMaphasget. Those assertions hold for every implementation that returns the declared type, so they cannot fail and give no signal.The module's contract is the identity-based pass: a result matches the nearest pending call with the same id, a later call with the same id supersedes the earlier call and its matched result, a call after content from an earlier message starts a new batch, and a transient call with a matching result is preserved. CONTEXT.md now names this module the single owner of those decisions. None of the four behaviors is tested here. A regression in supersession would surface only in a provider-conversion assertion far from the cause.
♻️ Proposed direct contract test
+ it("matches a result to its call and supersedes an earlier duplicate call", () => { + const firstCall = { + type: "tool-search", + toolCallId: "c1", + toolName: "search", + state: "input-available", + input: {}, + }; + const secondCall = { + type: "tool-search", + toolCallId: "c1", + toolName: "search", + state: "input-available", + input: {}, + }; + const result = { type: "tool_result", toolCallId: "c1", output: { ok: true } }; + + const matches = findProviderVisibleToolReplayMatches([ + { id: "m1", role: "assistant", parts: [firstCall] }, + { id: "m2", role: "assistant", parts: [secondCall] }, + { id: "m3", role: "tool", parts: [result] }, + ]); + + assertEquals(matches.matchedToolCallParts.has(secondCall), true); + assertEquals(matches.matchedToolResultParts.has(result), true); + assertEquals(matches.matchedToolResultNames.get(result), "search"); + assertEquals(matches.supersededToolCallParts.has(firstCall), true); + assertEquals(matches.preservedTransientToolParts.has(secondCall), true); + });Adjust the message literals to match the
ChatProviderModelInputMessagefields required by the repository's type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/tool-replay-reconciliation.test.ts` around lines 17 - 23, Replace the nonfunctional WeakSet/WeakMap shape assertions in the empty-history test with contract-focused cases for findProviderVisibleToolReplayMatches: verify nearest same-id result matching, later-call supersession, batch reset after intervening content, and preservation of transient calls with matching results. Use message literals containing the repository-required ChatProviderModelInputMessage fields, and assert identities and match sets rather than collection method types.src/chat/message-part-parsing.test.ts (1)
10-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the non-obvious tool-call predicates.
The module exports 10 symbols. This suite covers 3. The highest-consequence gap is
hasSelfContainedRawToolCallResult. It special-casesstate === "error"with nooutputand noerrorTextand returnsfalse, whilebuildToolResultOutputalone would return a non-nullerror-textresult for that state. That predicate feedsselfContainedResultintool-replay-reconciliation.ts, which drives supersession and therefore which provider messages are dropped. A future edit can remove that guard without failing any test in this file.Also consider
getFilePart, which returnsnullunless aurlfield is present.Separately, the name at line 39 describes only the first assertion. Lines 41-44 assert the
output-errormapping. Split the block or widen the name.♻️ Proposed additional cases
+ it("treats a bare error state as carrying no self-contained result", () => { + const call = getRawToolCallPart({ + type: "tool_call", + toolCallId: "c1", + toolName: "search", + state: "error", + }); + assertEquals(call?.toolCallId, "c1"); + assertEquals(hasSelfContainedRawToolCallResult(call!), false); + }); + + it("treats an error state with errorText as self-contained", () => { + const call = getRawToolCallPart({ + type: "tool_call", + toolCallId: "c1", + toolName: "search", + state: "error", + errorText: "bad", + }); + assertEquals(hasSelfContainedRawToolCallResult(call!), true); + }); + + it("requires a url on file parts", () => { + assertEquals(getFilePart({ type: "file", mediaType: "text/plain" }), null); + assertEquals( + getFilePart({ type: "image", mediaType: "image/png", url: "u" })?.data, + "u", + ); + });Add
getFilePart,getRawToolCallPart, andhasSelfContainedRawToolCallResultto the import list at lines 4-8.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/message-part-parsing.test.ts` around lines 10 - 46, Add focused tests in the message-part-parsing suite for getFilePart, getRawToolCallPart, and especially hasSelfContainedRawToolCallResult, covering the state "error" case with neither output nor errorText and asserting false, plus representative valid cases. Rename or split the existing "returns null output..." test so its name also accurately describes the output-error mapping, and include the new symbols in the import list.src/chat/part-field-access.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlias
ChatJsonValueasJsonValue.json-value.tsexports an equivalent type. Preserve the existingJsonValueexport for callers while using the shared definition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/part-field-access.ts` around lines 10 - 12, Replace the local JsonValue definition in part-field-access.ts with an alias or re-export of the shared JsonValue type from json-value.ts, while preserving the existing JsonValue export for current callers.src/chat/tool-replay-reconciliation.ts (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMove the shared provider input types to a lower-level module, or remove the lint safeguard claim.
conversation.tsimportstool-replay-reconciliation.ts, which importsChatProviderModelInputMessageback as a type.lint:module-boundariesrecognizes type-only imports but intentionally excludes them from cycle detection, so it will not report this cycle. Moving only the interface is insufficient because it depends on the related input-part types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/tool-replay-reconciliation.ts` around lines 21 - 25, Resolve the circular dependency between conversation.ts and tool-replay-reconciliation.ts by moving ChatProviderModelInputMessage together with all related input-part types it depends on into a lower-level shared module, then update both consumers to import them from that module; alternatively, remove the inaccurate lint safeguard comment. Ensure the type-only import no longer points back through conversation.ts.src/chat/message-part-parsing.ts (1)
242-259: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRename the local
datavariable tourl.getFilePartreads onlypart.urland returns that value as bothdataandurl, matching the pre-refactor implementation.ChatUiMessagePartrequiresurlfor file parts, so do not add adatafallback here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/message-part-parsing.ts` around lines 242 - 259, Rename the local data variable to url in getFilePart, keeping it sourced only from part.url with no data fallback. Use url for the required-value check and return both data and url fields from that same value, preserving ChatUiMessagePart compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/chat/message-part-parsing.test.ts`:
- Around line 10-46: Add focused tests in the message-part-parsing suite for
getFilePart, getRawToolCallPart, and especially
hasSelfContainedRawToolCallResult, covering the state "error" case with neither
output nor errorText and asserting false, plus representative valid cases.
Rename or split the existing "returns null output..." test so its name also
accurately describes the output-error mapping, and include the new symbols in
the import list.
In `@src/chat/message-part-parsing.ts`:
- Around line 242-259: Rename the local data variable to url in getFilePart,
keeping it sourced only from part.url with no data fallback. Use url for the
required-value check and return both data and url fields from that same value,
preserving ChatUiMessagePart compatibility.
In `@src/chat/part-field-access.test.ts`:
- Around line 39-43: Extend the stringifyUnknown test in the existing test case
to cover the JSON fallback via a record or array input, and add a separate
assertion using toJsonValue after importing it. Verify both cases produce the
expected stringified output, ensuring changes to the JSON conversion path are
caught here.
In `@src/chat/part-field-access.ts`:
- Around line 10-12: Replace the local JsonValue definition in
part-field-access.ts with an alias or re-export of the shared JsonValue type
from json-value.ts, while preserving the existing JsonValue export for current
callers.
In `@src/chat/tool-replay-reconciliation.test.ts`:
- Around line 17-23: Replace the nonfunctional WeakSet/WeakMap shape assertions
in the empty-history test with contract-focused cases for
findProviderVisibleToolReplayMatches: verify nearest same-id result matching,
later-call supersession, batch reset after intervening content, and preservation
of transient calls with matching results. Use message literals containing the
repository-required ChatProviderModelInputMessage fields, and assert identities
and match sets rather than collection method types.
In `@src/chat/tool-replay-reconciliation.ts`:
- Around line 21-25: Resolve the circular dependency between conversation.ts and
tool-replay-reconciliation.ts by moving ChatProviderModelInputMessage together
with all related input-part types it depends on into a lower-level shared
module, then update both consumers to import them from that module;
alternatively, remove the inaccurate lint safeguard comment. Ensure the
type-only import no longer points back through conversation.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b1e3794-f46a-4cf2-8844-f380845a4ae8
📒 Files selected for processing (10)
CONTEXT.mdscripts/lint/ban-chat-antipatterns.tssrc/chat/conversation.tssrc/chat/message-part-parsing.test.tssrc/chat/message-part-parsing.tssrc/chat/message-prep.tssrc/chat/part-field-access.test.tssrc/chat/part-field-access.tssrc/chat/tool-replay-reconciliation.test.tssrc/chat/tool-replay-reconciliation.ts
…3443) * test(chat): cover findProviderVisibleToolReplayMatches directly 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. * test(chat): fix false fence in tool-replay-reconciliation case 9 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.
Cover the JSON fallback in stringifyUnknown and toJsonValue, the hasSelfContainedRawToolCallResult error guard that drives supersession in tool-replay-reconciliation, and the getFilePart url requirement. Alias JsonValue to ChatJsonValue instead of re-declaring the same recursive shape, and name the getFilePart local after the field it actually reads.
|
All six nitpicks from the review body triaged. Four applied in 67ffa89, one was already fixed, one is rejected on evidence. Applied
Already fixed
Rejected
The exclusion at Verified: |
2 participants
Pure extraction, zero behaviour change. Splits
src/chat/conversation.tsalong its real seams and gives the tool-replay algorithm a name.Why
conversation.tswas a 1563-line grab-bag hiding a genuinely deep algorithm: ~260 lines that reconcile tool-call/result history by part object identity (WeakSet/WeakMap, single pass) to decide which occurrences are authoritative for provider conversion. It had no name and no home. The same file also exported generic value guards that had nothing to do with conversations.What changed
A three-layer ladder, each importing only downward:
part-field-access.tsunknownmessage-part-parsing.tstool-replay-reconciliation.tsconversation.tsNet +80 lines across
src/chat/. This is deliberately not a line-reduction change — the gain is that three named single-purpose modules replace one grab-bag, andCONTEXT.mdnow records two new domain terms.Also adds LOC ceilings for these five files to
ban-chat-antipatterns.ts.src/chat/previously had no ceiling at all, which is howconversation.tsreached 1563 lines. Precedent:modules/server/module-server.tswas refactored once and then regrew 1138 → 1806 lines across six follow-on PRs, ending larger than before. Extraction without a ceiling regresses.Evidence
deno task test:unit: 3804 passed / 27918 steps / 0 failed / 1 ignored — identical to the pre-refactor baseline of 3801/27907 plus exactly the 11 new test steps. Held at every commit.deno task verify:quick: exit 0.deno task dupes: 232 groups, unchanged from branch start — the refactor introduced no duplication.conversation.tsadds exactly 22 lines, all imports and re-exports; every other line is a deletion. No logic in that file was edited anywhere on the branch.toRecord, the one constructing function inpart-field-access.ts, is not imported by the reconciliation module.86 LOC exceeds ceiling 66) and then clear.Every task was implemented and reviewed in separate passes, with a fix round where review found a problem.
Notes for reviewers
conversation.tskeeps re-exportinggetStringField,isRecord,stringifyUnknown—isRecordalone has ~8 importers acrosssrc/agent/. Consolidating those is a separate increment (there are ~76 privateisRecordcopies repo-wide); this branch gives the canonical one a home first.tool-replay-reconciliation.tsimportsChatProviderModelInputMessagefromconversation.tsasimport type. It must stay type-only — a value import would create a real cycle.deno checkwould not catch that (types erase at emit) butlint:module-boundarieswill; there is a comment at the import saying so.Known follow-ups (deferred, not blocking)
findProviderVisibleToolReplayMatches— the algorithm this branch exists to expose — has only a smoke test. A direct table-driven test is the highest-value follow-up and is now easy to write against the extracted interface.message-part-parsing.tslack JSDoc (all were private and undocumented before; the verbatim-move rule preserved that).conversation.ts's ~423-line provider-conversion block is the obvious next seam, leaving ~600 lines that finally match the file's name. Sequencing matters: extract theChatProviderModelInput*types first, or the type-only back-edge simply relocates instead of disappearing.Summary by CodeRabbit
Improvements
Quality