diff --git a/rust/crates/sift_cli/assets/skills/sift/SKILL.md b/rust/crates/sift_cli/assets/skills/sift/SKILL.md index b3107b17b..32d21a69e 100644 --- a/rust/crates/sift_cli/assets/skills/sift/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/sift/SKILL.md @@ -47,7 +47,7 @@ errors. Read the tool schema instead of guessing. This map only tells you what exists. - **Discovery:** `list_assets`, `list_runs`, `list_channels`, `list_reports`, - `list_rules`, `list_rule_versions`, `list_annotations`. + `list_report_templates`, `list_rules`, `list_rule_versions`, `list_annotations`. - **People:** `list_users`. - **Report detail:** `list_report_rule_summaries`. - **Test results:** `list_test_reports`, `list_test_steps`, @@ -58,8 +58,8 @@ exists. - **Docs:** `search_docs`. - **Writes:** `create_rule`, `update_rule`, `archive_rule`, `unarchive_rule`, `create_annotation`, `update_annotation`, `create_report`, `update_report`, - `create_test_report`, `append_test_measurements`, `update_asset`, - `update_run`. + `create_report_template`, `update_report_template`, `create_test_report`, + `append_test_measurements`, `update_asset`, `update_run`. ## Workflows that span tools @@ -76,6 +76,14 @@ exists. chart and numbers, do both and give the user both. - **Answer a question about how Sift works.** Call `search_docs`. Do not answer from memory, and cite the page you used. +- **Evaluate rules against a run.** Find rules with `list_rules` and author rules + with `create_rule`. To reuse the same rule set across many runs, bundle + standard rules (`is_external: false`) into a template with `create_report_template`, + then call `create_report` with `report_template_id`. For a one-off — or for + ad-hoc rules (`is_external: true`, which the API also calls "external" but + cannot be attached to a template) — skip the template and pass `rule_ids`, + `rule_client_keys`, or `rule_version_ids` directly to `create_report`. Track + progress via `list_report_rule_summaries`. ## Rules that always apply diff --git a/rust/crates/sift_mcp/src/server/mod.rs b/rust/crates/sift_mcp/src/server/mod.rs index 9e252b42c..3c9569e2a 100644 --- a/rust/crates/sift_mcp/src/server/mod.rs +++ b/rust/crates/sift_mcp/src/server/mod.rs @@ -13,8 +13,8 @@ use crate::policy::RetryPolicy; use crate::service::{ annotations::AnnotationService, assets::AssetService, channels::ChannelService, data::DataService, docs::DocsService, ingest::IngestService, ping::PingService, - reports::ReportService, rules::RuleService, runs::RunService, test_reports::TestReportService, - url::UrlService, users::UserService, + report_templates::ReportTemplateService, reports::ReportService, rules::RuleService, + runs::RunService, test_reports::TestReportService, url::UrlService, users::UserService, }; #[derive(Clone)] @@ -31,6 +31,7 @@ pub struct SiftMcpServer { pub ping_service: PingService, pub run_service: RunService, pub report_service: ReportService, + pub report_template_service: ReportTemplateService, pub rule_service: RuleService, pub test_report_service: TestReportService, pub docs_service: DocsService, @@ -86,6 +87,7 @@ impl SiftMcpServer { tool_router.merge(Self::runs_router()); tool_router.merge(Self::channels_router()); tool_router.merge(Self::reports_router()); + tool_router.merge(Self::report_templates_router()); tool_router.merge(Self::data_router()); tool_router.merge(Self::explore_router()); tool_router.merge(Self::ping_router()); @@ -108,6 +110,8 @@ impl SiftMcpServer { let ping_service = PingService::new(channel.clone(), retry_policy.clone()); let run_service = RunService::new(channel.clone(), retry_policy.clone()); let report_service = ReportService::new(channel.clone(), retry_policy.clone()); + let report_template_service = + ReportTemplateService::new(channel.clone(), retry_policy.clone()); let rule_service = RuleService::new(channel.clone(), retry_policy.clone()); let test_report_service = TestReportService::new(channel.clone(), retry_policy.clone()); let docs_service = DocsService::new(channel.clone(), retry_policy.clone()); @@ -123,6 +127,7 @@ impl SiftMcpServer { ping_service, run_service, report_service, + report_template_service, rule_service, test_report_service, docs_service, diff --git a/rust/crates/sift_mcp/src/service/mod.rs b/rust/crates/sift_mcp/src/service/mod.rs index 152a22fe6..a83451035 100644 --- a/rust/crates/sift_mcp/src/service/mod.rs +++ b/rust/crates/sift_mcp/src/service/mod.rs @@ -5,6 +5,7 @@ pub mod data; pub mod docs; pub mod ingest; pub mod ping; +pub mod report_templates; pub mod reports; pub mod rules; pub mod runs; diff --git a/rust/crates/sift_mcp/src/service/report_templates/mod.rs b/rust/crates/sift_mcp/src/service/report_templates/mod.rs new file mode 100644 index 000000000..ce926fd11 --- /dev/null +++ b/rust/crates/sift_mcp/src/service/report_templates/mod.rs @@ -0,0 +1,254 @@ +use crate::policy::{RetryPolicy, with_retry}; +use crate::service::common; +use anyhow::{Context, Result, anyhow}; +use pbjson_types::FieldMask; +use sift_rs::{ + SiftChannel, + metadata::v1::MetadataValue, + report_templates::v1::{ + CreateReportTemplateRequest, CreateReportTemplateRequestClientKeys, + CreateReportTemplateRequestRuleIds, ListReportTemplatesRequest, + ListReportTemplatesResponse, ReportTemplate, ReportTemplateRule, ReportTemplateTag, + UpdateReportTemplateRequest, create_report_template_request, + report_template_service_client::ReportTemplateServiceClient, + }, +}; + +#[cfg(test)] +mod test; + +#[allow(clippy::enum_variant_names)] +pub enum TemplateRuleIdentifier { + RuleIds(Vec), + RuleClientKeys(Vec), +} + +#[derive(Default)] +pub struct ReportTemplateUpdate { + pub name: Option, + pub description: Option, + pub tag_names: Option>, + pub rules: Option, + pub metadata: Option>, + pub is_archived: Option, +} + +#[derive(Clone)] +pub struct ReportTemplateService { + channel: SiftChannel, + policy: RetryPolicy, +} + +impl ReportTemplateService { + pub fn new(channel: SiftChannel, policy: RetryPolicy) -> Self { + Self { channel, policy } + } + + pub async fn list_report_templates( + &self, + filter: String, + order_by: Option, + limit: Option, + organization_id: Option, + ) -> Result> { + let (page_size, record_limit) = common::paging(limit); + + let mut page_token = String::new(); + let mut results = Vec::new(); + + let order_by = order_by.unwrap_or_default(); + let organization_id = organization_id.unwrap_or_default(); + + loop { + let channel = self.channel.clone(); + let filter = filter.clone(); + let order_by = order_by.clone(); + let organization_id = organization_id.clone(); + let token = page_token.clone(); + + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let filter = filter.clone(); + let order_by = order_by.clone(); + let organization_id = organization_id.clone(); + let token = token.clone(); + async move { + let mut client = ReportTemplateServiceClient::new(channel); + client + .list_report_templates(ListReportTemplatesRequest { + page_size, + page_token: token, + filter, + organization_id, + order_by, + ..Default::default() + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to query report templates")?; + + let ListReportTemplatesResponse { + report_templates, + next_page_token, + } = resp; + if report_templates.is_empty() { + break; + } + results.extend(report_templates); + + if results.len() >= record_limit || next_page_token.is_empty() { + break; + } + page_token = next_page_token; + } + + results.truncate(record_limit); + + Ok(results) + } + + pub async fn create_report_template( + &self, + organization_id: Option, + name: String, + client_key: Option, + description: Option, + tag_names: Vec, + rules: TemplateRuleIdentifier, + metadata: Vec, + ) -> Result { + let rule_identifiers = match rules { + TemplateRuleIdentifier::RuleIds(rule_ids) => { + create_report_template_request::RuleIdentifiers::RuleIds( + CreateReportTemplateRequestRuleIds { rule_ids }, + ) + } + TemplateRuleIdentifier::RuleClientKeys(rule_client_keys) => { + create_report_template_request::RuleIdentifiers::RuleClientKeys( + CreateReportTemplateRequestClientKeys { rule_client_keys }, + ) + } + }; + + let request = CreateReportTemplateRequest { + name, + client_key, + description, + tag_names, + organization_id: organization_id.unwrap_or_default(), + rule_identifiers: Some(rule_identifiers), + metadata, + }; + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let request = request.clone(); + async move { + let mut client = ReportTemplateServiceClient::new(channel); + client + .create_report_template(request) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to create report template")?; + + resp.report_template + .ok_or_else(|| anyhow!("create_report_template response missing report_template")) + } + + pub async fn update_report_template( + &self, + report_template_id: String, + changes: ReportTemplateUpdate, + ) -> Result { + let ReportTemplateUpdate { + name, + description, + tag_names, + rules, + metadata, + is_archived, + } = changes; + + let mut template = ReportTemplate { + report_template_id, + ..Default::default() + }; + let mut paths = Vec::new(); + + if let Some(name) = name { + template.name = name; + paths.push("name".to_string()); + } + if let Some(description) = description { + template.description = Some(description); + paths.push("description".to_string()); + } + if let Some(tag_names) = tag_names { + template.tags = tag_names + .into_iter() + .map(|tag_name| ReportTemplateTag { tag_name }) + .collect(); + paths.push("tags".to_string()); + } + if let Some(rules) = rules { + template.rules = template_rules_from_identifier(rules); + paths.push("rules".to_string()); + } + if let Some(metadata) = metadata { + template.metadata = metadata; + paths.push("metadata".to_string()); + } + if let Some(is_archived) = is_archived { + template.is_archived = is_archived; + paths.push("is_archived".to_string()); + } + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let template = template.clone(); + let paths = paths.clone(); + async move { + let mut client = ReportTemplateServiceClient::new(channel); + client + .update_report_template(UpdateReportTemplateRequest { + report_template: Some(template), + update_mask: Some(FieldMask { paths }), + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to update report template")?; + + resp.report_template + .ok_or_else(|| anyhow!("update_report_template response missing report_template")) + } +} + +fn template_rules_from_identifier(rules: TemplateRuleIdentifier) -> Vec { + match rules { + TemplateRuleIdentifier::RuleIds(rule_ids) => rule_ids + .into_iter() + .map(|rule_id| ReportTemplateRule { + rule_id, + ..Default::default() + }) + .collect(), + TemplateRuleIdentifier::RuleClientKeys(client_keys) => client_keys + .into_iter() + .map(|client_key| ReportTemplateRule { + client_key, + ..Default::default() + }) + .collect(), + } +} diff --git a/rust/crates/sift_mcp/src/service/report_templates/test.rs b/rust/crates/sift_mcp/src/service/report_templates/test.rs new file mode 100644 index 000000000..220a3fcd1 --- /dev/null +++ b/rust/crates/sift_mcp/src/service/report_templates/test.rs @@ -0,0 +1,431 @@ +use sift_rs::{ + metadata::v1::{MetadataKey, MetadataKeyType, MetadataValue, metadata_value::Value}, + report_templates::v1::{ + CreateReportTemplateResponse, ListReportTemplatesResponse, ReportTemplate, + ReportTemplateRule, UpdateReportTemplateResponse, create_report_template_request, + report_template_service_server::ReportTemplateServiceServer, + }, +}; +use sift_test_util::{ + grpc::memory_sift_channel, mock::report_templates::v1::MockReportTemplateServiceImpl, +}; +use tokio::task::JoinHandle; +use tonic::{Response, Status, transport::Server}; + +use super::{ReportTemplateService, ReportTemplateUpdate, TemplateRuleIdentifier}; +use crate::service::common::DEFAULT_LIMIT; + +async fn service_with_mock( + mock: MockReportTemplateServiceImpl, +) -> (ReportTemplateService, JoinHandle<()>) { + let (client, server) = tokio::io::duplex(1024); + let channel = memory_sift_channel(client).await; + + let handle = tokio::spawn(async move { + Server::builder() + .add_service(ReportTemplateServiceServer::new(mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + ReportTemplateService::new(channel, crate::policy::RetryPolicy::default()), + handle, + ) +} + +#[tokio::test] +async fn list_report_templates_returns_single_page() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .withf(|req| req.get_ref().filter == "name == \"safety\"") + .returning(|_| { + Ok(Response::new(ListReportTemplatesResponse { + report_templates: vec![ReportTemplate { + report_template_id: "tmpl-1".into(), + name: "safety".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let templates = service + .list_report_templates("name == \"safety\"".to_string(), None, None, None) + .await + .expect("list_report_templates failed"); + + assert_eq!(templates.len(), 1); + assert_eq!(templates[0].report_template_id, "tmpl-1"); +} + +#[tokio::test] +async fn list_report_templates_forwards_organization_id() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .withf(|req| req.get_ref().organization_id == "org-123") + .returning(|_| { + Ok(Response::new(ListReportTemplatesResponse { + report_templates: vec![ReportTemplate { + report_template_id: "tmpl-1".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let templates = service + .list_report_templates(String::new(), None, None, Some("org-123".to_string())) + .await + .expect("list_report_templates failed"); + + assert_eq!(templates.len(), 1); +} + +#[tokio::test] +async fn list_report_templates_paginates_until_token_empty() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates().returning(|req| { + let req = req.into_inner(); + assert_eq!(req.page_size, DEFAULT_LIMIT); + let (report_templates, next) = match req.page_token.as_str() { + "" => ( + vec![ReportTemplate { + report_template_id: "tmpl-1".into(), + ..Default::default() + }], + "page-2".to_string(), + ), + "page-2" => ( + vec![ReportTemplate { + report_template_id: "tmpl-2".into(), + ..Default::default() + }], + String::new(), + ), + other => return Err(Status::invalid_argument(format!("bad token: {other}"))), + }; + Ok(Response::new(ListReportTemplatesResponse { + report_templates, + next_page_token: next, + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let templates = service + .list_report_templates(String::new(), None, None, None) + .await + .expect("list_report_templates failed"); + + let ids: Vec<&str> = templates + .iter() + .map(|t| t.report_template_id.as_str()) + .collect(); + assert_eq!(ids, vec!["tmpl-1", "tmpl-2"]); +} + +#[tokio::test] +async fn list_report_templates_respects_limit() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .times(1) + .returning(|req| { + assert_eq!(req.get_ref().page_size, 2); + Ok(Response::new(ListReportTemplatesResponse { + report_templates: vec![ + ReportTemplate { + report_template_id: "tmpl-1".into(), + ..Default::default() + }, + ReportTemplate { + report_template_id: "tmpl-2".into(), + ..Default::default() + }, + ], + next_page_token: "page-2".into(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let templates = service + .list_report_templates(String::new(), None, Some(2), None) + .await + .expect("list_report_templates failed"); + + assert_eq!(templates.len(), 2); +} + +#[tokio::test] +async fn list_report_templates_propagates_grpc_error() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .returning(|_| Err(Status::not_found("no such template"))); + + let (service, _h) = service_with_mock(mock).await; + + let err = service + .list_report_templates(String::new(), None, None, None) + .await + .expect_err("expected error"); + + assert!(err.to_string().contains("failed to query report templates")); +} + +#[tokio::test] +async fn create_report_template_forwards_rule_ids() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_create_report_template() + .withf(|req| { + let req = req.get_ref(); + req.name == "safety" + && matches!( + &req.rule_identifiers, + Some(create_report_template_request::RuleIdentifiers::RuleIds(r)) + if r.rule_ids == vec!["rule-1".to_string(), "rule-2".to_string()] + ) + }) + .returning(|_| { + Ok(Response::new(CreateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-new".into(), + name: "safety".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let template = service + .create_report_template( + None, + "safety".to_string(), + None, + None, + Vec::new(), + TemplateRuleIdentifier::RuleIds(vec!["rule-1".into(), "rule-2".into()]), + Vec::new(), + ) + .await + .expect("create_report_template failed"); + + assert_eq!(template.report_template_id, "tmpl-new"); +} + +#[tokio::test] +async fn create_report_template_forwards_client_keys() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_create_report_template() + .withf(|req| { + matches!( + &req.get_ref().rule_identifiers, + Some(create_report_template_request::RuleIdentifiers::RuleClientKeys(r)) + if r.rule_client_keys == vec!["k1".to_string()] + ) + }) + .returning(|_| { + Ok(Response::new(CreateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-new".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + service + .create_report_template( + None, + "safety".to_string(), + None, + None, + Vec::new(), + TemplateRuleIdentifier::RuleClientKeys(vec!["k1".into()]), + Vec::new(), + ) + .await + .expect("create_report_template failed"); +} + +#[tokio::test] +async fn create_report_template_missing_body_errors() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_create_report_template() + .returning(|_| Ok(Response::new(CreateReportTemplateResponse::default()))); + + let (service, _h) = service_with_mock(mock).await; + + let err = service + .create_report_template( + None, + "safety".to_string(), + None, + None, + Vec::new(), + TemplateRuleIdentifier::RuleIds(vec!["r1".into()]), + Vec::new(), + ) + .await + .expect_err("expected missing report_template error"); + + assert!(err.to_string().contains("missing report_template")); +} + +#[tokio::test] +async fn update_report_template_builds_mask_for_provided_fields() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_update_report_template() + .withf(|req| { + let req = req.get_ref(); + let mask_paths = req + .update_mask + .as_ref() + .map(|m| m.paths.clone()) + .unwrap_or_default(); + let template = req.report_template.as_ref().expect("template"); + + template.report_template_id == "tmpl-1" + && template.name == "renamed" + && mask_paths == vec!["name".to_string(), "tags".to_string()] + && template.tags.iter().map(|t| t.tag_name.as_str()).eq(["qa"]) + }) + .returning(|_| { + Ok(Response::new(UpdateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-1".into(), + name: "renamed".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let updated = service + .update_report_template( + "tmpl-1".to_string(), + ReportTemplateUpdate { + name: Some("renamed".to_string()), + tag_names: Some(vec!["qa".to_string()]), + ..Default::default() + }, + ) + .await + .expect("update_report_template failed"); + + assert_eq!(updated.name, "renamed"); +} + +#[tokio::test] +async fn update_report_template_replaces_rules() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_update_report_template() + .withf(|req| { + let req = req.get_ref(); + let paths = req + .update_mask + .as_ref() + .map(|m| m.paths.clone()) + .unwrap_or_default(); + let template = req.report_template.as_ref().expect("template"); + let rule_ids: Vec<&str> = template.rules.iter().map(|r| r.rule_id.as_str()).collect(); + + paths == vec!["rules".to_string()] + && rule_ids == vec!["r1", "r2"] + && template.rules.iter().all(|r| r.client_key.is_empty()) + }) + .returning(|_| { + Ok(Response::new(UpdateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-1".into(), + rules: vec![ + ReportTemplateRule { + rule_id: "r1".into(), + ..Default::default() + }, + ReportTemplateRule { + rule_id: "r2".into(), + ..Default::default() + }, + ], + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + service + .update_report_template( + "tmpl-1".to_string(), + ReportTemplateUpdate { + rules: Some(TemplateRuleIdentifier::RuleIds(vec![ + "r1".to_string(), + "r2".to_string(), + ])), + ..Default::default() + }, + ) + .await + .expect("update_report_template failed"); +} + +#[tokio::test] +async fn update_report_template_metadata_and_archive() { + let metadata_entry = MetadataValue { + key: Some(MetadataKey { + name: "owner".into(), + r#type: MetadataKeyType::String.into(), + ..Default::default() + }), + value: Some(Value::StringValue("qa-team".into())), + ..Default::default() + }; + + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_update_report_template() + .withf(|req| { + let req = req.get_ref(); + let paths = req + .update_mask + .as_ref() + .map(|m| m.paths.clone()) + .unwrap_or_default(); + let template = req.report_template.as_ref().expect("template"); + + paths == vec!["metadata".to_string(), "is_archived".to_string()] + && template.metadata.len() == 1 + && template.is_archived + }) + .returning(|_| { + Ok(Response::new(UpdateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-1".into(), + is_archived: true, + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + service + .update_report_template( + "tmpl-1".to_string(), + ReportTemplateUpdate { + metadata: Some(vec![metadata_entry]), + is_archived: Some(true), + ..Default::default() + }, + ) + .await + .expect("update_report_template failed"); +} diff --git a/rust/crates/sift_mcp/src/service/reports/mod.rs b/rust/crates/sift_mcp/src/service/reports/mod.rs index ff55f7114..934501c2f 100644 --- a/rust/crates/sift_mcp/src/service/reports/mod.rs +++ b/rust/crates/sift_mcp/src/service/reports/mod.rs @@ -4,23 +4,26 @@ use anyhow::{Context, Result, anyhow}; use pbjson_types::FieldMask; use sift_rs::{ SiftChannel, + common::r#type::v1::{ + ClientKeys, Ids, ResourceIdentifier, ResourceIdentifiers, resource_identifier, + resource_identifiers, + }, metadata::v1::MetadataValue, reports::v1::{ - CreateReportFromReportTemplateRequest, CreateReportFromRulesRequest, CreateReportRequest, - CreateReportRequestClientKeys, CreateReportRequestRuleIds, - CreateReportRequestRuleVersionIds, GetReportRequest, ListReportRuleSummariesRequest, - ListReportRuleSummariesResponse, ListReportsRequest, ListReportsResponse, Report, - ReportRuleSummary, UpdateReportRequest, create_report_from_rules_request, - create_report_request, report_service_client::ReportServiceClient, + GetReportRequest, ListReportRuleSummariesRequest, ListReportRuleSummariesResponse, + ListReportsRequest, ListReportsResponse, Report, ReportRuleSummary, UpdateReportRequest, + report_service_client::ReportServiceClient, + }, + rule_evaluation::v1::{ + EvaluateRulesFromCurrentRuleVersions, EvaluateRulesFromReportTemplate, + EvaluateRulesFromRuleVersions, EvaluateRulesRequest, evaluate_rules_request, + rule_evaluation_service_client::RuleEvaluationServiceClient, }, }; #[cfg(test)] mod test; -/// How the set of rules a report is built from is identified. Exactly one variant -/// is constructed from the flat tool params. Variant names mirror the proto -/// `rule_identifiers` oneof fields. #[allow(clippy::enum_variant_names)] pub enum RuleIdentifier { RuleIds(Vec), @@ -28,17 +31,16 @@ pub enum RuleIdentifier { RuleVersionIds(Vec), } -/// The source a report is created from. Flattens the `CreateReportRequest` -/// oneof into a typed choice built in the tool handler. pub enum ReportSource { - Template { - report_template_id: String, - }, - Rules { - description: Option, - tag_names: Vec, - rules: RuleIdentifier, - }, + Template { report_template_id: String }, + Rules { rules: RuleIdentifier }, +} + +#[derive(Debug)] +pub struct CreateReportOutput { + pub report: Report, + pub job_id: Option, + pub created_annotation_count: i32, } #[derive(Clone)] @@ -186,64 +188,65 @@ impl ReportService { organization_id: Option, run_id: String, name: String, - metadata: Option>, + description: Option, + metadata: Vec, source: ReportSource, - ) -> Result { - let request = match source { + ) -> Result { + let mode = match source { ReportSource::Template { report_template_id } => { - create_report_request::Request::ReportFromReportTemplateRequest( - CreateReportFromReportTemplateRequest { report_template_id }, - ) - } - ReportSource::Rules { - description, - tag_names, - rules, - } => { - let rule_identifiers = match rules { - RuleIdentifier::RuleIds(rule_ids) => { - create_report_from_rules_request::RuleIdentifiers::RuleIds( - CreateReportRequestRuleIds { rule_ids }, - ) - } - RuleIdentifier::RuleClientKeys(rule_client_keys) => { - create_report_from_rules_request::RuleIdentifiers::RuleClientKeys( - CreateReportRequestClientKeys { rule_client_keys }, - ) - } - RuleIdentifier::RuleVersionIds(rule_version_ids) => { - create_report_from_rules_request::RuleIdentifiers::RuleVersionIds( - CreateReportRequestRuleVersionIds { rule_version_ids }, - ) - } - }; - create_report_request::Request::ReportFromRulesRequest( - CreateReportFromRulesRequest { - name: name.clone(), - description, - tag_names, - rule_identifiers: Some(rule_identifiers), - }, - ) + evaluate_rules_request::Mode::ReportTemplate(EvaluateRulesFromReportTemplate { + report_template: Some(ResourceIdentifier { + identifier: Some(resource_identifier::Identifier::Id(report_template_id)), + }), + }) } + ReportSource::Rules { rules } => match rules { + RuleIdentifier::RuleIds(rule_ids) => { + evaluate_rules_request::Mode::Rules(EvaluateRulesFromCurrentRuleVersions { + rules: Some(ResourceIdentifiers { + identifiers: Some(resource_identifiers::Identifiers::Ids(Ids { + ids: rule_ids, + })), + }), + }) + } + RuleIdentifier::RuleClientKeys(rule_client_keys) => { + evaluate_rules_request::Mode::Rules(EvaluateRulesFromCurrentRuleVersions { + rules: Some(ResourceIdentifiers { + identifiers: Some(resource_identifiers::Identifiers::ClientKeys( + ClientKeys { + client_keys: rule_client_keys, + }, + )), + }), + }) + } + RuleIdentifier::RuleVersionIds(rule_version_ids) => { + evaluate_rules_request::Mode::RuleVersions(EvaluateRulesFromRuleVersions { + rule_version_ids, + }) + } + }, }; - let create_request = CreateReportRequest { + let evaluate_request = EvaluateRulesRequest { organization_id: organization_id.unwrap_or_default(), - run_id, - name: Some(name), - metadata: metadata.unwrap_or_default(), - request: Some(request), + report_name: Some(name), + time: Some(evaluate_rules_request::Time::Run(ResourceIdentifier { + identifier: Some(resource_identifier::Identifier::Id(run_id)), + })), + mode: Some(mode), + ..Default::default() }; let channel = self.channel.clone(); let resp = with_retry(&self.policy, move || { let channel = channel.clone(); - let create_request = create_request.clone(); + let request = evaluate_request.clone(); async move { - let mut client = ReportServiceClient::new(channel); + let mut client = RuleEvaluationServiceClient::new(channel); client - .create_report(create_request) + .evaluate_rules(request) .await .map(|resp| resp.into_inner()) } @@ -251,29 +254,58 @@ impl ReportService { .await .context("failed to create report")?; - resp.report - .ok_or_else(|| anyhow!("create_report response missing report")) + let report_id = resp + .report_id + .clone() + .ok_or_else(|| anyhow!("create_report response missing report_id"))?; + let job_id = resp.job_id.clone(); + let created_annotation_count = resp.created_annotation_count; + + let metadata_update = (!metadata.is_empty()).then_some(metadata); + if description.is_some() || metadata_update.is_some() { + self.update_report_fields(report_id.clone(), description, metadata_update) + .await?; + } + + let report = self.get_report(report_id).await?; + + Ok(CreateReportOutput { + report, + job_id, + created_annotation_count, + }) } - /// Update an existing report's metadata. Per - /// `protos/sift/reports/v1/reports.proto::UpdateReportRequest` the updatable - /// fields are `archived_date`, `is_archived`, and `metadata`; this service - /// exposes `metadata` only (archive flow is out of scope). `metadata` uses - /// REPLACE semantics. - /// - /// `UpdateReportResponse` is empty, so the updated `Report` is re-fetched via - /// `GetReport` and returned. pub async fn update_report( &self, report_id: String, metadata: Vec, ) -> Result { - let report = Report { - report_id: report_id.clone(), - metadata, + self.update_report_fields(report_id.clone(), None, Some(metadata)) + .await?; + self.get_report(report_id).await + } + + async fn update_report_fields( + &self, + report_id: String, + description: Option, + metadata: Option>, + ) -> Result<()> { + let mut report = Report { + report_id, ..Default::default() }; - let paths = vec!["metadata".to_string()]; + let mut paths = Vec::new(); + + if let Some(description) = description { + report.description = Some(description); + paths.push("description".to_string()); + } + if let Some(metadata) = metadata { + report.metadata = metadata; + paths.push("metadata".to_string()); + } let channel = self.channel.clone(); with_retry(&self.policy, move || { @@ -294,6 +326,10 @@ impl ReportService { .await .context("failed to update report")?; + Ok(()) + } + + async fn get_report(&self, report_id: String) -> Result { let channel = self.channel.clone(); let resp = with_retry(&self.policy, move || { let channel = channel.clone(); @@ -307,7 +343,7 @@ impl ReportService { } }) .await - .context("failed to fetch report after update")?; + .context("failed to fetch report")?; resp.report .ok_or_else(|| anyhow!("get_report response missing report")) diff --git a/rust/crates/sift_mcp/src/service/reports/test.rs b/rust/crates/sift_mcp/src/service/reports/test.rs index ec12a4d5f..874ece287 100644 --- a/rust/crates/sift_mcp/src/service/reports/test.rs +++ b/rust/crates/sift_mcp/src/service/reports/test.rs @@ -1,9 +1,19 @@ -use sift_rs::reports::v1::{ - CreateReportResponse, GetReportResponse, ListReportRuleSummariesResponse, ListReportsResponse, - Report, ReportRuleSummary, UpdateReportResponse, create_report_request, - report_service_server::ReportServiceServer, +use sift_rs::{ + reports::v1::{ + GetReportResponse, ListReportRuleSummariesResponse, ListReportsResponse, Report, + ReportRuleSummary, UpdateReportResponse, report_service_server::ReportServiceServer, + }, + rule_evaluation::v1::{ + EvaluateRulesResponse, evaluate_rules_request, + rule_evaluation_service_server::RuleEvaluationServiceServer, + }, +}; +use sift_test_util::{ + grpc::memory_sift_channel, + mock::{ + reports::v1::MockReportServiceImpl, rule_evaluation::v1::MockRuleEvaluationServiceImpl, + }, }; -use sift_test_util::{grpc::memory_sift_channel, mock::reports::v1::MockReportServiceImpl}; use tokio::task::JoinHandle; use tonic::{Response, Status, transport::Server}; @@ -28,6 +38,28 @@ async fn service_with_mock(mock: MockReportServiceImpl) -> (ReportService, JoinH ) } +async fn service_with_dual_mocks( + report_mock: MockReportServiceImpl, + eval_mock: MockRuleEvaluationServiceImpl, +) -> (ReportService, JoinHandle<()>) { + let (client, server) = tokio::io::duplex(1024); + let channel = memory_sift_channel(client).await; + + let handle = tokio::spawn(async move { + Server::builder() + .add_service(ReportServiceServer::new(report_mock)) + .add_service(RuleEvaluationServiceServer::new(eval_mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + ReportService::new(channel, crate::policy::RetryPolicy::default()), + handle, + ) +} + #[tokio::test] async fn list_reports_returns_single_page() { let mut mock = MockReportServiceImpl::new(); @@ -313,16 +345,10 @@ async fn list_report_rule_summaries_respects_limit() { .returning(|req| { assert_eq!(req.get_ref().page_size, 1); Ok(Response::new(ListReportRuleSummariesResponse { - report_rule_summaries: vec![ - ReportRuleSummary { - rule_id: "rule-1".into(), - ..Default::default() - }, - ReportRuleSummary { - rule_id: "rule-2".into(), - ..Default::default() - }, - ], + report_rule_summaries: vec![ReportRuleSummary { + rule_id: "rule-1".into(), + ..Default::default() + }], next_page_token: "page-2".into(), })) }); @@ -357,74 +383,214 @@ async fn list_report_rule_summaries_propagates_grpc_error() { } #[tokio::test] -async fn create_report_from_rules_maps_oneof() { - let mut mock = MockReportServiceImpl::new(); - mock.expect_create_report() +async fn create_report_from_rule_ids() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock + .expect_evaluate_rules() .withf(|req| { let req = req.get_ref(); - req.run_id == "run-1" - && req.name.as_deref() == Some("nightly report") - && matches!( - req.request, - Some(create_report_request::Request::ReportFromRulesRequest(_)) - ) + let run_ok = matches!( + &req.time, + Some(evaluate_rules_request::Time::Run(rid)) + if matches!(&rid.identifier, + Some(sift_rs::common::r#type::v1::resource_identifier::Identifier::Id(id)) + if id == "run-1") + ); + let mode_ok = matches!( + &req.mode, + Some(evaluate_rules_request::Mode::Rules(inner)) + if matches!( + &inner.rules.as_ref().and_then(|r| r.identifiers.as_ref()), + Some(sift_rs::common::r#type::v1::resource_identifiers::Identifiers::Ids(ids)) + if ids.ids == vec!["rule-1".to_string()] + ) + ); + req.report_name.as_deref() == Some("nightly report") && run_ok && mode_ok }) .returning(|_| { - Ok(Response::new(CreateReportResponse { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: Some("job-1".into()), + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock + .expect_get_report() + .withf(|req| req.get_ref().report_id == "rep-new") + .returning(|_| { + Ok(Response::new(GetReportResponse { report: Some(Report { report_id: "rep-new".into(), + name: "nightly report".into(), ..Default::default() }), })) }); - let (service, _h) = service_with_mock(mock).await; + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; - let report = service + let output = service .create_report( None, "run-1".to_string(), "nightly report".to_string(), None, + vec![], ReportSource::Rules { - description: None, - tag_names: vec![], rules: RuleIdentifier::RuleIds(vec!["rule-1".to_string()]), }, ) .await .expect("create_report failed"); - assert_eq!(report.report_id, "rep-new"); + assert_eq!(output.report.report_id, "rep-new"); + assert_eq!(output.job_id.as_deref(), Some("job-1")); + assert_eq!(output.created_annotation_count, 0); } #[tokio::test] -async fn create_report_from_template_maps_oneof() { - let mut mock = MockReportServiceImpl::new(); - mock.expect_create_report() +async fn create_report_from_rule_client_keys() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock + .expect_evaluate_rules() .withf(|req| { matches!( - req.get_ref().request, - Some(create_report_request::Request::ReportFromReportTemplateRequest(_)) + &req.get_ref().mode, + Some(evaluate_rules_request::Mode::Rules(inner)) + if matches!( + inner.rules.as_ref().and_then(|r| r.identifiers.as_ref()), + Some(sift_rs::common::r#type::v1::resource_identifiers::Identifiers::ClientKeys(k)) + if k.client_keys == vec!["ck-1".to_string()] + ) ) }) .returning(|_| { - Ok(Response::new(CreateReportResponse { - report: Some(Report { - report_id: "rep-tmpl".into(), - ..Default::default() - }), + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: None, + created_annotation_count: 0, })) }); - let (service, _h) = service_with_mock(mock).await; + let mut report_mock = MockReportServiceImpl::new(); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-new".into(), + ..Default::default() + }), + })) + }); - let report = service + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; + + service + .create_report( + None, + "run-1".to_string(), + "x".to_string(), + None, + vec![], + ReportSource::Rules { + rules: RuleIdentifier::RuleClientKeys(vec!["ck-1".to_string()]), + }, + ) + .await + .expect("create_report failed"); +} + +#[tokio::test] +async fn create_report_from_rule_version_ids() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock + .expect_evaluate_rules() + .withf(|req| { + matches!( + &req.get_ref().mode, + Some(evaluate_rules_request::Mode::RuleVersions(v)) + if v.rule_version_ids == vec!["rv-1".to_string()] + ) + }) + .returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: None, + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-new".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; + + service + .create_report( + None, + "run-1".to_string(), + "x".to_string(), + None, + vec![], + ReportSource::Rules { + rules: RuleIdentifier::RuleVersionIds(vec!["rv-1".to_string()]), + }, + ) + .await + .expect("create_report failed"); +} + +#[tokio::test] +async fn create_report_from_template() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock + .expect_evaluate_rules() + .withf(|req| { + matches!( + &req.get_ref().mode, + Some(evaluate_rules_request::Mode::ReportTemplate(t)) + if matches!( + t.report_template.as_ref().and_then(|r| r.identifier.as_ref()), + Some(sift_rs::common::r#type::v1::resource_identifier::Identifier::Id(id)) + if id == "tmpl-1" + ) + ) + }) + .returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-tmpl".into()), + job_id: Some("job-2".into()), + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-tmpl".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; + + let output = service .create_report( None, "run-1".to_string(), "from template".to_string(), None, + vec![], ReportSource::Template { report_template_id: "tmpl-1".to_string(), }, @@ -432,16 +598,146 @@ async fn create_report_from_template_maps_oneof() { .await .expect("create_report failed"); - assert_eq!(report.report_id, "rep-tmpl"); + assert_eq!(output.report.report_id, "rep-tmpl"); + assert_eq!(output.job_id.as_deref(), Some("job-2")); +} + +#[tokio::test] +async fn create_report_applies_description() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules().returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: Some("job-3".into()), + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock + .expect_update_report() + .withf(|req| { + let req = req.get_ref(); + let paths = req + .update_mask + .as_ref() + .map(|m| m.paths.clone()) + .unwrap_or_default(); + let report = req.report.as_ref().expect("report"); + paths == vec!["description".to_string()] + && report.report_id == "rep-new" + && report.description.as_deref() == Some("what this evaluates") + }) + .times(1) + .returning(|_| Ok(Response::new(UpdateReportResponse {}))); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-new".into(), + description: Some("what this evaluates".into()), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; + + let output = service + .create_report( + None, + "run-1".to_string(), + "x".to_string(), + Some("what this evaluates".to_string()), + vec![], + ReportSource::Rules { + rules: RuleIdentifier::RuleIds(vec!["rule-1".to_string()]), + }, + ) + .await + .expect("create_report failed"); + + assert_eq!( + output.report.description.as_deref(), + Some("what this evaluates") + ); +} + +#[tokio::test] +async fn create_report_skips_metadata_write_when_none_provided() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules().returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: None, + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-new".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_dual_mocks(report_mock, eval_mock).await; + + service + .create_report( + None, + "run-1".to_string(), + "x".to_string(), + None, + vec![], + ReportSource::Rules { + rules: RuleIdentifier::RuleIds(vec!["rule-1".to_string()]), + }, + ) + .await + .expect("create_report failed"); +} + +#[tokio::test] +async fn create_report_errors_when_no_report_id_returned() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules().returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: None, + job_id: None, + created_annotation_count: 0, + })) + }); + + let (service, _h) = service_with_dual_mocks(MockReportServiceImpl::new(), eval_mock).await; + + let err = service + .create_report( + None, + "run-1".to_string(), + "x".to_string(), + None, + vec![], + ReportSource::Rules { + rules: RuleIdentifier::RuleIds(vec!["rule-1".to_string()]), + }, + ) + .await + .expect_err("expected error"); + + assert!(err.to_string().contains("missing report_id")); } #[tokio::test] async fn create_report_propagates_grpc_error() { - let mut mock = MockReportServiceImpl::new(); - mock.expect_create_report() + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock + .expect_evaluate_rules() .returning(|_| Err(Status::invalid_argument("bad input"))); - let (service, _h) = service_with_mock(mock).await; + let (service, _h) = service_with_dual_mocks(MockReportServiceImpl::new(), eval_mock).await; let err = service .create_report( @@ -449,6 +745,7 @@ async fn create_report_propagates_grpc_error() { "run-1".to_string(), "x".to_string(), None, + vec![], ReportSource::Template { report_template_id: "tmpl-1".to_string(), }, diff --git a/rust/crates/sift_mcp/src/tool/annotations/mod.rs b/rust/crates/sift_mcp/src/tool/annotations/mod.rs index e110eca13..ce90546f1 100644 --- a/rust/crates/sift_mcp/src/tool/annotations/mod.rs +++ b/rust/crates/sift_mcp/src/tool/annotations/mod.rs @@ -120,7 +120,8 @@ impl SiftMcpServer { Guidance: - Narrow with `run_id == \"...\"` or `asset_id == \"...\"` when known — those are the most selective. - - Use `is_archived == false` to exclude archived annotations unless they're explicitly needed. + - Default add `is_archived == false` to the filter. Include archived annotations only when the user + explicitly asks for them. ", annotations(title = "annotations/list_annotations", read_only_hint = true) )] diff --git a/rust/crates/sift_mcp/src/tool/assets/mod.rs b/rust/crates/sift_mcp/src/tool/assets/mod.rs index 4699b56a4..6adf7e19b 100644 --- a/rust/crates/sift_mcp/src/tool/assets/mod.rs +++ b/rust/crates/sift_mcp/src/tool/assets/mod.rs @@ -59,7 +59,8 @@ impl SiftMcpServer { Guidance: - Narrow with `filter` whenever you know what you're looking for; pass `limit` regardless, so an open-ended listing stays bounded. - - Use `is_archived == false` to exclude archived assets unless they're explicitly needed. + - Default add `is_archived == false` to the filter. Include archived assets only when the user + explicitly asks for them. ", annotations(title = "assets/list_assets", read_only_hint = true) )] diff --git a/rust/crates/sift_mcp/src/tool/mod.rs b/rust/crates/sift_mcp/src/tool/mod.rs index 3479642dd..b82d00e17 100644 --- a/rust/crates/sift_mcp/src/tool/mod.rs +++ b/rust/crates/sift_mcp/src/tool/mod.rs @@ -6,6 +6,7 @@ pub mod data; pub mod docs; pub mod explore; pub mod ping; +pub mod report_templates; pub mod reports; pub mod rules; pub mod runs; diff --git a/rust/crates/sift_mcp/src/tool/report_templates/mod.rs b/rust/crates/sift_mcp/src/tool/report_templates/mod.rs new file mode 100644 index 000000000..4c885fe0a --- /dev/null +++ b/rust/crates/sift_mcp/src/tool/report_templates/mod.rs @@ -0,0 +1,371 @@ +use rmcp::{ + ErrorData, + handler::server::wrapper::Parameters, + model::{CallToolResult, ContentBlock}, + schemars::{self, JsonSchema}, + tool, tool_router, +}; +use serde::Deserialize; +use sift_rs::metadata::v1::MetadataValue; + +use crate::{ + error::{self, from_anyhow}, + server::SiftMcpServer, + service::report_templates::{ReportTemplateUpdate, TemplateRuleIdentifier}, + tool::common::MetadataEntry, +}; + +#[cfg(test)] +mod test; + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct ReportTemplateListParams { + filter: String, + order_by: Option, + limit: Option, + organization_id: Option, +} + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct CreateReportTemplateParams { + name: String, + description: Option, + client_key: Option, + tag_names: Option>, + organization_id: Option, + metadata: Option>, + rule_ids: Option>, + rule_client_keys: Option>, +} + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct UpdateReportTemplateParams { + report_template_id: String, + name: Option, + description: Option, + tag_names: Option>, + rule_ids: Option>, + rule_client_keys: Option>, + metadata: Option>, + is_archived: Option, +} + +#[tool_router(router = report_templates_router, vis = "pub(crate)")] +impl SiftMcpServer { + #[tool( + name = "list_report_templates", + description = " + List report templates in Sift, optionally filtered by a CEL expression and ordered by one or more fields. + A report template is a named, reusable bundle of rules; reports created from a template inherit its + rule set. Only standard rules (`is_external: false`) can be attached to a template; ad-hoc rules + (`is_external: true`) cannot. Wraps `report_templates/v1 ListReportTemplates`. + + Output: + - `{ \"report_templates\": [ReportTemplate, ...] }`. Each item carries `report_template_id`, + `organization_id`, `client_key`, `name`, `description`, `created_date`, `modified_date`, + `created_by_user_id`, `modified_by_user_id`, `is_archived`, `archived_date`, `tags`, `metadata`, + and the ordered `rules` list. Each rule includes `rule_id`, `rule_version_id`, + `rule_version_number`, `client_key`, and `display_order`. + + Parameters: + - `filter`: CEL expression. Pass an empty string to list everything. Filterable fields: + `report_template_id`, `tag_id`, `tag_name`, `client_key`, `metadata`, `name`, `is_archived`. + Reference metadata entries as `metadata.{key}` (e.g. `metadata.owner == \"qa-team\"`). + When filtering or searching, use `name.matches(\"(?i)avionics\")`, not `==`. Use `==` only for an + exact value from a prior result. `contains`/`startsWith`/`endsWith` are case-SENSITIVE: + `contains(\"Avionics\")` silently misses `avionics-power-limit`. + - `order_by`: optional comma-separated `FIELD_NAME[ desc]` list. Orderable fields: `created_date`, + `modified_date`. Default sort is `created_date desc` (newest first). Example: + `\"created_date desc,modified_date\"`. + - `limit`: max items to return. Start at 50 and only raise it if the result is capped and you still + need more. Values are clamped to `1..=200`; omitting it defaults to 50. + - `organization_id`: optional. Required only when the caller belongs to multiple organizations; + scopes the listing to that org. Omit for single-organization users. + + Errors: + - `INVALID_PARAMS` if `filter` is not a valid CEL expression or `order_by` references an unknown field. + - `INTERNAL_ERROR` for upstream gRPC failures. + + Guidance: + - Use this to discover a template before referencing it from `create_report` via `report_template_id`. + - Default add `is_archived == false` to the filter. Include archived templates only when the user + explicitly asks for them. + ", + annotations( + title = "report_templates/list_report_templates", + read_only_hint = true + ) + )] + pub async fn list_report_templates( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(ReportTemplateListParams { + filter, + order_by, + limit, + organization_id, + }) = params; + + let templates = self + .report_template_service + .list_report_templates(filter, order_by, limit, organization_id) + .await + .map_err(from_anyhow)?; + + Ok(CallToolResult::structured( + serde_json::json!({ "report_templates": templates }), + )) + } + + #[tool( + name = "create_report_template", + description = " + Create a report template from a set of standard rules. A report template is a named, reusable + bundle of rules that any future `create_report` call can reference via `report_template_id`. Only + standard rules (`is_external: false`) can be attached; ad-hoc rules (`is_external: true`) are + rejected by the server. Wraps `report_templates/v1 CreateReportTemplate`. + + Output: + - `{ \"report_template\": ReportTemplate, \"next_step\": string }`. The returned template is the + server-assigned state including its new `report_template_id`, resolved `rules` (with + `display_order` set), and timestamps. + + Parameters: + - `name`: required; the template name. + - `description`: optional; free-form description of what the template evaluates. + - `client_key`: optional; a stable caller-defined key for referring to this template. Must be unique + within the organization. + - `tag_names`: optional list of tag names to attach to the template. + - `organization_id`: optional. Required only when the caller belongs to multiple organizations. + - `metadata`: optional list of `{ \"name\": \"\", \"value\": }` entries. + + Provide EXACTLY ONE of the following to identify the rules on the new template: + - `rule_ids`: ordered list of rule IDs. Position in the list becomes each rule's `display_order` + on the template (first = 0, second = 1, ...). + - `rule_client_keys`: list of rule client keys. Server resolves keys to rules; the resulting rule + order on the template is server-defined (not the position in this list). + + Errors: + - `INVALID_PARAMS` if `name` is empty, or if zero or more than one of `rule_ids`/`rule_client_keys` + is provided. + - `INTERNAL_ERROR` for upstream gRPC failures (e.g. unknown rule). + + Guidance: + - This is a write. CONFIRM the template name and the rule set with the user before invoking. + - Prefer `rule_ids` when you want deterministic on-template ordering. + - After creation, invoke `create_report` with the new `report_template_id` to run the template + over a run. + ", + annotations( + title = "report_templates/create_report_template", + read_only_hint = false, + destructive_hint = false, + idempotent_hint = false, + ) + )] + pub async fn create_report_template( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(CreateReportTemplateParams { + name, + description, + client_key, + tag_names, + organization_id, + metadata, + rule_ids, + rule_client_keys, + }) = params; + + if name.is_empty() { + return Err(ErrorData::invalid_params("`name` must not be empty", None)); + } + + let rules = match (rule_ids, rule_client_keys) { + (Some(ids), None) => TemplateRuleIdentifier::RuleIds(ids), + (None, Some(keys)) => TemplateRuleIdentifier::RuleClientKeys(keys), + (None, None) => { + return Err(ErrorData::invalid_params( + "provide exactly one of `rule_ids` or `rule_client_keys` to identify the template's rules", + None, + )); + } + (Some(_), Some(_)) => { + return Err(ErrorData::invalid_params( + "provide only one of `rule_ids` or `rule_client_keys`, not both", + None, + )); + } + }; + + let metadata = metadata + .map(|m| m.into_iter().map(MetadataValue::from).collect::>()) + .unwrap_or_default(); + + let template = self + .report_template_service + .create_report_template( + organization_id, + name, + client_key, + description, + tag_names.unwrap_or_default(), + rules, + metadata, + ) + .await + .map_err(from_anyhow)?; + + let next_step = format!( + "Created report template `{}` ({}) with {} rule(s). Surface it to the user. To run the template \ + over a run, call `create_report` with `report_template_id = \"{}\"`.", + template.name, + template.report_template_id, + template.rules.len(), + template.report_template_id, + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "report_template": template, + "next_step": next_step, + })); + result.content = vec![ContentBlock::text(next_step)]; + Ok(result) + } + + #[tool( + name = "update_report_template", + description = " + Update selected fields on an existing report template. Wraps `report_templates/v1 UpdateReportTemplate`. + + Output: + - `{ \"report_template\": ReportTemplate, \"next_step\": string }`. The returned template reflects + the post-update server state. + + Parameters: + - `report_template_id`: required; the id of the template to update. + - `name`: optional new name. + - `description`: optional new description. + - `tag_names`: optional REPLACEMENT list of tag names. Passing this overwrites the template's full + tag list; pass `[]` to clear all tags. + - `metadata`: optional REPLACEMENT metadata list of `{ \"name\": ..., \"value\": ... }` entries. + Passing this overwrites the template's full metadata; pass `[]` to clear. + - `is_archived`: optional; set to `true` to archive the template or `false` to unarchive it. + + To replace the template's rule set, provide EXACTLY ONE of: + - `rule_ids`: ordered list of rule IDs. Position becomes each rule's `display_order`. + - `rule_client_keys`: list of rule client keys. Server resolves keys; on-template order is + server-defined. + Providing both is rejected. Omit both to leave the rule set untouched. + + At least one updatable field (`name`, `description`, `tag_names`, `rule_ids`, `rule_client_keys`, + `metadata`, or `is_archived`) must be set. + + Errors: + - `INVALID_PARAMS` if `report_template_id` is empty, no updatable field is provided, or both + `rule_ids` and `rule_client_keys` are provided. + - `RESOURCE_NOT_FOUND` if no template matches `report_template_id`. + - `INTERNAL_ERROR` for upstream gRPC failures. + + Guidance: + - This is a write with REPLACE semantics on `tags`, `rules`, and `metadata`. CONFIRM the intended + shape with the user — for appends, read the current template via `list_report_templates` filtered + by `report_template_id == \"\"` and send the union. + - Reports already created from this template are NOT retroactively updated; only future reports + created from the template pick up the change. + ", + annotations( + title = "report_templates/update_report_template", + read_only_hint = false, + destructive_hint = true, + idempotent_hint = true, + ) + )] + pub async fn update_report_template( + &self, + params: Parameters, + ) -> error::McpResult { + self.require_destructive()?; + + let Parameters(UpdateReportTemplateParams { + report_template_id, + name, + description, + tag_names, + rule_ids, + rule_client_keys, + metadata, + is_archived, + }) = params; + + if report_template_id.is_empty() { + return Err(ErrorData::invalid_params( + "`report_template_id` must not be empty", + None, + )); + } + + let rules = match (rule_ids, rule_client_keys) { + (None, None) => None, + (Some(ids), None) => Some(TemplateRuleIdentifier::RuleIds(ids)), + (None, Some(keys)) => Some(TemplateRuleIdentifier::RuleClientKeys(keys)), + (Some(_), Some(_)) => { + return Err(ErrorData::invalid_params( + "provide only one of `rule_ids` or `rule_client_keys`, not both", + None, + )); + } + }; + + let metadata = metadata.map(|m| m.into_iter().map(MetadataValue::from).collect::>()); + + let changes = ReportTemplateUpdate { + name, + description, + tag_names, + rules, + metadata, + is_archived, + }; + + if changes.name.is_none() + && changes.description.is_none() + && changes.tag_names.is_none() + && changes.rules.is_none() + && changes.metadata.is_none() + && changes.is_archived.is_none() + { + return Err(ErrorData::invalid_params( + "provide at least one field to update: `name`, `description`, `tag_names`, `rule_ids`, \ + `rule_client_keys`, `metadata`, or `is_archived`", + None, + )); + } + + let template = self + .report_template_service + .update_report_template(report_template_id, changes) + .await + .map_err(from_anyhow)?; + + let archive_clause = if template.is_archived { + " Template is now archived." + } else { + "" + }; + let next_step = format!( + "Updated report template `{}` ({}).{} Surface the new state to the user and confirm nothing was \ + unintentionally dropped — `tags`, `rules`, and `metadata` use REPLACE semantics.", + template.name, template.report_template_id, archive_clause, + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "report_template": template, + "next_step": next_step, + })); + result.content = vec![ContentBlock::text(next_step)]; + Ok(result) + } +} diff --git a/rust/crates/sift_mcp/src/tool/report_templates/test.rs b/rust/crates/sift_mcp/src/tool/report_templates/test.rs new file mode 100644 index 000000000..c1ec33178 --- /dev/null +++ b/rust/crates/sift_mcp/src/tool/report_templates/test.rs @@ -0,0 +1,311 @@ +use rmcp::{handler::server::wrapper::Parameters, model::ErrorCode}; +use sift_rs::report_templates::v1::{ + CreateReportTemplateResponse, ListReportTemplatesResponse, ReportTemplate, + UpdateReportTemplateResponse, report_template_service_server::ReportTemplateServiceServer, +}; +use sift_test_util::{ + grpc::memory_sift_channel, mock::report_templates::v1::MockReportTemplateServiceImpl, +}; +use tokio::task::JoinHandle; +use tonic::{Response, Status, transport::Server}; + +use super::{CreateReportTemplateParams, ReportTemplateListParams, UpdateReportTemplateParams}; +use crate::{server::SiftMcpServer, tool::common::test_support::structured_field}; + +async fn server_with_mock(mock: MockReportTemplateServiceImpl) -> (SiftMcpServer, JoinHandle<()>) { + server_with_mock_and_flag(mock, true).await +} + +async fn server_with_mock_and_flag( + mock: MockReportTemplateServiceImpl, + allow_destructive: bool, +) -> (SiftMcpServer, JoinHandle<()>) { + let (client, server) = tokio::io::duplex(1024); + let channel = memory_sift_channel(client).await; + + let handle = tokio::spawn(async move { + Server::builder() + .add_service(ReportTemplateServiceServer::new(mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + SiftMcpServer::new( + channel, + String::from("https://app.test.local"), + allow_destructive, + ), + handle, + ) +} + +fn create_params() -> CreateReportTemplateParams { + CreateReportTemplateParams { + name: "safety".into(), + description: None, + client_key: None, + tag_names: None, + organization_id: None, + metadata: None, + rule_ids: None, + rule_client_keys: None, + } +} + +fn update_params() -> UpdateReportTemplateParams { + UpdateReportTemplateParams { + report_template_id: "tmpl-1".into(), + name: None, + description: None, + tag_names: None, + rule_ids: None, + rule_client_keys: None, + metadata: None, + is_archived: None, + } +} + +#[tokio::test] +async fn list_report_templates_returns_single_page() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .withf(|req| req.get_ref().filter == "name == \"safety\"") + .returning(|_| { + Ok(Response::new(ListReportTemplatesResponse { + report_templates: vec![ReportTemplate { + report_template_id: "tmpl-1".into(), + name: "safety".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let resp = server + .list_report_templates(Parameters(ReportTemplateListParams { + filter: "name == \"safety\"".into(), + order_by: None, + limit: None, + organization_id: None, + })) + .await + .expect("list_report_templates failed"); + + let templates = structured_field(resp, "report_templates"); + let arr = templates.as_array().unwrap(); + assert_eq!(arr.len(), 1); + assert_eq!(arr[0]["reportTemplateId"], "tmpl-1"); +} + +#[tokio::test] +async fn list_report_templates_forwards_organization_id() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .withf(|req| req.get_ref().organization_id == "org-7") + .returning(|_| { + Ok(Response::new(ListReportTemplatesResponse { + report_templates: vec![ReportTemplate { + report_template_id: "tmpl-1".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + server + .list_report_templates(Parameters(ReportTemplateListParams { + filter: String::new(), + order_by: None, + limit: None, + organization_id: Some("org-7".into()), + })) + .await + .expect("list_report_templates failed"); +} + +#[tokio::test] +async fn list_report_templates_propagates_grpc_error() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_list_report_templates() + .returning(|_| Err(Status::invalid_argument("bad filter"))); + + let (server, _h) = server_with_mock(mock).await; + + let err = server + .list_report_templates(Parameters(ReportTemplateListParams { + filter: "nope".into(), + order_by: None, + limit: None, + organization_id: None, + })) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); + assert!(err.message.contains("bad filter")); +} + +#[tokio::test] +async fn create_report_template_happy_path() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_create_report_template().returning(|_| { + Ok(Response::new(CreateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-new".into(), + name: "safety".into(), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let mut params = create_params(); + params.rule_ids = Some(vec!["rule-1".into(), "rule-2".into()]); + + let resp = server + .create_report_template(Parameters(params)) + .await + .expect("create_report_template failed"); + + let template = structured_field(resp, "report_template"); + assert_eq!(template["reportTemplateId"], "tmpl-new"); +} + +#[tokio::test] +async fn create_report_template_rejects_empty_name() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let mut params = create_params(); + params.name = String::new(); + params.rule_ids = Some(vec!["rule-1".into()]); + + let err = server + .create_report_template(Parameters(params)) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn create_report_template_rejects_no_rules() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let err = server + .create_report_template(Parameters(create_params())) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn create_report_template_rejects_both_rule_shapes() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let mut params = create_params(); + params.rule_ids = Some(vec!["rule-1".into()]); + params.rule_client_keys = Some(vec!["ck-1".into()]); + + let err = server + .create_report_template(Parameters(params)) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn update_report_template_happy_path() { + let mut mock = MockReportTemplateServiceImpl::new(); + mock.expect_update_report_template().returning(|_| { + Ok(Response::new(UpdateReportTemplateResponse { + report_template: Some(ReportTemplate { + report_template_id: "tmpl-1".into(), + name: "renamed".into(), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let mut params = update_params(); + params.name = Some("renamed".into()); + + let resp = server + .update_report_template(Parameters(params)) + .await + .expect("update_report_template failed"); + + let template = structured_field(resp, "report_template"); + assert_eq!(template["name"], "renamed"); +} + +#[tokio::test] +async fn update_report_template_rejects_empty_id() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let mut params = update_params(); + params.report_template_id = String::new(); + params.name = Some("renamed".into()); + + let err = server + .update_report_template(Parameters(params)) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn update_report_template_rejects_no_fields() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let err = server + .update_report_template(Parameters(update_params())) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn update_report_template_rejects_both_rule_shapes() { + let (server, _h) = server_with_mock(MockReportTemplateServiceImpl::new()).await; + + let mut params = update_params(); + params.rule_ids = Some(vec!["rule-1".into()]); + params.rule_client_keys = Some(vec!["ck-1".into()]); + + let err = server + .update_report_template(Parameters(params)) + .await + .expect_err("expected error"); + + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn update_report_template_blocked_without_allow_destructive() { + let mock = MockReportTemplateServiceImpl::new(); + let (server, _h) = server_with_mock_and_flag(mock, false).await; + + let mut params = update_params(); + params.name = Some("renamed".into()); + + let err = server + .update_report_template(Parameters(params)) + .await + .expect_err("expected destructive gate to reject the call"); + + assert_eq!(err.code, ErrorCode::INVALID_REQUEST); + assert!(err.message.contains("--allow-destructive")); +} diff --git a/rust/crates/sift_mcp/src/tool/reports/mod.rs b/rust/crates/sift_mcp/src/tool/reports/mod.rs index 20d9044d7..bc811ff72 100644 --- a/rust/crates/sift_mcp/src/tool/reports/mod.rs +++ b/rust/crates/sift_mcp/src/tool/reports/mod.rs @@ -42,7 +42,6 @@ pub struct CreateReportParams { metadata: Option>, report_template_id: Option, description: Option, - tag_names: Option>, rule_ids: Option>, rule_client_keys: Option>, rule_version_ids: Option>, @@ -91,7 +90,8 @@ impl SiftMcpServer { Guidance: - When the report's run is known, narrow with `run_id == \"...\"` first — it's the most selective field. - - Use `is_archived == false` to exclude archived reports unless they're explicitly needed. + - Default add `is_archived == false` to the filter. Include archived reports only when the user + explicitly asks for them. - Order by `created_date desc` when surfacing the most recent reports to a user. ", annotations(title = "reports/list_reports", read_only_hint = true) @@ -196,33 +196,35 @@ impl SiftMcpServer { #[tool( name = "create_report", description = " - Create a report over a run, either from a report template or from an explicit set of rules. Wraps - `reports/v1 CreateReport`. + Create a report over a run and start its evaluation. Output: - - `{ \"report\": Report, \"report_url\": string|null, \"next_step\": string }`. The returned `Report` - is the server-assigned state including its new `report_id` and `job_id`. `report_url` is the report's - Sift web link (`/reports/`), or null on self-hosted deployments where the host - can't be derived. + - `{ \"report\": Report, \"report_url\": string|null, \"job_id\": string|null, + \"created_annotation_count\": number, \"next_step\": string }`. The returned `Report` may still + be in a running state — poll `list_report_rule_summaries` to track progress. `report_url` is + the report's Sift web link, or null when it can't be derived. Parameters: - `run_id`: required; the run the report is generated over. - `name`: required; the report name. - `organization_id`: optional. Required only when the caller belongs to multiple organizations. - - `metadata`: optional list of `{ \"name\": \"\", \"value\": }` entries. + - `description`: optional free-form description. + - `metadata`: optional list of `{ \"name\": \"\", \"value\": }` entries (REPLACE + semantics on the metadata list). The report SOURCE is one of two mutually exclusive shapes — provide exactly one: - Template: set `report_template_id`. The template defines which rules run. - Rules: leave `report_template_id` unset and provide EXACTLY ONE of `rule_ids`, `rule_client_keys`, - or `rule_version_ids`. `description` and `tag_names` are optional and apply only to this shape. + or `rule_version_ids`. `rule_ids` / `rule_client_keys` resolve to each rule's current version; + `rule_version_ids` pins to specific historical versions. Errors: - `INVALID_PARAMS` if `run_id` or `name` is empty, if both a template and rule identifiers are given, if neither is given, or if more than one rule-identifier list is given. - - `INTERNAL_ERROR` for upstream gRPC failures (e.g. unknown run, template, or rule). + - `INTERNAL_ERROR` for upstream gRPC failures. Guidance: - - This is a write that kicks off report execution. CONFIRM the run, source, and name with the user + - This is a write that starts report execution. CONFIRM the run, source, and name with the user before invoking. - Use `list_report_rule_summaries` on the returned `report_id` to track per-rule progress. ", @@ -241,7 +243,6 @@ impl SiftMcpServer { metadata, report_template_id, description, - tag_names, rule_ids, rule_client_keys, rule_version_ids, @@ -278,11 +279,7 @@ impl SiftMcpServer { .flatten() .next() .expect("one source"); - ReportSource::Rules { - description, - tag_names: tag_names.unwrap_or_default(), - rules, - } + ReportSource::Rules { rules } } (None, 0) => { return Err(ErrorData::invalid_params( @@ -299,26 +296,39 @@ impl SiftMcpServer { } }; - let metadata = metadata.map(|m| m.into_iter().map(MetadataValue::from).collect::>()); + let metadata = metadata + .map(|m| m.into_iter().map(MetadataValue::from).collect::>()) + .unwrap_or_default(); - let report = self + let output = self .report_service - .create_report(organization_id, run_id, name, metadata, source) + .create_report(organization_id, run_id, name, description, metadata, source) .await .map_err(from_anyhow)?; - let report_url = self.url_service.build_report_url(&report.report_id).ok(); + let report_url = self + .url_service + .build_report_url(&output.report.report_id) + .ok(); + let job_clause = output + .job_id + .as_deref() + .map(|jid| format!(" Evaluation job `{jid}` is running asynchronously.")) + .unwrap_or_default(); let next_step = format!( - "Created report `{}` ({}).{} Surface it to the user. Use `list_report_rule_summaries` \ - with this `report_id` to track per-rule progress.", - report.name, - report.report_id, + "Created report `{}` ({}) and queued its evaluation.{}{} Surface it to the user. Use \ + `list_report_rule_summaries` with this `report_id` to track per-rule progress.", + output.report.name, + output.report.report_id, + job_clause, url_clause(report_url.as_deref()), ); let mut result = CallToolResult::structured(serde_json::json!({ - "report": report, + "report": output.report, "report_url": report_url, + "job_id": output.job_id, + "created_annotation_count": output.created_annotation_count, "next_step": next_step, })); result.content = vec![ContentBlock::text(next_step)]; diff --git a/rust/crates/sift_mcp/src/tool/reports/test.rs b/rust/crates/sift_mcp/src/tool/reports/test.rs index e902ff7a7..b5d153e6d 100644 --- a/rust/crates/sift_mcp/src/tool/reports/test.rs +++ b/rust/crates/sift_mcp/src/tool/reports/test.rs @@ -1,9 +1,19 @@ use rmcp::{handler::server::wrapper::Parameters, model::ErrorCode}; -use sift_rs::reports::v1::{ - CreateReportResponse, GetReportResponse, ListReportRuleSummariesResponse, ListReportsResponse, - Report, ReportRuleSummary, UpdateReportResponse, report_service_server::ReportServiceServer, +use sift_rs::{ + reports::v1::{ + GetReportResponse, ListReportRuleSummariesResponse, ListReportsResponse, Report, + ReportRuleSummary, UpdateReportResponse, report_service_server::ReportServiceServer, + }, + rule_evaluation::v1::{ + EvaluateRulesResponse, rule_evaluation_service_server::RuleEvaluationServiceServer, + }, +}; +use sift_test_util::{ + grpc::memory_sift_channel, + mock::{ + reports::v1::MockReportServiceImpl, rule_evaluation::v1::MockRuleEvaluationServiceImpl, + }, }; -use sift_test_util::{grpc::memory_sift_channel, mock::reports::v1::MockReportServiceImpl}; use tokio::task::JoinHandle; use tonic::{Response, Status, transport::Server}; @@ -20,7 +30,6 @@ fn create_report_params() -> CreateReportParams { metadata: None, report_template_id: None, description: None, - tag_names: None, rule_ids: None, rule_client_keys: None, rule_version_ids: None, @@ -45,6 +54,28 @@ async fn server_with_mock(mock: MockReportServiceImpl) -> (SiftMcpServer, JoinHa ) } +async fn server_with_dual_mocks( + report_mock: MockReportServiceImpl, + eval_mock: MockRuleEvaluationServiceImpl, +) -> (SiftMcpServer, JoinHandle<()>) { + let (client, server) = tokio::io::duplex(1024); + let channel = memory_sift_channel(client).await; + + let handle = tokio::spawn(async move { + Server::builder() + .add_service(ReportServiceServer::new(report_mock)) + .add_service(RuleEvaluationServiceServer::new(eval_mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + SiftMcpServer::new(channel, String::from("https://app.test.local"), true), + handle, + ) +} + fn report_params(filter: &str, limit: Option) -> Parameters { Parameters(ReportListParams { filter: filter.into(), @@ -313,9 +344,18 @@ async fn list_report_rule_summaries_rejects_empty_report_id() { #[tokio::test] async fn create_report_from_rules_happy_path() { - let mut mock = MockReportServiceImpl::new(); - mock.expect_create_report().returning(|_| { - Ok(Response::new(CreateReportResponse { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules().returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: Some("job-1".into()), + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { report: Some(Report { report_id: "rep-new".into(), name: "nightly report".into(), @@ -324,7 +364,7 @@ async fn create_report_from_rules_happy_path() { })) }); - let (server, _h) = server_with_mock(mock).await; + let (server, _h) = server_with_dual_mocks(report_mock, eval_mock).await; let mut params = create_report_params(); params.rule_ids = Some(vec!["rule-1".into()]); @@ -336,10 +376,62 @@ async fn create_report_from_rules_happy_path() { let report_url = structured_field(resp.clone(), "report_url"); assert_eq!(report_url, "https://app.test.local/reports/rep-new"); + let job_id = structured_field(resp.clone(), "job_id"); + assert_eq!(job_id, "job-1"); let report = structured_field(resp, "report"); assert_eq!(report["reportId"], "rep-new"); } +#[tokio::test] +async fn create_report_applies_description() { + let mut eval_mock = MockRuleEvaluationServiceImpl::new(); + eval_mock.expect_evaluate_rules().returning(|_| { + Ok(Response::new(EvaluateRulesResponse { + report_id: Some("rep-new".into()), + job_id: None, + created_annotation_count: 0, + })) + }); + + let mut report_mock = MockReportServiceImpl::new(); + report_mock + .expect_update_report() + .withf(|req| { + let paths = req + .get_ref() + .update_mask + .as_ref() + .map(|m| m.paths.clone()) + .unwrap_or_default(); + paths == vec!["description".to_string()] + }) + .times(1) + .returning(|_| Ok(Response::new(UpdateReportResponse {}))); + report_mock.expect_get_report().returning(|_| { + Ok(Response::new(GetReportResponse { + report: Some(Report { + report_id: "rep-new".into(), + description: Some("evaluates nightly runs".into()), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_dual_mocks(report_mock, eval_mock).await; + + let mut params = create_report_params(); + params.rule_ids = Some(vec!["rule-1".into()]); + params.description = Some("evaluates nightly runs".into()); + + let resp = server + .create_report(Parameters(params)) + .await + .expect("create_report failed"); + + let report = structured_field(resp, "report"); + assert_eq!(report["description"], "evaluates nightly runs"); +} + #[tokio::test] async fn create_report_rejects_no_source() { let (server, _h) = server_with_mock(MockReportServiceImpl::new()).await; diff --git a/rust/crates/sift_mcp/src/tool/rules/mod.rs b/rust/crates/sift_mcp/src/tool/rules/mod.rs index f8a716824..5835684b7 100644 --- a/rust/crates/sift_mcp/src/tool/rules/mod.rs +++ b/rust/crates/sift_mcp/src/tool/rules/mod.rs @@ -92,7 +92,8 @@ impl SiftMcpServer { Guidance: - Scope with `asset_id == \"...\"` when the rule's target asset is known — it's the most selective field for narrowing rule listings. - - Use `is_archived == false` to exclude archived rules unless they're explicitly needed. + - Default add `is_archived == false` to the filter. Include archived rules only when the user + explicitly asks for them. - Use `is_live_evaluation_enabled == true` to find only rules that run against live data. ", annotations(title = "rules/list_rules", read_only_hint = true) diff --git a/rust/crates/sift_mcp/src/tool/runs/mod.rs b/rust/crates/sift_mcp/src/tool/runs/mod.rs index d55928c1f..01ae4f1ad 100644 --- a/rust/crates/sift_mcp/src/tool/runs/mod.rs +++ b/rust/crates/sift_mcp/src/tool/runs/mod.rs @@ -71,6 +71,8 @@ impl SiftMcpServer { - To find runs covering a specific moment, filter on both `start_time` and `stop_time` rather than pulling everything and filtering client-side. - Order by `start_time desc` when surfacing the most recent runs to a user. + - Default add `is_archived == false` to the filter. Include archived runs only when the user + explicitly asks for them. ", annotations(title = "runs/list_runs", read_only_hint = true) )] diff --git a/rust/crates/sift_mcp/src/tool/test_reports/mod.rs b/rust/crates/sift_mcp/src/tool/test_reports/mod.rs index 91ba90414..7b9cd1ca3 100644 --- a/rust/crates/sift_mcp/src/tool/test_reports/mod.rs +++ b/rust/crates/sift_mcp/src/tool/test_reports/mod.rs @@ -83,6 +83,8 @@ impl SiftMcpServer { Guidance: - To audit a single run, resolve the report first (by `name`, `run_id`, or `test_case`), then pass its `test_report_id` to the step and measurement tools. + - Default add `is_archived == false` to the filter. Include archived test reports only when the + user explicitly asks for them. ", annotations(title = "test_reports/list_test_reports", read_only_hint = true) )] diff --git a/rust/crates/sift_test_util/src/mock/mod.rs b/rust/crates/sift_test_util/src/mock/mod.rs index dee6c5285..114d1917a 100644 --- a/rust/crates/sift_test_util/src/mock/mod.rs +++ b/rust/crates/sift_test_util/src/mock/mod.rs @@ -4,7 +4,9 @@ pub mod channels; pub mod data; pub mod docs; pub mod me; +pub mod report_templates; pub mod reports; +pub mod rule_evaluation; pub mod rules; pub mod runs; pub mod test_reports; diff --git a/rust/crates/sift_test_util/src/mock/report_templates/mod.rs b/rust/crates/sift_test_util/src/mock/report_templates/mod.rs new file mode 100644 index 000000000..a3a6d96c3 --- /dev/null +++ b/rust/crates/sift_test_util/src/mock/report_templates/mod.rs @@ -0,0 +1 @@ +pub mod v1; diff --git a/rust/crates/sift_test_util/src/mock/report_templates/v1.rs b/rust/crates/sift_test_util/src/mock/report_templates/v1.rs new file mode 100644 index 000000000..a026c9695 --- /dev/null +++ b/rust/crates/sift_test_util/src/mock/report_templates/v1.rs @@ -0,0 +1,45 @@ +use async_trait::async_trait; +use mockall::mock; +use sift_rs::report_templates::v1::{ + CreateReportTemplateRequest, CreateReportTemplateResponse, GetReportTemplateRequest, + GetReportTemplateResponse, ListReportTemplatesRequest, ListReportTemplatesResponse, + UpdateReportTemplateRequest, UpdateReportTemplateResponse, + report_template_service_server::ReportTemplateService, +}; +use tonic::{Request, Response, Status}; + +mock! { + pub ReportTemplateServiceImpl {} + + #[async_trait] + impl ReportTemplateService for ReportTemplateServiceImpl { + async fn get_report_template( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + async fn create_report_template( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + async fn list_report_templates( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + async fn update_report_template( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + } +} diff --git a/rust/crates/sift_test_util/src/mock/rule_evaluation/mod.rs b/rust/crates/sift_test_util/src/mock/rule_evaluation/mod.rs new file mode 100644 index 000000000..a3a6d96c3 --- /dev/null +++ b/rust/crates/sift_test_util/src/mock/rule_evaluation/mod.rs @@ -0,0 +1 @@ +pub mod v1; diff --git a/rust/crates/sift_test_util/src/mock/rule_evaluation/v1.rs b/rust/crates/sift_test_util/src/mock/rule_evaluation/v1.rs new file mode 100644 index 000000000..f78477abf --- /dev/null +++ b/rust/crates/sift_test_util/src/mock/rule_evaluation/v1.rs @@ -0,0 +1,29 @@ +use async_trait::async_trait; +use mockall::mock; +use sift_rs::rule_evaluation::v1::{ + EvaluateRulesPreviewRequest, EvaluateRulesPreviewResponse, EvaluateRulesRequest, + EvaluateRulesResponse, rule_evaluation_service_server::RuleEvaluationService, +}; +use tonic::{Request, Response, Status}; + +mock! { + pub RuleEvaluationServiceImpl {} + + #[async_trait] + impl RuleEvaluationService for RuleEvaluationServiceImpl { + async fn evaluate_rules( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + async fn evaluate_rules_preview( + &self, + request: Request, + ) -> std::result::Result< + Response, + Status, + >; + } +}