diff --git a/rust/crates/sift_mcp/CLAUDE.md b/rust/crates/sift_mcp/CLAUDE.md index 6644dee492..f4e738e96a 100644 --- a/rust/crates/sift_mcp/CLAUDE.md +++ b/rust/crates/sift_mcp/CLAUDE.md @@ -277,6 +277,13 @@ pub async fn list_webhooks(&self, params: Parameters) -> error::McpR set both the structured `next_step` field and `result.content = vec![Content::text(next_step)]` so the calling agent sees it. `get_data` and `explore_url` are the reference patterns. Write tools use `next_step` to force a confirmation before the agent proceeds. +- **Read tools keep their payload in `content`.** Clients such as Claude Code show the model + `content`, not `structuredContent`. Replacing `content` with `next_step` alone hides the payload + from the agent, so a read tool that returns data the agent must see (versions, summaries, dry-run + results) returns `read_result(body, next_step)` from `tool/common/`, which puts `next_step` + first and keeps the JSON body after it. Replace `content` with `next_step` only when + `next_step` itself carries everything the agent needs, such as the file path from `get_data` or + the URL from `explore_url`. `preview_rule` is the reference. ### Writing the description diff --git a/rust/crates/sift_mcp/src/tool/calculated_channels/mod.rs b/rust/crates/sift_mcp/src/tool/calculated_channels/mod.rs index 8b01db110c..be40e87ec9 100644 --- a/rust/crates/sift_mcp/src/tool/calculated_channels/mod.rs +++ b/rust/crates/sift_mcp/src/tool/calculated_channels/mod.rs @@ -16,7 +16,7 @@ use crate::{ service::calculated_channels::{ CalculatedChannelUpdate, CalculatedChannelWrite, NewCalculatedChannel, }, - tool::common::{ListParams, MetadataEntry, list_body, to_values}, + tool::common::{ListParams, MetadataEntry, list_body, read_result, to_values}, }; #[cfg(test)] @@ -252,9 +252,7 @@ impl SiftMcpServer { ); body["next_step"] = serde_json::Value::String(next_step.clone()); - let mut result = CallToolResult::structured(body); - result.content = vec![ContentBlock::text(next_step)]; - Ok(result) + Ok(read_result(body, next_step)) } #[tool( diff --git a/rust/crates/sift_mcp/src/tool/calculated_channels/test.rs b/rust/crates/sift_mcp/src/tool/calculated_channels/test.rs index ec893578a8..50254b05d0 100644 --- a/rust/crates/sift_mcp/src/tool/calculated_channels/test.rs +++ b/rust/crates/sift_mcp/src/tool/calculated_channels/test.rs @@ -17,7 +17,7 @@ use super::{ }; use crate::{ server::SiftMcpServer, - tool::common::test_support::{list_params, list_params_with_fields, structured}, + tool::common::test_support::{content_texts, list_params, list_params_with_fields, structured}, }; const REFERENCES_JSON: &str = r#"[ @@ -185,6 +185,10 @@ async fn list_calculated_channel_versions_returns_versions() { .await .expect("list_calculated_channel_versions failed"); + let texts = content_texts(&resp); + assert_eq!(texts.len(), 2); + assert!(texts[1].contains("calculated_channel_versions")); + let body = structured(resp); let versions = body["calculated_channel_versions"] .as_array() diff --git a/rust/crates/sift_mcp/src/tool/common/mod.rs b/rust/crates/sift_mcp/src/tool/common/mod.rs index c59964d1bf..cbdd11bfcf 100644 --- a/rust/crates/sift_mcp/src/tool/common/mod.rs +++ b/rust/crates/sift_mcp/src/tool/common/mod.rs @@ -1,4 +1,5 @@ use rmcp::ErrorData; +use rmcp::model::{CallToolResult, ContentBlock}; use rmcp::schemars::{self, JsonSchema}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -213,6 +214,18 @@ pub(crate) fn list_body( Value::Object(body) } +/// Shape a read tool's result so the calling agent sees both `next_step` and +/// the payload. `CallToolResult::structured` already mirrors `body` into a text +/// content block; replacing `content` with `next_step` alone, as write tools +/// do, leaves the payload only in `structuredContent`, which clients such as +/// Claude Code do not show the model. `preview_rule` hit exactly this: agents +/// saw an annotation count but none of the annotations. +pub(crate) fn read_result(body: Value, next_step: String) -> CallToolResult { + let mut result = CallToolResult::structured(body); + result.content.insert(0, ContentBlock::text(next_step)); + result +} + impl From for MetadataValue { fn from(entry: MetadataEntry) -> Self { let (key_type, value) = match entry.value { @@ -275,6 +288,22 @@ pub(crate) mod test_support { .expect("expected structured content") } + /// Collect the text of every content block, which is what clients such as + /// Claude Code show the model. + pub(crate) fn content_texts(result: &CallToolResult) -> Vec { + result + .content + .iter() + .map(|block| { + block + .as_text() + .expect("expected a text content block") + .text + .clone() + }) + .collect() + } + /// Extract a single field from a tool result's structured JSON body. pub(crate) fn structured_field(result: CallToolResult, key: &str) -> Value { let mut value = structured(result); diff --git a/rust/crates/sift_mcp/src/tool/common/test.rs b/rust/crates/sift_mcp/src/tool/common/test.rs index f3bdf00200..c40ad6abf9 100644 --- a/rust/crates/sift_mcp/src/tool/common/test.rs +++ b/rust/crates/sift_mcp/src/tool/common/test.rs @@ -1,4 +1,6 @@ -use super::{ListParams, list_body, project_fields, url_clause}; +use super::{ + ListParams, list_body, project_fields, read_result, test_support::content_texts, url_clause, +}; use serde_json::{Value, json}; /// `filter` is deliberately required on list tools: the caller must choose a @@ -259,3 +261,20 @@ fn list_body_omits_unmatched_fields_when_the_page_is_empty() { "an empty page must not look like a spelling problem: {body}" ); } + +/// A read tool's payload must reach `content`, not only `structuredContent`: +/// Claude Code shows the model `content`, so a payload left out of it is +/// invisible to the agent. +#[test] +fn read_result_keeps_payload_in_content_after_next_step() { + let body = json!({ "items": [{ "id": "a" }], "next_step": "Listed 1 item." }); + + let result = read_result(body.clone(), "Listed 1 item.".to_string()); + + let texts = content_texts(&result); + assert_eq!(texts.len(), 2); + assert_eq!(texts[0], "Listed 1 item."); + let payload: Value = serde_json::from_str(&texts[1]).expect("payload should be JSON"); + assert_eq!(payload, body); + assert_eq!(result.structured_content, Some(body)); +} diff --git a/rust/crates/sift_mcp/src/tool/reports/mod.rs b/rust/crates/sift_mcp/src/tool/reports/mod.rs index 4b73e3bc3e..c3f79f06b8 100644 --- a/rust/crates/sift_mcp/src/tool/reports/mod.rs +++ b/rust/crates/sift_mcp/src/tool/reports/mod.rs @@ -12,7 +12,7 @@ use crate::{ error::{self, from_anyhow}, server::SiftMcpServer, service::reports::{ReportSource, RuleIdentifier}, - tool::common::{MetadataEntry, url_clause, with_urls}, + tool::common::{MetadataEntry, read_result, url_clause, with_urls}, }; #[cfg(test)] @@ -185,13 +185,14 @@ impl SiftMcpServer { url_clause("report", None, report_url.as_deref()), ); - let mut result = CallToolResult::structured(serde_json::json!({ - "report_rule_summaries": summaries, - "report_url": report_url, - "next_step": next_step, - })); - result.content = vec![ContentBlock::text(next_step)]; - Ok(result) + Ok(read_result( + serde_json::json!({ + "report_rule_summaries": summaries, + "report_url": report_url, + "next_step": next_step, + }), + next_step, + )) } #[tool( diff --git a/rust/crates/sift_mcp/src/tool/reports/test.rs b/rust/crates/sift_mcp/src/tool/reports/test.rs index 5b48c77476..b67399b9cf 100644 --- a/rust/crates/sift_mcp/src/tool/reports/test.rs +++ b/rust/crates/sift_mcp/src/tool/reports/test.rs @@ -20,7 +20,10 @@ use tonic::{Response, Status, transport::Server}; use super::{ CreateReportParams, ReportListParams, ReportRuleSummaryListParams, UpdateReportParams, }; -use crate::{server::SiftMcpServer, tool::common::test_support::structured_field}; +use crate::{ + server::SiftMcpServer, + tool::common::test_support::{content_texts, structured_field}, +}; fn create_report_params() -> CreateReportParams { CreateReportParams { @@ -320,6 +323,10 @@ async fn list_report_rule_summaries_returns_single_page() { .await .expect("list_report_rule_summaries failed"); + let texts = content_texts(&resp); + assert_eq!(texts.len(), 2); + assert!(texts[1].contains("rule-1")); + let summaries = structured_field(resp, "report_rule_summaries"); assert_eq!(summaries.as_array().unwrap().len(), 1); assert_eq!(summaries[0]["ruleId"], "rule-1"); diff --git a/rust/crates/sift_mcp/src/tool/rule_evaluation/mod.rs b/rust/crates/sift_mcp/src/tool/rule_evaluation/mod.rs index 6eef4bc4b5..cc285a62fe 100644 --- a/rust/crates/sift_mcp/src/tool/rule_evaluation/mod.rs +++ b/rust/crates/sift_mcp/src/tool/rule_evaluation/mod.rs @@ -1,7 +1,6 @@ use rmcp::{ ErrorData, handler::server::wrapper::Parameters, - model::{CallToolResult, ContentBlock}, schemars::{self, JsonSchema}, tool, tool_router, }; @@ -12,6 +11,7 @@ use crate::{ error::{self, from_anyhow}, server::SiftMcpServer, service::{common::cel_escape, rule_evaluation::PreviewRuleSource}, + tool::common::read_result, }; #[cfg(test)] @@ -126,13 +126,14 @@ impl SiftMcpServer { result.created_annotation_count, ); - let mut result = CallToolResult::structured(serde_json::json!({ - "created_annotation_count": result.created_annotation_count, - "dry_run_annotations": result.dry_run_annotations, - "next_step": next_step, - })); - result.content = vec![ContentBlock::text(next_step)]; - Ok(result) + Ok(read_result( + serde_json::json!({ + "created_annotation_count": result.created_annotation_count, + "dry_run_annotations": result.dry_run_annotations, + "next_step": next_step, + }), + next_step, + )) } } diff --git a/rust/crates/sift_mcp/src/tool/rule_evaluation/test.rs b/rust/crates/sift_mcp/src/tool/rule_evaluation/test.rs index ac1034f9c7..3edfb44608 100644 --- a/rust/crates/sift_mcp/src/tool/rule_evaluation/test.rs +++ b/rust/crates/sift_mcp/src/tool/rule_evaluation/test.rs @@ -15,7 +15,10 @@ use tokio::task::JoinHandle; use tonic::{Response, Status, transport::Server}; use super::PreviewRuleParams; -use crate::{server::SiftMcpServer, tool::common::test_support::structured_field}; +use crate::{ + server::SiftMcpServer, + tool::common::test_support::{content_texts, structured_field}, +}; fn preview_rule_params() -> PreviewRuleParams { PreviewRuleParams { @@ -102,6 +105,38 @@ async fn preview_rule_saved_by_id_happy_path() { assert_eq!(annotations.as_array().unwrap().len(), 1); } +#[tokio::test] +async fn preview_rule_shows_dry_run_annotations_in_content() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules_preview().returning(|_| { + Ok(Response::new(EvaluateRulesPreviewResponse { + created_annotation_count: 1, + dry_run_annotations: vec![DryRunAnnotation { + condition_id: "cond-1".into(), + name: "overtemp".into(), + ..Default::default() + }], + })) + }); + + let (server, _h) = server_with_eval_mock(eval_mock).await; + + let mut params = preview_rule_params(); + params.rule_id = Some("rule-1".into()); + + let resp = server + .preview_rule(Parameters(params)) + .await + .expect("preview_rule failed"); + + let texts = content_texts(&resp); + assert_eq!(texts.len(), 2); + assert!(texts[0].starts_with("Preview evaluated 1 annotation(s)")); + let payload: serde_json::Value = + serde_json::from_str(&texts[1]).expect("payload should be JSON"); + assert_eq!(payload["dry_run_annotations"][0]["name"], "overtemp"); +} + #[tokio::test] async fn preview_rule_saved_by_name_resolves_rule_id() { let mut rule_mock = MockRuleServiceImpl::new(); diff --git a/rust/crates/sift_mcp/src/tool/rules/mod.rs b/rust/crates/sift_mcp/src/tool/rules/mod.rs index a82f632490..2d9902e324 100644 --- a/rust/crates/sift_mcp/src/tool/rules/mod.rs +++ b/rust/crates/sift_mcp/src/tool/rules/mod.rs @@ -12,7 +12,7 @@ use crate::{ error::{self, from_anyhow}, server::SiftMcpServer, service::rules::RuleUpdate, - tool::common::{ListParams, list_body, url_clause, with_urls}, + tool::common::{ListParams, list_body, read_result, url_clause, with_urls}, }; #[cfg(test)] @@ -196,13 +196,14 @@ impl SiftMcpServer { url_clause("rule", None, rule_url.as_deref()), ); - let mut result = CallToolResult::structured(serde_json::json!({ - "rule_versions": rule_versions, - "rule_url": rule_url, - "next_step": next_step, - })); - result.content = vec![ContentBlock::text(next_step)]; - Ok(result) + Ok(read_result( + serde_json::json!({ + "rule_versions": rule_versions, + "rule_url": rule_url, + "next_step": next_step, + }), + next_step, + )) } #[tool(