refactor(chat): give provider message conversion its own module - #3449
Conversation
Splits RawToolCallMessagePart, RawToolResultMessagePart, ChatProviderModelInputToolCallPart, ChatProviderModelInputToolResultPart, ChatProviderModelInputPart, and ChatProviderModelInputMessage out of conversation.ts into provider-input-types.ts. This removes the deliberate type-only back-edge tool-replay-reconciliation.ts had into conversation.ts, and sets up the next increment to move provider conversion out of conversation.ts without creating a cycle. Pure type move, zero behaviour change.
Move convertUiMessagesToProviderModelMessages and its private helpers (buildToolNameMap, resolveRawToolResultPart, shouldSkipTransientToolCall, convertSystemMessage, convertUserMessage, convertAssistantMessage, convertToolMessage, ProviderToolResultContent) out of conversation.ts into src/chat/provider-message-conversion.ts, verbatim. conversation.ts keeps the persisted-conversation format and how UI messages become stored parts; provider-message-conversion.ts is now the single owner of turning replay history into the ordered message list a provider sees. Consumers (message-prep.ts, compat.ts, chat-request.test.ts, conversation.test.ts) repoint directly at the new module rather than via a re-export from conversation.ts.
conversation.ts shrank from 1018 to 557 lines and split off two new leaf modules; tighten ceilings to match actual sizes and add entries for provider-input-types.ts and provider-message-conversion.ts so regrowth fails CI immediately. Record Provider Message Conversion as a domain term in CONTEXT.md.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe change extracts provider message conversion and its input types from ChangesProvider Message Conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatHistory
participant convertUiMessagesToProviderModelMessages
participant ToolReplayReconciliation
participant ProviderModelMessages
ChatHistory->>convertUiMessagesToProviderModelMessages: provide ordered messages
convertUiMessagesToProviderModelMessages->>ToolReplayReconciliation: reconcile tool occurrences
ToolReplayReconciliation-->>convertUiMessagesToProviderModelMessages: return authoritative tool matches
convertUiMessagesToProviderModelMessages->>ProviderModelMessages: emit ordered provider messages
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/hosted/chat-request.test.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the internal source alias.
Line 2 crosses a module boundary from
src/agent/hostedtosrc/chat. Replace the relative path with#veryfront/chat/provider-message-conversion.ts.Proposed fix
-import { convertUiMessagesToProviderModelMessages } from "../../chat/provider-message-conversion.ts"; +import { convertUiMessagesToProviderModelMessages } from "`#veryfront/chat/provider-message-conversion.ts`";As per coding guidelines, use
#veryfront/*for internal source imports.🤖 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/agent/hosted/chat-request.test.ts` at line 2, Update the import of convertUiMessagesToProviderModelMessages in the test to use the `#veryfront/chat/provider-message-conversion.ts` internal source alias instead of the relative path.Sources: Coding guidelines, Learnings
🤖 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/agent/hosted/chat-request.test.ts`:
- Line 2: Update the import of convertUiMessagesToProviderModelMessages in the
test to use the `#veryfront/chat/provider-message-conversion.ts` internal source
alias instead of the relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bebb0193-d79d-469d-8ad4-8bad9fdd4dcb
📒 Files selected for processing (11)
CONTEXT.mdscripts/lint/ban-chat-antipatterns.tssrc/agent/hosted/chat-request.test.tssrc/chat/compat.tssrc/chat/conversation.test.tssrc/chat/conversation.tssrc/chat/message-prep.tssrc/chat/provider-input-types.tssrc/chat/provider-message-conversion.tssrc/chat/tool-replay-reconciliation.test.tssrc/chat/tool-replay-reconciliation.ts
Finishes the split started in #3439. Zero behaviour change.
src/chat/conversation.ts993 → 557 lines (1563 → 557 across both increments). What remains is one coherent subject that finally matches the file's name: the persisted conversation format and how UI messages become stored parts.What moved
provider-message-conversion.ts(new)provider-input-types.ts(new)ChatProviderModelInput*shapes both conversion and reconciliation needprovider-message-conversion.tsexports exactly one function —convertUiMessagesToProviderModelMessages. The other seven (buildToolNameMap,resolveRawToolResultPart,shouldSkipTransientToolCall, and the fourconvert*) are private to it.The sequencing is the design
tool-replay-reconciliation.tshad a deliberate type-only import ofChatProviderModelInputMessagefromconversation.ts— the one back-edge left by #3439.Moving the conversion out without moving those types first would not have removed that edge; it would have inverted it into a genuine two-way dependency between the conversion module and
conversation.ts. Strictly worse than the single type-only edge that existed before.So the types moved first, in their own commit (
94b22420c), and that commit's success criterion was a single grep:grep -n "conversation" src/chat/tool-replay-reconciliation.tsmust return empty. It does. The back-edge is gone, not relocated.They went into a new leaf module rather than
src/chat/types.ts, because that file is a publisheddeno.jsonexport and putting them there would have widened the public API surface — cleanup, not scope growth.Resulting graph, every arrow one-way:
The conversion module's single edge into
conversation.tsis for the source-ID plumbing (getProviderModelMessageSourceId/withProviderModelMessageSourceId), which stays there by design.conversation.tsimports nothing back.Evidence
deno task test:unit: 3811 passed / 27980 steps / 0 failed / 1 ignored — identical before and after every commit.conversation.test.ts(40 cases) andmessage-prep.test.ts(42 cases) are the fence.convertAssistantMessage(5282 characters identical).deno task verify:quickexit 0.deno task dupes232 groups, unchanged.lint:module-boundariesreports 0 cyclic edges; baseline untouched.Two corrections found by measurement
isToolCallPart/isToolResultPart/extractTextFromMessagewere used by the conversion code. They aren't — none is called anywhere inconversation.ts, including inside the moved functions. They stayed put. (Their real consumers aremessage-prep.tsandfinal-step-fallback.ts;extractTextFromMessagehas no production consumer at all, only its test.)conversation.test.tsimports the entry point directly across 44 assertions. Missing it would have broken the fence test's compilation.Consumers were repointed directly rather than re-exported —
conversation.tsis not a published export and the list is short and known. #3439 already shipped a dead re-export that review caught; not repeating it.Ceilings
All
src/chat/*ceilings re-pinned to realwc -lvalues, with entries added for both new modules. Two had gone slack and were tightened (tool-replay-reconciliation.ts294→291,part-field-access.ts66→65).The gate was proven to fire, not assumed: appending 20 blank lines to the new module produced
and reverting returned exit 0. This matters because
modules/server/module-server.tsregrew 1138 → 1806 after its own refactor — past its pre-refactor size — for want of exactly this.CONTEXT.mdrecords Provider Message Conversion as a domain term.Summary by CodeRabbit