-
Notifications
You must be signed in to change notification settings - Fork 223
Fix: Allow 'tool' role in ChatMessageInput #501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return { | ||
| role: role ?? "user", | ||
| content: parts, | ||
| }; | ||
| } as ChatMessageData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tool role converted to wrong content shape
Allowing ChatMessageRoleData in ChatMessageInput means callers can now pass { role: "tool", content: "..." }, but chatMessageInputToChatMessageData still always builds text/file parts and then forcibly casts the result to ChatMessageData here. Per ChatHistoryData.ts (lines 139‑157) tool messages must contain toolCallResult parts, so a tool message created through Chat.from now produces an invalid ChatMessageData object. Any downstream validation with chatMessageDataSchema (e.g., when cloning a ChatMessage) or the LLM request path will throw or drop the tool result because the expected toolCallResult part is missing.
Useful? React with 👍 / 👎.
- Changed ChatMessageInput.role type from hardcoded union to ChatMessageRoleData - Updated chatMessageInputSchema to use chatMessageRoleDataSchema - Fixes issue lmstudio-ai#486: ChatMessageInput does not allow 'tool' role
cf17f0e to
d1d7f6d
Compare
otchlan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read the CLA Document and I hereby sign the CLA
Problem
Fixes #486
The
ChatMessageInputinterface did not allow thetoolrole, even though this role was defined inChatMessageRoleDataand is necessary for proper tool calling workflows.This prevented users from:
Solution
Changed
ChatMessageInputto useChatMessageRoleDatatype instead of hardcoded union type:rolefield to useChatMessageRoleDatachatMessageInputSchemato usechatMessageRoleDataSchemaTesting
Created a reproduction test that verifies
Chat.from()now accepts messages withrole: "tool":Changes
packages/lms-client/src/ChatInput.ts: Updated interface and schema