fix(acp): preserve content audience boundaries (#10493)
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
use crate::session::message_to_markdown;
|
||||
use crate::session::user_projected_message_to_markdown;
|
||||
use anyhow::{Context, Result};
|
||||
|
||||
use cliclack::{confirm, multiselect, select};
|
||||
@@ -249,7 +249,7 @@ pub async fn handle_session_export(
|
||||
let conversation = session
|
||||
.conversation
|
||||
.ok_or_else(|| anyhow::anyhow!("Session has no messages"))?;
|
||||
export_session_to_markdown(conversation.messages().to_vec(), &session.name)
|
||||
export_session_to_markdown(conversation.user_visible_messages(), &session.name)
|
||||
}
|
||||
_ => return Err(anyhow::anyhow!("Unsupported format: {}", format)),
|
||||
};
|
||||
@@ -397,7 +397,7 @@ fn export_session_to_markdown(
|
||||
// don't create a new User section - we'll attach the responses to the tool calls
|
||||
if skip_next_if_tool_response && is_only_tool_response {
|
||||
// Export the tool responses without a User heading
|
||||
markdown_output.push_str(&message_to_markdown(message, false));
|
||||
markdown_output.push_str(&user_projected_message_to_markdown(message));
|
||||
markdown_output.push_str("\n\n---\n\n");
|
||||
skip_next_if_tool_response = false;
|
||||
continue;
|
||||
@@ -416,7 +416,7 @@ fn export_session_to_markdown(
|
||||
}
|
||||
|
||||
// Add the message content
|
||||
markdown_output.push_str(&message_to_markdown(message, false));
|
||||
markdown_output.push_str(&user_projected_message_to_markdown(message));
|
||||
markdown_output.push_str("\n\n---\n\n");
|
||||
|
||||
// Check if this message has any tool requests, to handle the next message differently
|
||||
@@ -487,3 +487,35 @@ pub async fn prompt_interactive_session_selection(
|
||||
Err(anyhow::anyhow!("Invalid selection"))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use goose::conversation::message::Message;
|
||||
use goose::conversation::Conversation;
|
||||
use rmcp::model::{Content, Role};
|
||||
|
||||
#[test]
|
||||
fn markdown_export_preserves_user_audience_tool_output() {
|
||||
let user_output = Content::text("user-visible output").with_audience(vec![Role::User]);
|
||||
let assistant_output =
|
||||
Content::text("assistant-only output").with_audience(vec![Role::Assistant]);
|
||||
let conversation = Conversation::new_unvalidated([Message::user().with_tool_response(
|
||||
"tool-1",
|
||||
Ok(rmcp::model::CallToolResult::success(vec![
|
||||
user_output,
|
||||
assistant_output,
|
||||
Content::text("shared output"),
|
||||
])),
|
||||
)]);
|
||||
|
||||
let markdown = export_session_to_markdown(
|
||||
conversation.user_visible_messages(),
|
||||
&"Audience export".to_string(),
|
||||
);
|
||||
|
||||
assert!(markdown.contains("user-visible output"));
|
||||
assert!(markdown.contains("shared output"));
|
||||
assert!(!markdown.contains("assistant-only output"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -214,7 +214,13 @@ pub fn tool_request_to_markdown(req: &ToolRequest, export_all_content: bool) ->
|
||||
md
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub fn tool_response_to_markdown(resp: &ToolResponse, export_all_content: bool) -> String {
|
||||
let audience = (!export_all_content).then_some(Role::Assistant);
|
||||
tool_response_to_markdown_for_audience(resp, audience)
|
||||
}
|
||||
|
||||
fn tool_response_to_markdown_for_audience(resp: &ToolResponse, audience: Option<Role>) -> String {
|
||||
let mut md = String::new();
|
||||
md.push_str("#### Tool Response:\n");
|
||||
|
||||
@@ -225,9 +231,9 @@ pub fn tool_response_to_markdown(resp: &ToolResponse, export_all_content: bool)
|
||||
}
|
||||
|
||||
for content in &result.content {
|
||||
if !export_all_content {
|
||||
if let Some(ref role) = audience {
|
||||
if let Some(audience) = content.audience() {
|
||||
if !audience.contains(&Role::Assistant) {
|
||||
if !audience.contains(role) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
@@ -337,6 +343,19 @@ pub fn tool_response_to_markdown(resp: &ToolResponse, export_all_content: bool)
|
||||
}
|
||||
|
||||
pub fn message_to_markdown(message: &Message, export_all_content: bool) -> String {
|
||||
let audience = (!export_all_content).then_some(Role::Assistant);
|
||||
message_to_markdown_for_audience(message, export_all_content, audience)
|
||||
}
|
||||
|
||||
pub fn user_projected_message_to_markdown(message: &Message) -> String {
|
||||
message_to_markdown_for_audience(message, false, Some(Role::User))
|
||||
}
|
||||
|
||||
fn message_to_markdown_for_audience(
|
||||
message: &Message,
|
||||
export_all_content: bool,
|
||||
audience: Option<Role>,
|
||||
) -> String {
|
||||
let mut md = String::new();
|
||||
for content in &message.content {
|
||||
match content {
|
||||
@@ -371,7 +390,10 @@ pub fn message_to_markdown(message: &Message, export_all_content: bool) -> Strin
|
||||
md.push('\n');
|
||||
}
|
||||
MessageContent::ToolResponse(resp) => {
|
||||
md.push_str(&tool_response_to_markdown(resp, export_all_content));
|
||||
md.push_str(&tool_response_to_markdown_for_audience(
|
||||
resp,
|
||||
audience.clone(),
|
||||
));
|
||||
md.push('\n');
|
||||
}
|
||||
MessageContent::Image(image) => {
|
||||
|
||||
@@ -13,14 +13,14 @@ mod thinking;
|
||||
use crate::session::task_execution_display::{
|
||||
format_task_execution_notification, TASK_EXECUTION_NOTIFICATION_TYPE,
|
||||
};
|
||||
use goose::conversation::Conversation;
|
||||
use goose::conversation::{fix_conversation, Conversation};
|
||||
use std::env;
|
||||
use std::io::Write;
|
||||
use std::str::FromStr;
|
||||
use tokio::signal::ctrl_c;
|
||||
use tokio_util::task::AbortOnDropHandle;
|
||||
|
||||
pub use self::export::message_to_markdown;
|
||||
pub use self::export::{message_to_markdown, user_projected_message_to_markdown};
|
||||
pub use builder::{build_session, SessionBuilderConfig};
|
||||
use console::Color;
|
||||
use goose::agents::AgentEvent;
|
||||
@@ -61,6 +61,11 @@ use tracing::warn;
|
||||
|
||||
const GOOSE_PLANNER_CONTEXT_LIMIT: &str = "GOOSE_PLANNER_CONTEXT_LIMIT";
|
||||
|
||||
fn planner_provider_messages(plan_messages: &Conversation) -> Conversation {
|
||||
let projected_messages = plan_messages.agent_visible_messages();
|
||||
fix_conversation(Conversation::new_unvalidated(projected_messages)).0
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug)]
|
||||
struct JsonOutput {
|
||||
messages: Vec<Message>,
|
||||
@@ -246,6 +251,15 @@ pub async fn classify_planner_response(
|
||||
}
|
||||
}
|
||||
|
||||
fn planner_classification_text(response: &Message) -> Result<String> {
|
||||
let text = response.agent_visible_content().as_concat_text();
|
||||
anyhow::ensure!(
|
||||
!text.trim().is_empty(),
|
||||
"Planner returned no agent-visible text to classify"
|
||||
);
|
||||
Ok(text)
|
||||
}
|
||||
|
||||
impl CliSession {
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn new(
|
||||
@@ -523,6 +537,7 @@ impl CliSession {
|
||||
|
||||
let conversation_strings: Vec<String> = self
|
||||
.messages
|
||||
.user_visible_messages()
|
||||
.iter()
|
||||
.map(|msg| {
|
||||
let role = match msg.role {
|
||||
@@ -1057,17 +1072,30 @@ impl CliSession {
|
||||
model_config: goose_providers::model::ModelConfig,
|
||||
) -> Result<(), anyhow::Error> {
|
||||
let plan_prompt = self.agent.get_plan_prompt(&self.session_id).await?;
|
||||
let provider_messages = planner_provider_messages(&plan_messages);
|
||||
output::show_thinking();
|
||||
let (plan_response, _usage) = goose::session_context::with_session_id(
|
||||
Some(self.session_id.clone()),
|
||||
reasoner.complete(&model_config, &plan_prompt, plan_messages.messages(), &[]),
|
||||
reasoner.complete(
|
||||
&model_config,
|
||||
&plan_prompt,
|
||||
provider_messages.messages(),
|
||||
&[],
|
||||
),
|
||||
)
|
||||
.await?;
|
||||
let classifier_text = planner_classification_text(&plan_response);
|
||||
let plan_response = plan_response.user_visible_content();
|
||||
output::render_message(&plan_response, self.debug);
|
||||
output::hide_thinking();
|
||||
let classifier_text = classifier_text?;
|
||||
anyhow::ensure!(
|
||||
!plan_response.content.is_empty(),
|
||||
"Planner returned no user-visible content"
|
||||
);
|
||||
let planner_response_type = classify_planner_response(
|
||||
&self.session_id,
|
||||
plan_response.as_concat_text(),
|
||||
classifier_text,
|
||||
self.agent.provider().await?,
|
||||
self.agent
|
||||
.model_config_for_session(&self.session_id)
|
||||
@@ -1406,7 +1434,7 @@ impl CliSession {
|
||||
},
|
||||
};
|
||||
let json_output = JsonOutput {
|
||||
messages: self.messages.messages().to_vec(),
|
||||
messages: self.messages.user_visible_messages(),
|
||||
metadata,
|
||||
};
|
||||
println!("{}", serde_json::to_string_pretty(&json_output)?);
|
||||
@@ -1562,18 +1590,19 @@ impl CliSession {
|
||||
|
||||
/// Render all past messages from the session history
|
||||
pub fn render_message_history(&self) {
|
||||
if self.messages.is_empty() {
|
||||
let messages = self.messages.user_visible_messages();
|
||||
if messages.is_empty() {
|
||||
return;
|
||||
}
|
||||
|
||||
println!(
|
||||
"\n {} {}",
|
||||
console::style("↻").cyan(),
|
||||
console::style(format!("{} messages restored", self.messages.len())).dim()
|
||||
console::style(format!("{} messages restored", messages.len())).dim()
|
||||
);
|
||||
|
||||
// Render each message
|
||||
for message in self.messages.iter() {
|
||||
for message in &messages {
|
||||
output::render_message(message, self.debug);
|
||||
}
|
||||
|
||||
@@ -2327,6 +2356,67 @@ mod tests {
|
||||
use std::time::Duration;
|
||||
use test_case::test_case;
|
||||
|
||||
#[test]
|
||||
fn planner_classification_excludes_user_only_content() {
|
||||
use rmcp::model::{AnnotateAble, RawTextContent, Role};
|
||||
|
||||
let user_only = RawTextContent {
|
||||
text: "user-only plan".to_string(),
|
||||
meta: None,
|
||||
}
|
||||
.no_annotation()
|
||||
.with_audience(vec![Role::User]);
|
||||
let assistant_only = RawTextContent {
|
||||
text: "agent classification text".to_string(),
|
||||
meta: None,
|
||||
}
|
||||
.no_annotation()
|
||||
.with_audience(vec![Role::Assistant]);
|
||||
let mixed = Message::assistant()
|
||||
.with_content(MessageContent::Text(user_only.clone()))
|
||||
.with_content(MessageContent::Text(assistant_only));
|
||||
|
||||
assert_eq!(
|
||||
planner_classification_text(&mixed).unwrap(),
|
||||
"agent classification text"
|
||||
);
|
||||
assert!(planner_classification_text(
|
||||
&Message::assistant().with_content(MessageContent::Text(user_only))
|
||||
)
|
||||
.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn planner_history_is_fixed_after_audience_projection() {
|
||||
use rmcp::model::{AnnotateAble, RawTextContent, Role};
|
||||
|
||||
let hidden_separator = MessageContent::Text(
|
||||
RawTextContent {
|
||||
text: "hidden separator".to_string(),
|
||||
meta: None,
|
||||
}
|
||||
.no_annotation()
|
||||
.with_audience(vec![Role::User]),
|
||||
);
|
||||
let history = Conversation::new_unvalidated([
|
||||
Message::user().with_text("first request"),
|
||||
Message::assistant().with_content(hidden_separator),
|
||||
Message::user().with_text("second request"),
|
||||
]);
|
||||
|
||||
let provider_messages = planner_provider_messages(&history).agent_visible_messages();
|
||||
|
||||
assert_eq!(provider_messages.len(), 1);
|
||||
assert_eq!(provider_messages[0].role, Role::User);
|
||||
assert_eq!(
|
||||
provider_messages[0].as_concat_text(),
|
||||
"first request\nsecond request"
|
||||
);
|
||||
assert!(!provider_messages[0]
|
||||
.as_concat_text()
|
||||
.contains("hidden separator"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_format_elapsed_time_under_60_seconds() {
|
||||
// Test sub-second duration
|
||||
|
||||
@@ -237,6 +237,10 @@ pub fn set_thinking_message(s: &String) {
|
||||
}
|
||||
|
||||
pub fn render_message(message: &Message, debug: bool) {
|
||||
if !message.is_user_visible() {
|
||||
return;
|
||||
}
|
||||
let message = message.user_visible_content();
|
||||
let theme = get_theme();
|
||||
|
||||
for content in &message.content {
|
||||
@@ -296,6 +300,10 @@ pub fn render_message_streaming(
|
||||
thinking_header_shown: &mut bool,
|
||||
debug: bool,
|
||||
) {
|
||||
if !message.is_user_visible() {
|
||||
return;
|
||||
}
|
||||
let message = message.user_visible_content();
|
||||
let theme = get_theme();
|
||||
|
||||
for content in &message.content {
|
||||
|
||||
Reference in New Issue
Block a user