-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Include tool-result in chat #1682
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
feat: Include tool-result in chat #1682
Conversation
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.
❌ Changes requested. Reviewed everything up to da76592 in 2 minutes and 6 seconds
More details
- Looked at
30
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. apps/studio/electron/main/chat/index.ts:99
- Draft comment:
Consider handling potential undefined or empty toolResults to avoid runtime errors. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/studio/src/lib/editor/engine/chat/stream.ts:54
- Draft comment:
Avoid using @ts-expect-error; update type definitions instead if possible. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment points out a real code quality issue - using @ts-expect-error is generally a code smell that indicates type definitions need updating. However, we don't have access to the type definitions (they're likely in the 'ai' package) to verify if this can be fixed. The author may be using @ts-expect-error because they can't modify the external type definitions.
I might be wrong about the location of type definitions - they could be internal. Also, even if external, there might be a way to extend/augment the types locally.
While true, without seeing the type definitions or knowing if they're internal/external, we can't be confident that this suggestion is actionable by the PR author.
Delete the comment since we don't have enough context to know if updating the type definitions is actually possible or how it should be done.
3. apps/studio/electron/main/chat/index.ts:99
- Draft comment:
onStepFinish now emits 'tool-result' messages. Ensure the UI properly distinguishes these messages and consider updating type definitions to formally support 'tool-result' instead of relying on implicit handling. Also, the PR description and linked issue refer to onboarding docs, which don’t align with these changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment makes several speculative suggestions without clear evidence of problems. The code shows tool results are handled through the existing emitMessagePart mechanism, which already handles TextStreamPart. There's no evidence the UI can't handle these messages or that type definitions are inadequate. The comment about onboarding docs is out of scope for code review.
Maybe there are UI or type definition issues I can't see without access to those files. The comment might be raising valid concerns about cross-file impacts.
Without seeing clear evidence of UI or type definition problems in this file, and given the rule to ignore cross-file issues, these speculative suggestions should be removed.
Delete this comment as it makes speculative suggestions about potential issues without clear evidence, asks for verification, and raises cross-file concerns that are out of scope.
Workflow ID: wflow_BPDftbgYi0L3dUJQ
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -51,7 +51,8 @@ export class StreamResolver { | |||
resolveToolCallPart( | |||
payload: TextStreamPart<ToolSet>, | |||
): TextPart | ToolCallPart | ToolResultPart | null { | |||
if (payload.type === 'tool-call') { | |||
// @ts-expect-error tool-result is included |
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.
Avoid using @ts-expect-error
for 'tool-result'. It's better to extend the types to include 'tool-result' explicitly to maintain type safety.
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.
Thanks for the contribution @Wh1isper! This points out a bad flaw in our chat that I'll have to patch. I'm not able to push to this branch but will merge and add on :)
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: fix: error when add new color to group (onlook-dev#1683) update searching brand color picker (onlook-dev#1681) Quicker build with freestyle (onlook-dev#1685) Add tool result support (onlook-dev#1684) feat: Include tool-result in chat (onlook-dev#1682)
Description
This prevents the problem of the AI forgetting documents they have already read and causing hallucinations.
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Adds support for including
tool-result
in chat streams to prevent AI hallucinations by retaining document context.index.ts
,onStepFinish
now emitstoolResults
usingemitMessagePart()
.stream.ts
,resolveToolCallPart()
now handlestool-result
type, allowing it to be part of the content stream.emitMessagePart()
inindex.ts
now processestoolResults
.resolveToolCallPart()
instream.ts
updated to includetool-result
type.stream.ts
to indicatetool-result
handling.This description was created by
for da76592. It will automatically update as commits are pushed.