diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c2169..553c3b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- Preserve text-only MCP results in response envelopes while bounding the + generated structured text and retaining upstream metadata and full user notices. + ## [2.3.29] - 2026-11-09 ### Added diff --git a/src/markdown_chat/output.rs b/src/markdown_chat/output.rs index 343de40..99de5ab 100644 --- a/src/markdown_chat/output.rs +++ b/src/markdown_chat/output.rs @@ -2,6 +2,9 @@ use serde_json::{Map, Value, json}; #[cfg(test)] use super::USER_MESSAGE_FIELD; +#[cfg(test)] +use crate::output_budget::DEFAULT_MAX_TOOL_OUTPUT_TOKENS; +use crate::output_budget::fill_text_mirror_with_metadata_budget; use crate::types::{ToolContent, ToolResult}; pub(crate) fn timestamped_user_message(markdown: &str, at_ms: Option) -> String { @@ -104,15 +107,30 @@ pub(crate) fn attach_field( field: &str, message: Option, ) { - attach_fields(result, original_schema, vec![(field, message)]); + attach_fields( + result, + original_schema, + vec![(field, message)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); } pub(crate) fn attach_fields( result: &mut ToolResult, original_schema: Option<&Value>, fields: Vec<(&str, Option)>, + max_output_tokens: u64, ) { let original = result.structured_content.take(); + // Unstructured MCP text tools (catalog/gateway) have no output schema. + // Do not turn their existing model-visible text into an empty JSON object + // when optional Markdown-chat notice fields are absent. + let text_fallback = original_schema.is_none() + && original + .as_ref() + .and_then(Value::as_object) + .is_none_or(|object| !object.contains_key("content")) + && !result.joined_text().is_empty(); let wrap = fields .iter() .any(|(field, _)| needs_envelope(original_schema, field)) @@ -130,6 +148,17 @@ pub(crate) fn attach_fields( .and_then(|value| value.as_object().cloned()) .unwrap_or_default() }; + let mut missing_mirror = false; + if text_fallback { + fill_text_mirror_with_metadata_budget(result, object, max_output_tokens); + match result.structured_content.take() { + Some(Value::Object(mirror)) => object = mirror, + _ => { + missing_mirror = true; + object = Map::new(); + } + } + } for (field, message) in fields { if let Some(message) = message.filter(|message| !message.is_empty()) { object.insert(field.into(), Value::String(message.clone())); @@ -139,7 +168,8 @@ pub(crate) fn attach_fields( .push(ToolContent::Text(json!({field:message}).to_string())); } } - result.structured_content = Some(Value::Object(object)); + result.structured_content = + (!missing_mirror || !object.is_empty()).then_some(Value::Object(object)); } #[cfg(test)] @@ -159,6 +189,114 @@ mod tests { ); } + #[test] + fn unstructured_tool_text_survives_empty_notice_envelope() { + let mut result = ToolResult::text("workspace inspection: OK"); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None), ("workspace_changed", None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert_eq!( + result.structured_content, + Some(json!({"content": "workspace inspection: OK"})) + ); + assert_eq!(result.joined_text(), "workspace inspection: OK"); + assert!(jsonschema::is_valid( + &schema(None), + result.structured_content.as_ref().unwrap() + )); + } + + #[test] + fn unstructured_tool_text_and_user_notice_are_both_visible() { + let mut result = ToolResult::text("root=C:/approved"); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, Some("user says hello".into()))], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert_eq!( + result.structured_content.as_ref().unwrap()["content"], + "root=C:/approved" + ); + assert_eq!( + result.structured_content.as_ref().unwrap()[USER_MESSAGE_FIELD], + "user says hello" + ); + } + + #[test] + fn upstream_structured_content_is_unchanged_by_notice_envelope() { + let mut result = + ToolResult::text("upstream text").with_structured(json!({"owner": "upstream"})); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert_eq!( + result.structured_content, + Some(json!({"owner": "upstream", "content": "upstream text"})) + ); + assert_eq!(result.joined_text(), "upstream text"); + } + + #[test] + fn upstream_metadata_does_not_hide_text_in_chat_result() { + let mut result = ToolResult::text("nodeRepl.write: inspection worked") + .with_structured(json!({"execution_duration_ms": 0})); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert_eq!( + result.structured_content, + Some(json!({ + "execution_duration_ms": 0, + "content": "nodeRepl.write: inspection worked" + })) + ); + } + + #[test] + fn upstream_content_field_is_not_overwritten() { + let mut result = ToolResult::text("extra text") + .with_structured(json!({"content": "original upstream content"})); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert_eq!( + result.structured_content, + Some(json!({"content": "original upstream content"})) + ); + assert_eq!(result.joined_text(), "extra text"); + } + + #[test] + fn unstructured_error_is_not_hidden_as_empty_json() { + let mut result = ToolResult::error("permission denied"); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); + assert!(result.is_error); + assert_eq!( + result.structured_content, + Some(json!({"content": "permission denied"})) + ); + } + #[test] fn native_object_keeps_its_shape_and_optional_chat_field() { let original = crate::tool::text_output_schema(); @@ -211,6 +349,7 @@ mod tests { (USER_MESSAGE_FIELD, Some("full user text".into())), ("workspace_changed", Some("new workspace".into())), ], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, ); let value = result.structured_content.unwrap(); assert_eq!(value[USER_MESSAGE_FIELD], "full user text"); diff --git a/src/output_budget.rs b/src/output_budget.rs index 56fefdc..3a7fe96 100644 --- a/src/output_budget.rs +++ b/src/output_budget.rs @@ -6,7 +6,7 @@ use std::io::{self, Write}; -use serde_json::Value; +use serde_json::{Map, Value}; use crate::types::{AppConfig, ToolContent, ToolResult}; @@ -185,6 +185,40 @@ pub fn fill_text_mirror_with_budget(result: &mut ToolResult, max_output_tokens: candidate_budget = scaled.min(candidate_budget - 1); } + text_mirror_budget_error(result, max_output_tokens, original_text_tokens) +} + +/// Reserve the original metadata's serialized size before adding a text mirror. +/// Notice fields are attached separately and are not charged to this budget. +pub(crate) fn fill_text_mirror_with_metadata_budget( + result: &mut ToolResult, + mut metadata: Map, + max_output_tokens: u64, +) -> bool { + if metadata.is_empty() { + return fill_text_mirror_with_budget(result, max_output_tokens); + } + let budget = max_output_tokens.max(1); + let original_text_tokens = text_content_token_count(&result.content); + let reserved = serialized_token_count(&Value::Object(metadata.clone())); + let remaining = budget.saturating_sub(reserved); + if remaining == 0 { + return text_mirror_budget_error(result, budget, original_text_tokens); + } + let truncated = fill_text_mirror_with_budget(result, remaining); + let Some(Value::Object(mirror)) = result.structured_content.take() else { + return text_mirror_budget_error(result, budget, original_text_tokens); + }; + metadata.extend(mirror); + result.structured_content = Some(Value::Object(metadata)); + truncated +} + +fn text_mirror_budget_error( + result: &mut ToolResult, + max_output_tokens: u64, + original_text_tokens: u64, +) -> bool { result.is_error = true; result.structured_content = None; result.content = vec![ToolContent::Text( diff --git a/src/server.rs b/src/server.rs index 89b6f3d..a45dbba 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1898,6 +1898,7 @@ impl ServerHandler for CodexHandler { &mut result, tool.and_then(|tool| tool.output_schema()).as_ref(), output_fields, + tool_output_token_budget(&self.config), ); } if ticketed && let Some(audit) = &self.audit { @@ -4302,6 +4303,117 @@ mod tests { ); } + #[test] + fn unstructured_chat_envelopes_bound_text_and_metadata_before_notices() { + let root = tempfile::tempdir().unwrap(); + let mut config = crate::config::default_config(root.path().to_path_buf()); + for budget in [20_u64, 10_000] { + config.output.max_tool_output_tokens = Some(budget); + for metadata in [ + None, + Some(json!({"duration": 0})), + Some(json!({WORKSPACE_CHANGE_FIELD: 17})), + Some(json!([1, 2])), + ] { + for text in [ + "x".repeat(budget as usize * 8), + String::from_utf8([34, 92, 10].repeat(budget as usize * 8)).unwrap(), + "é😀".repeat(budget as usize * 4), + ] { + let mut result = ToolResult::text(text); + result.structured_content = metadata.clone(); + result.content.push(ToolContent::Image { + data: "image-data".into(), + mime_type: "image/png".into(), + }); + finalize_model_visible_result(None, &mut result, &config); + crate::markdown_chat::output::attach_fields( + &mut result, + None, + vec![(WORKSPACE_CHANGE_FIELD, None)], + tool_output_token_budget(&config), + ); + assert!(!result.is_error); + let value = result.structured_content.as_ref().unwrap(); + assert!( + (serde_json::to_vec(value).unwrap().len() as u64).div_ceil(4) <= budget + ); + assert!( + crate::output_budget::approx_token_count(&result.joined_text()) <= budget + ); + assert_eq!(value["content"], result.joined_text()); + assert_eq!(result.audit.truncated, Some(true)); + assert!(result.content.iter().any(|block| matches!(block, ToolContent::Image { data, .. } if data == "image-data"))); + if let Some(original) = metadata.as_ref() { + if original.is_array() || original.get(WORKSPACE_CHANGE_FIELD).is_some() { + assert_eq!(&value["upstream_result"], original); + } else { + assert_eq!(value["duration"], original["duration"]); + } + } + } + } + } + } + + #[test] + fn bounded_unstructured_text_keeps_full_user_notices_and_ticket_handoff() { + let root = tempfile::tempdir().unwrap(); + let mut config = crate::config::default_config(root.path().to_path_buf()); + config.output.max_tool_output_tokens = Some(20); + let user = "Complete user instructions\n".repeat(10_000); + let mut result = + ToolResult::text("x".repeat(1_000)).with_structured(json!({"duration": 0})); + finalize_model_visible_result(None, &mut result, &config); + crate::markdown_chat::output::attach_fields( + &mut result, + None, + vec![ + (crate::markdown_chat::USER_MESSAGE_FIELD, Some(user.clone())), + ( + crate::agent_tickets::OUTPUT_FIELD, + Some("next-ticket".into()), + ), + ], + tool_output_token_budget(&config), + ); + assert!(!result.is_error); + let mut value = result.structured_content.clone().unwrap(); + let object = value.as_object_mut().unwrap(); + assert_eq!( + object.remove(crate::markdown_chat::USER_MESSAGE_FIELD), + Some(json!(user)) + ); + assert_eq!( + object.remove(crate::agent_tickets::OUTPUT_FIELD), + Some(json!("next-ticket")) + ); + assert_eq!(object["duration"], 0); + assert!((serde_json::to_vec(&value).unwrap().len() as u64).div_ceil(4) <= 20); + assert!(result.joined_text().contains("Complete user instructions")); + } + + #[test] + fn tiny_unstructured_mirror_budget_returns_a_bounded_error_without_empty_json() { + let root = tempfile::tempdir().unwrap(); + let mut config = crate::config::default_config(root.path().to_path_buf()); + for budget in [1, 2, 3] { + config.output.max_tool_output_tokens = Some(budget); + let mut result = ToolResult::text("original text"); + finalize_model_visible_result(None, &mut result, &config); + crate::markdown_chat::output::attach_fields( + &mut result, + None, + vec![(WORKSPACE_CHANGE_FIELD, None)], + tool_output_token_budget(&config), + ); + assert!(result.is_error); + assert!(result.structured_content.is_none()); + assert!(!result.joined_text().is_empty()); + assert!(crate::output_budget::approx_token_count(&result.joined_text()) <= budget); + } + } + #[test] fn finalizer_bounds_text_and_generated_structured_mirror() { let root = tempfile::tempdir().unwrap(); diff --git a/src/server_agent_ticket_tests.rs b/src/server_agent_ticket_tests.rs index 67d8b51..4e2c232 100644 --- a/src/server_agent_ticket_tests.rs +++ b/src/server_agent_ticket_tests.rs @@ -24,11 +24,8 @@ async fn ticket_test_configuration_keeps_chat_state_inside_the_fixture() { for enabled in [false, true] { let root = tempfile::tempdir().unwrap(); let workspace = root.path().canonicalize().unwrap(); - let mut handler = handler_with_tools( - &workspace, - Vec::new(), - crate::types::ToolLogLevel::Info, - ); + let mut handler = + handler_with_tools(&workspace, Vec::new(), crate::types::ToolLogLevel::Info); handler.config = Arc::new(ticket_test_config(&workspace, enabled)); // Fail before creating state if replacing the fixture config restores // the default storage in the user's real home directory. @@ -47,11 +44,18 @@ async fn ticket_test_configuration_keeps_chat_state_inside_the_fixture() { assert!(path.starts_with(&workspace)); chat.ensure().await.unwrap(); chat.warn_ticket_rejection().await.unwrap(); - assert!(std::fs::read_to_string(&path).unwrap().contains(crate::agent_tickets::WARNING)); + assert!( + std::fs::read_to_string(&path) + .unwrap() + .contains(crate::agent_tickets::WARNING) + ); drop(chat); drop(handler); root.close().unwrap(); - assert!(!path.exists(), "dropping the fixture must remove its chat state"); + assert!( + !path.exists(), + "dropping the fixture must remove its chat state" + ); } }