-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Next camp tool notifications #5726
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
Conversation
|
Closing while I look into a backend solution that doesn't modify messages on the front end |
…essages array on the front end
| }); | ||
|
|
||
| let confirmation = Message::user().with_tool_confirmation_request( | ||
| let confirmation = Message::assistant().with_tool_confirmation_request( |
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 looked up what llms expect for this role and there is no clear standard. Goose says using Message::assistant() for tool confirmations is fine and arguably more semantically correct. The assistant is asking for permission, so it makes sense for it to come from the assistant role. Since these messages are filtered out before sending to the LLM, the role only matters for frontend rendering.
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.
can't we/shouldn't we just set this to agent invisible?
| }); | ||
|
|
||
| let confirmation = Message::user().with_tool_confirmation_request( | ||
| let confirmation = Message::assistant().with_tool_confirmation_request( |
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.
can't we/shouldn't we just set this to agent invisible?
|
|
||
| const resultsCache = new Map<string, { messages: Message[]; session: Session }>(); | ||
|
|
||
| export type NotificationEvent = Extract<MessageEvent, { type: 'Notification' }>; |
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 would flip the dependency around here and move this to message.ts since it is a related type
| throwOnError: true, | ||
| }); | ||
| } catch (error) { | ||
| console.error('Failed to send tool cancellation to backend:', error); |
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.
this will probably not fail, but we should be louder about it than logging since the agent I think will hang now
| stopStreaming: () => void; | ||
| sessionLoadError?: string; | ||
| tokenState: TokenState; | ||
| notifications: NotificationEvent[]; |
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 think the only consumer of this converts this to a map sorted by request_id - I would suggest to do that on construction
Summary
Add back tool notifications support for manual approval mid chat.
Verified locally
follow up for #5706