-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add filtering for agentVisible: false messages on streaming providers #4847
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
Changes from 9 commits
45da1a2
3f2262f
ea2af98
dfff91e
d2decd2
e5d9980
77f1123
a9ae41d
25ee0c4
8aeae4e
0619a9d
c4d167a
891ffb1
f9318d7
61d83e0
e324b87
65a118d
b98dd63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| use crate::conversation::message::{Message, MessageContent}; | ||
| use crate::conversation::message::{Message, MessageContent, MessageMetadata}; | ||
| use rmcp::model::Role; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::collections::HashSet; | ||
|
|
@@ -102,6 +102,42 @@ impl Conversation { | |
| self.0.clear(); | ||
| } | ||
|
|
||
| /// Filter messages based on visibility criteria. | ||
| /// | ||
| /// # Arguments | ||
| /// * `filter` - A closure that takes a MessageMetadata and returns whether to include the message | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// # use goose::conversation::{Conversation, message::Message}; | ||
| /// # let conversation = Conversation::new_unvalidated(vec![ | ||
| /// # Message::user().with_text("Hello") | ||
| /// # ]); | ||
| /// // Get agent-visible messages regardless of user visibility (don't care about user_visible) | ||
| /// let agent_messages = conversation.filtered_messages(|meta| meta.agent_visible); | ||
| /// | ||
| /// // Get messages visible ONLY to agent (not visible to user) | ||
| /// let agent_only = conversation.filtered_messages(|meta| meta.agent_visible && !meta.user_visible); | ||
| /// ``` | ||
| pub fn filtered_messages<F>(&self, filter: F) -> Vec<Message> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, this is not really what I meant - meant just pass it a visibility level for the messages and then return those
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. visibility level kind of needs some kind of mask though on MessageMetadata? Don't quite follow.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i meant something like pub fn visible_messages(for: User|Agent) but maybe that's not the API you imagined.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do that but I was imagining adding more tags to message metadata like summary marker, notification so wanted to keep them synced.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, some other time, let's just drop this for now. or keep it but remove the comment
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack. leaving for now but will follow up and use it + might update it a bit. |
||
| where | ||
| F: Fn(&MessageMetadata) -> bool, | ||
| { | ||
| self.0 | ||
| .iter() | ||
| .filter(|msg| filter(&msg.metadata)) | ||
| .cloned() | ||
| .collect() | ||
| } | ||
|
|
||
| pub fn agent_visible_messages(&self) -> Vec<Message> { | ||
| self.filtered_messages(|meta| meta.agent_visible) | ||
| } | ||
|
|
||
| pub fn user_visible_messages(&self) -> Vec<Message> { | ||
| self.filtered_messages(|meta| meta.user_visible) | ||
| } | ||
|
|
||
| fn validate(self) -> Result<Self, InvalidConversation> { | ||
| let (_messages, issues) = fix_messages(self.0.clone()); | ||
| if !issues.is_empty() { | ||
|
|
@@ -531,4 +567,142 @@ mod tests { | |
| let (_fixed, issues) = run_verify(messages); | ||
| assert_eq!(issues.len(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_filtered_messages() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these tests don't add anything
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| use crate::conversation::message::MessageMetadata; | ||
|
|
||
| let conversation = Conversation::new_unvalidated(vec![ | ||
| Message::user().with_text("User message 1"), | ||
| Message::assistant() | ||
| .with_text("Assistant message 1") | ||
| .with_visibility(true, true), // Both visible | ||
| Message::user() | ||
| .with_text("User message 2") | ||
| .with_metadata(MessageMetadata::agent_only()), // Only agent visible | ||
| Message::assistant() | ||
| .with_text("Assistant message 2") | ||
| .with_metadata(MessageMetadata::user_only()), // Only user visible | ||
| Message::user() | ||
| .with_text("User message 3") | ||
| .with_metadata(MessageMetadata::invisible()), // Neither visible | ||
| ]); | ||
|
|
||
| // Test filtering for agent-visible messages | ||
| let agent_messages = conversation.filtered_messages(|meta| meta.agent_visible); | ||
| assert_eq!(agent_messages.len(), 3); // Messages 0, 1, 2 | ||
| assert_eq!(agent_messages[0].as_concat_text(), "User message 1"); | ||
| assert_eq!(agent_messages[1].as_concat_text(), "Assistant message 1"); | ||
| assert_eq!(agent_messages[2].as_concat_text(), "User message 2"); | ||
|
|
||
| // Test filtering for user-visible messages | ||
| let user_messages = conversation.filtered_messages(|meta| meta.user_visible); | ||
| assert_eq!(user_messages.len(), 3); // Messages 0, 1, 3 | ||
| assert_eq!(user_messages[0].as_concat_text(), "User message 1"); | ||
| assert_eq!(user_messages[1].as_concat_text(), "Assistant message 1"); | ||
| assert_eq!(user_messages[2].as_concat_text(), "Assistant message 2"); | ||
|
|
||
| // Test filtering for messages visible to both | ||
| let both_visible = | ||
| conversation.filtered_messages(|meta| meta.user_visible && meta.agent_visible); | ||
| assert_eq!(both_visible.len(), 2); // Messages 0, 1 | ||
| assert_eq!(both_visible[0].as_concat_text(), "User message 1"); | ||
| assert_eq!(both_visible[1].as_concat_text(), "Assistant message 1"); | ||
|
|
||
| // Test filtering for invisible messages | ||
| let invisible = | ||
| conversation.filtered_messages(|meta| !meta.user_visible && !meta.agent_visible); | ||
| assert_eq!(invisible.len(), 1); // Message 4 | ||
| assert_eq!(invisible[0].as_concat_text(), "User message 3"); | ||
|
|
||
| // Test "don't care" scenario - agent must be visible, don't care about user | ||
| let agent_regardless_of_user = conversation.filtered_messages(|meta| meta.agent_visible); | ||
| assert_eq!(agent_regardless_of_user.len(), 3); // Messages 0, 1, 2 | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_convenience_filter_methods() { | ||
| use crate::conversation::message::MessageMetadata; | ||
|
|
||
| let conversation = Conversation::new_unvalidated(vec![ | ||
| Message::user().with_text("User message 1"), | ||
| Message::assistant() | ||
| .with_text("Assistant message 1") | ||
| .with_metadata(MessageMetadata::agent_only()), | ||
| Message::user() | ||
| .with_text("User message 2") | ||
| .with_metadata(MessageMetadata::user_only()), | ||
| Message::assistant() | ||
| .with_text("Assistant message 2") | ||
| .with_metadata(MessageMetadata::invisible()), | ||
| ]); | ||
|
|
||
| // Test agent_visible_messages() | ||
| let agent_messages = conversation.agent_visible_messages(); | ||
| assert_eq!(agent_messages.len(), 2); // Messages 0, 1 | ||
| assert_eq!(agent_messages[0].as_concat_text(), "User message 1"); | ||
| assert_eq!(agent_messages[1].as_concat_text(), "Assistant message 1"); | ||
|
|
||
| // Test user_visible_messages() | ||
| let user_messages = conversation.user_visible_messages(); | ||
| assert_eq!(user_messages.len(), 2); // Messages 0, 2 | ||
| assert_eq!(user_messages[0].as_concat_text(), "User message 1"); | ||
| assert_eq!(user_messages[1].as_concat_text(), "User message 2"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_specific_visibility_combinations() { | ||
| let conversation = Conversation::new_unvalidated(vec![ | ||
| Message::user() | ||
| .with_text("Both visible") | ||
| .with_visibility(true, true), | ||
| Message::assistant() | ||
| .with_text("Agent only") | ||
| .with_visibility(false, true), // user_visible: false, agent_visible: true | ||
| Message::user() | ||
| .with_text("User only") | ||
| .with_visibility(true, false), // user_visible: true, agent_visible: false | ||
| Message::assistant() | ||
| .with_text("Neither visible") | ||
| .with_visibility(false, false), | ||
| ]); | ||
|
|
||
| // Get messages visible ONLY to agent (agent_visible: true, user_visible: false) | ||
| let agent_only = | ||
| conversation.filtered_messages(|meta| meta.agent_visible && !meta.user_visible); | ||
| assert_eq!(agent_only.len(), 1); | ||
| assert_eq!(agent_only[0].as_concat_text(), "Agent only"); | ||
|
|
||
| // Get messages visible ONLY to user (user_visible: true, agent_visible: false) | ||
| let user_only = | ||
| conversation.filtered_messages(|meta| meta.user_visible && !meta.agent_visible); | ||
| assert_eq!(user_only.len(), 1); | ||
| assert_eq!(user_only[0].as_concat_text(), "User only"); | ||
|
|
||
| // Get messages visible to BOTH (user_visible: true, agent_visible: true) | ||
| let both_visible = | ||
| conversation.filtered_messages(|meta| meta.user_visible && meta.agent_visible); | ||
| assert_eq!(both_visible.len(), 1); | ||
| assert_eq!(both_visible[0].as_concat_text(), "Both visible"); | ||
|
|
||
| // Get messages visible to NEITHER (user_visible: false, agent_visible: false) | ||
| let neither_visible = | ||
| conversation.filtered_messages(|meta| !meta.user_visible && !meta.agent_visible); | ||
| assert_eq!(neither_visible.len(), 1); | ||
| assert_eq!(neither_visible[0].as_concat_text(), "Neither visible"); | ||
|
|
||
| // Get all agent-visible messages (regardless of user visibility) | ||
| // This includes "Both visible" and "Agent only" | ||
| let all_agent_visible = conversation.filtered_messages(|meta| meta.agent_visible); | ||
| assert_eq!(all_agent_visible.len(), 2); | ||
| assert_eq!(all_agent_visible[0].as_concat_text(), "Both visible"); | ||
| assert_eq!(all_agent_visible[1].as_concat_text(), "Agent only"); | ||
|
|
||
| // Get all user-visible messages (regardless of agent visibility) | ||
| // This includes "Both visible" and "User only" | ||
| let all_user_visible = conversation.filtered_messages(|meta| meta.user_visible); | ||
| assert_eq!(all_user_visible.len(), 2); | ||
| assert_eq!(all_user_visible[0].as_concat_text(), "Both visible"); | ||
| assert_eq!(all_user_visible[1].as_concat_text(), "User only"); | ||
| } | ||
| } | ||
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.
shouldn't we move this inside get_messages_token_counts_async?
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.
done, nice call.