From 25ac827fe33fe24f49f02757ed42d36add3c9e09 Mon Sep 17 00:00:00 2001 From: tomi Date: Sat, 31 Oct 2026 17:59:58 +0100 Subject: [PATCH 1/3] fix(mcp): retain catalog text-only results in chat response envelope (cherry picked from commit b4b8be38e60a45909226095d4f82ec8b5acad1e6) --- src/markdown_chat/output.rs | 103 ++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/src/markdown_chat/output.rs b/src/markdown_chat/output.rs index 343de403..38a46e1b 100644 --- a/src/markdown_chat/output.rs +++ b/src/markdown_chat/output.rs @@ -113,6 +113,20 @@ pub(crate) fn attach_fields( fields: Vec<(&str, Option)>, ) { 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 = if original_schema.is_none() + && original + .as_ref() + .and_then(Value::as_object) + .is_none_or(|object| !object.contains_key("content")) + { + let text = result.joined_text(); + (!text.is_empty()).then_some(text) + } else { + None + }; let wrap = fields .iter() .any(|(field, _)| needs_envelope(original_schema, field)) @@ -130,6 +144,9 @@ pub(crate) fn attach_fields( .and_then(|value| value.as_object().cloned()) .unwrap_or_default() }; + if let Some(text) = text_fallback { + object.insert("content".into(), Value::String(text)); + } for (field, message) in fields { if let Some(message) = message.filter(|message| !message.is_empty()) { object.insert(field.into(), Value::String(message.clone())); @@ -159,6 +176,92 @@ 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)], + ); + 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()))], + ); + 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)]); + 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)]); + 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)]); + 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)]); + 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(); From 5cbaf9370fcc1627ce61c9654cc425732f03d898 Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 10 Nov 2026 21:34:48 +0100 Subject: [PATCH 2/3] Bound generated text mirrors before attaching notices --- CHANGELOG.md | 5 ++ src/markdown_chat/output.rs | 66 ++++++++++++++++----- src/output_budget.rs | 36 +++++++++++- src/server.rs | 112 ++++++++++++++++++++++++++++++++++++ 4 files changed, 203 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c21694..553c3b05 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 38a46e1b..99de5abc 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,29 +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 = if original_schema.is_none() + let text_fallback = original_schema.is_none() && original .as_ref() .and_then(Value::as_object) .is_none_or(|object| !object.contains_key("content")) - { - let text = result.joined_text(); - (!text.is_empty()).then_some(text) - } else { - None - }; + && !result.joined_text().is_empty(); let wrap = fields .iter() .any(|(field, _)| needs_envelope(original_schema, field)) @@ -144,8 +148,16 @@ pub(crate) fn attach_fields( .and_then(|value| value.as_object().cloned()) .unwrap_or_default() }; - if let Some(text) = text_fallback { - object.insert("content".into(), Value::String(text)); + 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()) { @@ -156,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)] @@ -183,6 +196,7 @@ mod tests { &mut result, None, vec![(USER_MESSAGE_FIELD, None), ("workspace_changed", None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, ); assert_eq!( result.structured_content, @@ -202,6 +216,7 @@ mod tests { &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"], @@ -217,7 +232,12 @@ mod tests { 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)]); + 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"})) @@ -229,7 +249,12 @@ mod tests { 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)]); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); assert_eq!( result.structured_content, Some(json!({ @@ -243,7 +268,12 @@ mod tests { 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)]); + 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"})) @@ -254,7 +284,12 @@ mod tests { #[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)]); + attach_fields( + &mut result, + None, + vec![(USER_MESSAGE_FIELD, None)], + DEFAULT_MAX_TOOL_OUTPUT_TOKENS, + ); assert!(result.is_error); assert_eq!( result.structured_content, @@ -314,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 56fefdc3..3a7fe96f 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 89b6f3d0..a45dbbaf 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(); From 853b8e6ff22600fbfceae8e60f0b4e5f648b0e7e Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 10 Nov 2026 21:41:54 +0100 Subject: [PATCH 3/3] Format ticket isolation tests --- src/server_agent_ticket_tests.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/server_agent_ticket_tests.rs b/src/server_agent_ticket_tests.rs index 67d8b51b..4e2c2325 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" + ); } }