-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 10 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"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -328,28 +328,18 @@ pub trait Provider: Send + Sync { | |
| ) -> Result<(Message, ProviderUsage), ProviderError>; | ||
|
|
||
| // Default implementation: use the provider's configured model | ||
| // This method filters messages to only include agent_visible ones | ||
| async fn complete( | ||
| &self, | ||
| system: &str, | ||
| messages: &[Message], | ||
| tools: &[Tool], | ||
| ) -> Result<(Message, ProviderUsage), ProviderError> { | ||
| let model_config = self.get_model_config(); | ||
|
|
||
| // Filter messages to only include agent_visible ones | ||
| let agent_visible_messages: Vec<Message> = messages | ||
| .iter() | ||
| .filter(|m| m.is_agent_visible()) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| self.complete_with_model(&model_config, system, &agent_visible_messages, tools) | ||
| self.complete_with_model(&model_config, system, messages, tools) | ||
| .await | ||
| } | ||
|
|
||
| // Check if a fast model is configured, otherwise fall back to regular model | ||
| // This method filters messages to only include agent_visible ones | ||
| async fn complete_fast( | ||
| &self, | ||
| system: &str, | ||
|
|
@@ -359,15 +349,8 @@ pub trait Provider: Send + Sync { | |
| let model_config = self.get_model_config(); | ||
| let fast_config = model_config.use_fast_model(); | ||
|
|
||
| // Filter messages to only include agent_visible ones | ||
| let agent_visible_messages: Vec<Message> = messages | ||
| .iter() | ||
| .filter(|m| m.is_agent_visible()) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| match self | ||
| .complete_with_model(&fast_config, system, &agent_visible_messages, tools) | ||
| .complete_with_model(&fast_config, system, messages, tools) | ||
| .await | ||
| { | ||
| Ok(result) => Ok(result), | ||
|
|
@@ -379,7 +362,7 @@ pub trait Provider: Send + Sync { | |
| e, | ||
| model_config.model_name | ||
| ); | ||
| self.complete_with_model(&model_config, system, &agent_visible_messages, tools) | ||
| self.complete_with_model(&model_config, system, messages, tools) | ||
| .await | ||
| } else { | ||
| Err(e) | ||
|
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. nice. there's also snowflake & anthropic that come with their own message formats
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 |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,8 +32,8 @@ const DATA_FIELD: &str = "data"; | |
| pub fn format_messages(messages: &[Message]) -> Vec<Value> { | ||
| let mut anthropic_messages = Vec::new(); | ||
|
|
||
| // Convert messages to Anthropic format | ||
| for message in messages { | ||
| // Filter messages to only include agent_visible ones and convert to Anthropic format | ||
| for message in messages.iter().filter(|m| m.is_agent_visible()) { | ||
|
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. to be honest, if we go through the loop here anyway, we might as well skip them in place rather than prefilter
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. Goose claims: The current implementation with |
||
| let role = match message.role { | ||
| Role::User => USER_ROLE, | ||
| Role::Assistant => ASSISTANT_ROLE, | ||
|
|
@@ -96,18 +96,14 @@ pub fn format_messages(messages: &[Message]) -> Vec<Value> { | |
| MessageContent::SummarizationRequested(_) => { | ||
| // Skip | ||
| } | ||
| MessageContent::Thinking(thinking) => { | ||
| content.push(json!({ | ||
| TYPE_FIELD: THINKING_TYPE, | ||
| THINKING_TYPE: thinking.thinking, | ||
| SIGNATURE_FIELD: thinking.signature | ||
| })); | ||
| MessageContent::Thinking(_) => { | ||
| // Skip thinking blocks - they should not be sent to the API | ||
| // Thinking is only for internal use | ||
| continue; | ||
| } | ||
| MessageContent::RedactedThinking(redacted) => { | ||
| content.push(json!({ | ||
| TYPE_FIELD: REDACTED_THINKING_TYPE, | ||
| DATA_FIELD: redacted.data | ||
| })); | ||
| MessageContent::RedactedThinking(_) => { | ||
| // Skip redacted thinking - internal use only | ||
| continue; | ||
|
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'm not sure about this. it sounds reasonable, but do we want to make a change we don't really really understand for this if we want to cherry-pick?
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. Agreed, reverted but wanted to just call this out as somewhere to investigate. |
||
| } | ||
| MessageContent::Image(_) => continue, // Anthropic doesn't support image content yet | ||
| MessageContent::FrontendToolRequest(tool_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.
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.